Patch to assembler to strip LOCK prefix.

PATCH New Kinsella 3 participants 5 messages Oct 2, 2015 – Oct 7, 2015

Share & tools

Kinsella

2015-10-02 11:49 UTC #0 lore raw

PATCH v5 2/2 Patch to assembler to strip LOCK prefix.

The lock prefix is usually stripped by passing to the compiler -Wa,momit-lock-prefix=yes via EXTRA_CFLAGS. However I couldn't find a way to get buildroot to pass CFLAGS/EXTRA_CFLAGS around.

I tried using BR2_TARGET_OPTIMIZATION, and creating a new processor type but the flag kept stripping it out. I found the most reliable way to achieve the behavior was to make stripping the LOCK prefix the assembler's default behavior, hence the patch

Signed-off-by Ray Kinsella <[email protected]>

---
 .../galileo/patches/binutils/2.25/default-lock-prefix.patch   | 11 +++++++++++
 1 file changed, 11 insertions(+)
 create mode 100644 board/intel/galileo/patches/binutils/2.25/default-lock-prefix.patch

board/intel/galileo/patches/binutils/2.25/default-lock-prefix.patch new file +11 -0

new file mode 100644
index 0000000..7814ae4
@@ -0,0 +1,11 @@
+--- a/gas/config/tc-i386.orig	2015-07-24 15:54:51.359080778 +0100
++++ b/gas/config/tc-i386.c	2015-07-24 15:54:59.692192247 +0100
+@@ -545,7 +545,7 @@ static int allow_index_reg = 0;
+ 
+ /* 1 if the assembler should ignore LOCK prefix, even if it was
+    specified explicitly.  */
+-static int omit_lock_prefix = 0;
++static int omit_lock_prefix = 1;
+ 
+ static enum check_kind
+   {

Signature · 1 line

2.1.0

Arnout Vandecappelle

2015-10-04 18:15 UTC #1 lore raw

PATCH v5 2/2 Patch to assembler to strip LOCK prefix.

Hi Ray,

Quoted 8 lines · Ray

The lock prefix is usually stripped by passing to the compiler -Wa,momit-lock-prefix=yes via EXTRA_CFLAGS. However I couldn't find a way to get buildroot to pass CFLAGS/EXTRA_CFLAGS around.
I tried using BR2_TARGET_OPTIMIZATION, and creating a new processor type but the flag kept stripping it out. I found the most reliable way to achieve the behavior was to make stripping the LOCK prefix the assembler's default behavior, hence the patch

We now have a toolchain wrapper both for internal and external toolchains. This
will allow you to reliably pass options to the compiler. So we'd preferably use that approach.

Also, we think it's worthwhile to add a new subarchitecture for the Quark in
arch/Config.in.x86. This should set the arch and tune options to i586, but the toolchain wrapper can then carry an option that adds -Wa,-momit-lock-prefix=yes. This also fixes the problem for external toolchains.

So the proposal is that you create a first patch that adds BR2_x86_quark and
adds -Wa,-momit-lock-prefix=yes, and then a second patch that adds the Galileo2 defconfig.

I have now marked both patches as Changes Requested in out patch tracking
system, so please resubmit a new version that fixes the above comments, otherwise we'll forget about your contribution.

Regards,
Arnout

Quoted 24 lines

Signed-off-by Ray Kinsella <[email protected]>

---
 .../galileo/patches/binutils/2.25/default-lock-prefix.patch   | 11 +++++++++++
 1 file changed, 11 insertions(+)
 create mode 100644 board/intel/galileo/patches/binutils/2.25/default-lock-prefix.patch

board/intel/galileo/patches/binutils/2.25/default-lock-prefix.patch new file +11 -0

new file mode 100644
index 0000000..7814ae4
@@ -0,0 +1,11 @@
+--- a/gas/config/tc-i386.orig	2015-07-24 15:54:51.359080778 +0100
++++ b/gas/config/tc-i386.c	2015-07-24 15:54:59.692192247 +0100
+@@ -545,7 +545,7 @@ static int allow_index_reg = 0;
+ 
+ /* 1 if the assembler should ignore LOCK prefix, even if it was
+    specified explicitly.  */
+-static int omit_lock_prefix = 0;
++static int omit_lock_prefix = 1;
+ 
+ static enum check_kind
+   {

Signature · 6 lines

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

Kinsella

2015-10-07 09:53 UTC #2 lore raw

PATCH v5 2/2 Patch to assembler to strip LOCK prefix.

Hi Arnout,

Quoted 3 lines · Arnout Vandecappelle

We now have a toolchain wrapper both for internal and external toolchains. This
will allow you to reliably pass options to the compiler. So we'd preferably use that approach.

Ok - I will investigate - any examples I can look at?

Quoted 4 lines

Also, we think it's worthwhile to add a new subarchitecture for the Quark in
arch/Config.in.x86. This should set the arch and tune options to i586, but the toolchain wrapper can then carry an option that adds -Wa,-momit-lock-prefix=yes. This also fixes the problem for external toolchains.

Ok - I actually proto-typed this and I couldn't get it to work reliably, hence the nasty patch.
I should have it on a branch (I hope), I can dust it off.

Quoted 3 lines

So the proposal is that you create a first patch that adds BR2_x86_quark and
adds -Wa,-momit-lock-prefix=yes, and then a second patch that adds the Galileo2 defconfig.

Quoted 3 lines

I have now marked both patches as Changes Requested in out patch tracking
system, so please resubmit a new version that fixes the above comments, otherwise we'll forget about your contribution.

ok - will followup.
(is the patch tracking system public?)

Ray K

Quoted 31 lines

Regards,
Arnout

Quoted 24 lines

Signed-off-by Ray Kinsella <[email protected]>

---
 .../galileo/patches/binutils/2.25/default-lock-prefix.patch   | 11 +++++++++++
 1 file changed, 11 insertions(+)
 create mode 100644 board/intel/galileo/patches/binutils/2.25/default-lock-prefix.patch

board/intel/galileo/patches/binutils/2.25/default-lock-prefix.patch new file +11 -0

new file mode 100644
index 0000000..7814ae4
@@ -0,0 +1,11 @@
+--- a/gas/config/tc-i386.orig	2015-07-24 15:54:51.359080778 +0100
++++ b/gas/config/tc-i386.c	2015-07-24 15:54:59.692192247 +0100
+@@ -545,7 +545,7 @@ static int allow_index_reg = 0;
+ 
+ /* 1 if the assembler should ignore LOCK prefix, even if it was
+    specified explicitly.  */
+-static int omit_lock_prefix = 0;
++static int omit_lock_prefix = 1;
+ 
+ static enum check_kind
+   {

Baruch Siach

2015-10-07 10:09 UTC #3 lore raw

PATCH v5 2/2 Patch to assembler to strip LOCK prefix.

Hi Ray,

Quoted 7 lines · Ray

Quoted 3 lines · Arnout Vandecappelle
I have now marked both patches as Changes Requested in out patch tracking
system, so please resubmit a new version that fixes the above comments, otherwise we'll forget about your contribution.
ok - will followup.
(is the patch tracking system public?)

See http://patchwork.ozlabs.org/project/buildroot/list/.

baruch

Signature · 3 lines

     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Arnout Vandecappelle

2015-10-07 22:09 UTC #4 lore raw

PATCH v5 2/2 Patch to assembler to strip LOCK prefix.

In reply to Kinsella

Quoted 9 lines · Ray

Hi Arnout,
Quoted 3 lines · Arnout Vandecappelle
We now have a toolchain wrapper both for internal and external toolchains. This
will allow you to reliably pass options to the compiler. So we'd preferably use that approach.
Ok - I will investigate - any examples I can look at?

Quoted 7 lines

Quoted 4 lines
Also, we think it's worthwhile to add a new subarchitecture for the Quark in
arch/Config.in.x86. This should set the arch and tune options to i586, but the toolchain wrapper can then carry an option that adds -Wa,-momit-lock-prefix=yes. This also fixes the problem for external toolchains.
Ok - I actually proto-typed this and I couldn't get it to work reliably, hence the nasty patch.

Should be fairly simple with the toolchain wrapper. I think it it best to
refactor toolchain/toolchain-wrapper.mk a little to create a new variable to contain the BR_ADDITIONAL_CFLAGS, and then conditionally add the -Wa,-momit-... option to it.

Quoted 6 lines

I should have it on a branch (I hope), I can dust it off.
Quoted 3 lines
So the proposal is that you create a first patch that adds BR2_x86_quark and
adds -Wa,-momit-lock-prefix=yes, and then a second patch that adds the Galileo2 defconfig.

Quoted 8 lines

ok
Quoted 3 lines
I have now marked both patches as Changes Requested in out patch tracking
system, so please resubmit a new version that fixes the above comments, otherwise we'll forget about your contribution.
ok - will followup.
(is the patch tracking system public?)

http://patchwork.ozlabs.org/project/buildroot/list/

If you create a login with the same email address as the one you send patches
from, then you can update the state of your own patches (e.g. mark them as Superseded when you send a new version).

Regards,
Arnout

Signature · 6 lines

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF