gas: add flex mode to .bundle_lock
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_binutils_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-arm |
success
|
Test passed
|
Commit Message
Allow .bundle_lock flex to accept sequences larger than the bundle size.
Keep the existing alignment handling and strict checks for ordinary locks.
Add tests for straddling, non-straddling, oversized, and nested locks.
gas/
* read.c (bundle_flex_lock): New variable.
(assemble_one): Skip the bundle size check in flex mode.
(s_bundle_lock): Parse the optional flex argument.
(s_bundle_unlock): Skip the bundle size check in flex mode.
gas/testsuite/
* gas/i386/bundle-align-straddle.s: New test.
* gas/i386/bundle-align-straddle.d: New test.
* gas/i386/bundle-align-nostraddle.s: New test.
* gas/i386/bundle-align-nostraddle.d: New test.
* gas/i386/bundle-align-oversize.s: New test.
* gas/i386/bundle-align-oversize.d: New test.
* gas/i386/i386.exp: Run the new tests.
Signed-off-by: Sarvesh Chandra <Sarvesh.Chandra@amd.com>
---
[This is my first binutils patch, so I may not be following the expected format. I borrowed the format from GCC patches; please correct me if needed.]
Comments
Hi Sarvesh,
Thanks for the patch, and welcome. The format looks fine to me.
On 03/10/26 13:47, Sarvesh Chandra wrote:
> Allow .bundle_lock flex to accept sequences larger than the bundle size.
> Keep the existing alignment handling and strict checks for ordinary locks.
Could you say in the commit message what needs oversized locked sequences?
gas/doc/as.texi describes a .bundle_lock sequence as staying within the
same bundle, which flex changes, so the new argument needs documenting
there. A gas/NEWS entry would probably be good too.
>
> Add tests for straddling, non-straddling, oversized, and nested locks.
>
> gas/
> * read.c (bundle_flex_lock): New variable.
> (assemble_one): Skip the bundle size check in flex mode.
> (s_bundle_lock): Parse the optional flex argument.
> (s_bundle_unlock): Skip the bundle size check in flex mode.
>
> gas/testsuite/
> * gas/i386/bundle-align-straddle.s: New test.
> * gas/i386/bundle-align-straddle.d: New test.
> * gas/i386/bundle-align-nostraddle.s: New test.
> * gas/i386/bundle-align-nostraddle.d: New test.
> * gas/i386/bundle-align-oversize.s: New test.
> * gas/i386/bundle-align-oversize.d: New test.
> * gas/i386/i386.exp: Run the new tests.
>
> Signed-off-by: Sarvesh Chandra<Sarvesh.Chandra@amd.com>
>
> ---
> [This is my first binutils patch, so I may not be following the expected format. I borrowed the format from GCC patches; please correct me if needed.]
>
> diff --git a/gas/read.c b/gas/read.c
> index 99405ef68..619ab3bfe 100644
> --- a/gas/read.c
> +++ b/gas/read.c
> @@ -215,6 +215,8 @@ static int dwarf_file_string;
> it's the exponent of the bundle size, and aligned instruction bundle
> mode is in effect. */
> static unsigned int bundle_align_p2;
> +/* Whether the outermost lock permits an oversized sequence. */
> +static unsigned int bundle_flex_lock;
>
> /* These are set by .bundle_lock and .bundle_unlock. .bundle_lock sets
> bundle_lock_frag to frag_now and then starts a new frag with
> @@ -767,7 +769,7 @@ assemble_one (char *line)
> /* Make sure this hasn't pushed the locked sequence
> past the bundle size. */
> valueT bundle_size = pending_bundle_size (bundle_lock_frag);
> - if (bundle_size > (valueT) 1 << bundle_align_p2)
> + if (bundle_size > (valueT) 1 << bundle_align_p2 && !bundle_flex_lock)
> as_bad (_ (".bundle_lock sequence at %" PRIu64 " bytes, "
> "but .bundle_align_mode limit is %" PRIu64 " bytes"),
> (uint64_t) bundle_size, (uint64_t) 1 << bundle_align_p2);
> @@ -776,7 +778,7 @@ assemble_one (char *line)
> {
> valueT insn_size = pending_bundle_size (insn_start_frag);
>
> - if (insn_size > (valueT) 1 << bundle_align_p2)
> + if (insn_size > (valueT) 1 << bundle_align_p2 && !bundle_flex_lock)
> as_bad (_("single instruction is %" PRIu64 " bytes long, "
> "but .bundle_align_mode limit is %" PRIu64 " bytes"),
> (uint64_t) insn_size, (uint64_t) 1 << bundle_align_p2);
> @@ -6792,6 +6794,20 @@ s_bundle_align_mode (int arg ATTRIBUTE_UNUSED)
> void
> s_bundle_lock (int arg ATTRIBUTE_UNUSED)
> {
> + /* Parse the optional flex argument for the outermost lock. */
> + unsigned int flex = 0;
> + SKIP_WHITESPACE ();
> + if (!is_it_end_of_statement ())
> + {
> + char *name;
> + char c = get_symbol_name (&name);
> + if (strcmp (name, "flex") == 0)
> + flex = 1;
> + else
> + as_bad (_("unknown .bundle_lock argument `%s' (expected `flex')"),
> + name);
> + restore_line_pointer (c);
> + }
> demand_empty_rest_of_line ();
>
> if (bundle_align_p2 == 0)
> @@ -6802,6 +6818,7 @@ s_bundle_lock (int arg ATTRIBUTE_UNUSED)
>
> if (bundle_lock_depth == 0)
> {
> + bundle_flex_lock = flex;
A ".bundle_lock flex" on a nested lock is accepted but has no effect,
since flex is only recorded at depth 0. Maybe warn about it?
> bundle_lock_frchain = frchain_now;
> bundle_lock_frag = start_bundle ();
> }
> @@ -6829,7 +6846,7 @@ s_bundle_unlock (int arg ATTRIBUTE_UNUSED)
>
> size = pending_bundle_size (bundle_lock_frag);
>
> - if (size > (valueT) 1 << bundle_align_p2)
> + if (size > (valueT) 1 << bundle_align_p2 && !bundle_flex_lock)
> as_bad (_(".bundle_lock sequence is %" PRIu64 " bytes, "
bundle_flex_lock is never cleared when the outermost lock ends, so it
stays set after .bundle_unlock. Because the "single instruction is
... bytes long" check in assemble_one now also tests it, an oversized
instruction after a flex lock is no longer diagnosed, until the next
ordinary .bundle_lock resets the flag. I assume that isn't intended?
I think the fix is to clear bundle_flex_lock at the end of
s_bundle_unlock, next to bundle_lock_frag = NULL, and to leave the
"single instruction" check unchanged: it only runs outside a lock, so
the flag shouldn't affect it. A test that an oversized instruction
after a completed flex lock still errors would cover this.
Thanks,
Abhay
> "but bundle size is only %" PRIu64 " bytes"),
> (uint64_t) size, (uint64_t) 1 << bundle_align_p2);
> diff --git a/gas/testsuite/gas/i386/bundle-align-nostraddle.d b/gas/testsuite/gas/i386/bundle-align-nostraddle.d
> new file mode 100644
> index 000000000..63cefe383
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/bundle-align-nostraddle.d
> @@ -0,0 +1,11 @@
> +#objdump: -drw
> +#name: i386 .bundle_lock does not pad a non-straddling sequence
> +
> +.*: +file format .*i386.*
> +
> +Disassembly of section \.text:
> +
> +0+0000 <\.text>:
> + *0:\s+aa\s+.*
> + *1:\s+11 22\s+.*
> + *3:\s+bb\s+.*
> diff --git a/gas/testsuite/gas/i386/bundle-align-nostraddle.s b/gas/testsuite/gas/i386/bundle-align-nostraddle.s
> new file mode 100644
> index 000000000..6d46b169d
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/bundle-align-nostraddle.s
> @@ -0,0 +1,8 @@
> + .text
> + .bundle_align_mode 5
> + .p2align 5
> + .byte 0xaa
> + .bundle_lock
> + .byte 0x11, 0x22
> + .bundle_unlock
> + .byte 0xbb
> diff --git a/gas/testsuite/gas/i386/bundle-align-oversize.d b/gas/testsuite/gas/i386/bundle-align-oversize.d
> new file mode 100644
> index 000000000..d44a5d5a9
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/bundle-align-oversize.d
> @@ -0,0 +1,11 @@
> +#objdump: -drw
> +#name: i386 flex .bundle_lock does not pad an oversize sequence
> +
> +.*: +file format .*i386.*
> +
> +Disassembly of section \.text:
> +
> +0+0000 <\.text>:
> + *0:\s+aa\s+.*
> +#...
> + *29:\s+bb\s+.*
> diff --git a/gas/testsuite/gas/i386/bundle-align-oversize.s b/gas/testsuite/gas/i386/bundle-align-oversize.s
> new file mode 100644
> index 000000000..cc4aace02
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/bundle-align-oversize.s
> @@ -0,0 +1,11 @@
> + .text
> + .bundle_align_mode 5
> + .bundle_lock flex
> + .byte 0xaa
> + .bundle_lock
> + .rept 40
> + .byte 0x90
> + .endr
> + .bundle_unlock
> + .bundle_unlock
> + .byte 0xbb
> diff --git a/gas/testsuite/gas/i386/bundle-align-straddle.d b/gas/testsuite/gas/i386/bundle-align-straddle.d
> new file mode 100644
> index 000000000..e56c32668
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/bundle-align-straddle.d
> @@ -0,0 +1,13 @@
> +#objdump: -drw
> +#name: i386 .bundle_lock aligns a straddling sequence
> +
> +.*: +file format .*i386.*
> +
> +Disassembly of section \.text:
> +
> +0+0000 <\.text>:
> + *0:\s+aa\s+.*
> +#...
> + *1f:\s+90\s+.*
> + *20:\s+11 22\s+.*
> + *22:\s+bb\s+.*
> diff --git a/gas/testsuite/gas/i386/bundle-align-straddle.s b/gas/testsuite/gas/i386/bundle-align-straddle.s
> new file mode 100644
> index 000000000..503c6b7d8
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/bundle-align-straddle.s
> @@ -0,0 +1,9 @@
> + .text
> + .bundle_align_mode 5
> + .p2align 5
> + .byte 0xaa
> + .space 30, 0xcc
> + .bundle_lock
> + .byte 0x11, 0x22
> + .bundle_unlock
> + .byte 0xbb
> diff --git a/gas/testsuite/gas/i386/i386.exp b/gas/testsuite/gas/i386/i386.exp
> index d6ca2d609..b1fb31de7 100644
> --- a/gas/testsuite/gas/i386/i386.exp
> +++ b/gas/testsuite/gas/i386/i386.exp
> @@ -373,6 +373,9 @@ if [gas_32_check] then {
> run_dump_test "rdrnd-intel"
> run_dump_test "bundle"
> run_dump_test "bundle-lock"
> + run_dump_test "bundle-align-straddle"
> + run_dump_test "bundle-align-nostraddle"
> + run_dump_test "bundle-align-oversize"
> run_dump_test "bundle-bad"
> run_dump_test "adx"
> run_dump_test "adx-intel"
On 03.10.2026 10:17, Sarvesh Chandra wrote:
> Allow .bundle_lock flex to accept sequences larger than the bundle size.
> Keep the existing alignment handling and strict checks for ordinary locks.
>
> Add tests for straddling, non-straddling, oversized, and nested locks.
>
> gas/
> * read.c (bundle_flex_lock): New variable.
> (assemble_one): Skip the bundle size check in flex mode.
> (s_bundle_lock): Parse the optional flex argument.
> (s_bundle_unlock): Skip the bundle size check in flex mode.
>
> gas/testsuite/
> * gas/i386/bundle-align-straddle.s: New test.
> * gas/i386/bundle-align-straddle.d: New test.
> * gas/i386/bundle-align-nostraddle.s: New test.
> * gas/i386/bundle-align-nostraddle.d: New test.
> * gas/i386/bundle-align-oversize.s: New test.
> * gas/i386/bundle-align-oversize.d: New test.
> * gas/i386/i386.exp: Run the new tests.
>
> Signed-off-by: Sarvesh Chandra <Sarvesh.Chandra@amd.com>
>
> ---
> [This is my first binutils patch, so I may not be following the expected format. I borrowed the format from GCC patches; please correct me if needed.]
Looks pretty okay to me.
What's missing, also to understand the motivation (after all what you do
looks to go against what bundles are for), is an update to gas/doc/as.texi.
(Of course the motivation could also plausibly be part of the description.)
> @@ -767,7 +769,7 @@ assemble_one (char *line)
> /* Make sure this hasn't pushed the locked sequence
> past the bundle size. */
> valueT bundle_size = pending_bundle_size (bundle_lock_frag);
> - if (bundle_size > (valueT) 1 << bundle_align_p2)
> + if (bundle_size > (valueT) 1 << bundle_align_p2 && !bundle_flex_lock)
While it was an issue already before, with your addition it gets worse:
The shift expression (also again below) wants parenthesizing the latest
now.
> @@ -6792,6 +6794,20 @@ s_bundle_align_mode (int arg ATTRIBUTE_UNUSED)
> void
> s_bundle_lock (int arg ATTRIBUTE_UNUSED)
> {
> + /* Parse the optional flex argument for the outermost lock. */
> + unsigned int flex = 0;
Boolean variables want to be of bool type.
> + SKIP_WHITESPACE ();
> + if (!is_it_end_of_statement ())
> + {
> + char *name;
> + char c = get_symbol_name (&name);
> + if (strcmp (name, "flex") == 0)
> + flex = 1;
> + else
> + as_bad (_("unknown .bundle_lock argument `%s' (expected `flex')"),
> + name);
> + restore_line_pointer (c);
> + }
> demand_empty_rest_of_line ();
>
> if (bundle_align_p2 == 0)
> @@ -6802,6 +6818,7 @@ s_bundle_lock (int arg ATTRIBUTE_UNUSED)
>
> if (bundle_lock_depth == 0)
> {
> + bundle_flex_lock = flex;
Shouldn't use of the "flex" argument be diagnosed (perhaps as a warning)
when not used with the outermost .bundle_lock?
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/bundle-align-nostraddle.d
> @@ -0,0 +1,11 @@
> +#objdump: -drw
> +#name: i386 .bundle_lock does not pad a non-straddling sequence
> +
> +.*: +file format .*i386.*
> +
> +Disassembly of section \.text:
> +
> +0+0000 <\.text>:
> + *0:\s+aa\s+.*
> + *1:\s+11 22\s+.*
> + *3:\s+bb\s+.*
To help readability, please prefer [ ] over \s.
Further, while this happens to be exactly 4 bytes of .text contribution
right now, you still want to (uniformly, i.e. in all such testcases)
make sure to cover for possible padding which may be added automatically.
Generally (see e.g. bundle.d and bundle-lock.d) that's a "#pass" at the
end. (You will want to particularly run the testsuite for one of the
PE/COFF targets, where such automatic padding can be added.)
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/bundle-align-nostraddle.s
> @@ -0,0 +1,8 @@
> + .text
> + .bundle_align_mode 5
> + .p2align 5
> + .byte 0xaa
> + .bundle_lock
> + .byte 0x11, 0x22
> + .bundle_unlock
> + .byte 0xbb
This being an x86-specific testcase, why would you need to resort to
.byte? Just use instructions that encode with a suitable number of
bytes. Or use .nop / .nops. That'll also help readability of the
corresponding .d file then.
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/bundle-align-oversize.s
> @@ -0,0 +1,11 @@
> + .text
> + .bundle_align_mode 5
> + .bundle_lock flex
> + .byte 0xaa
> + .bundle_lock
> + .rept 40
> + .byte 0x90
> + .endr
Shorter as ".nops 40" perhaps?
Jan
@@ -215,6 +215,8 @@ static int dwarf_file_string;
it's the exponent of the bundle size, and aligned instruction bundle
mode is in effect. */
static unsigned int bundle_align_p2;
+/* Whether the outermost lock permits an oversized sequence. */
+static unsigned int bundle_flex_lock;
/* These are set by .bundle_lock and .bundle_unlock. .bundle_lock sets
bundle_lock_frag to frag_now and then starts a new frag with
@@ -767,7 +769,7 @@ assemble_one (char *line)
/* Make sure this hasn't pushed the locked sequence
past the bundle size. */
valueT bundle_size = pending_bundle_size (bundle_lock_frag);
- if (bundle_size > (valueT) 1 << bundle_align_p2)
+ if (bundle_size > (valueT) 1 << bundle_align_p2 && !bundle_flex_lock)
as_bad (_ (".bundle_lock sequence at %" PRIu64 " bytes, "
"but .bundle_align_mode limit is %" PRIu64 " bytes"),
(uint64_t) bundle_size, (uint64_t) 1 << bundle_align_p2);
@@ -776,7 +778,7 @@ assemble_one (char *line)
{
valueT insn_size = pending_bundle_size (insn_start_frag);
- if (insn_size > (valueT) 1 << bundle_align_p2)
+ if (insn_size > (valueT) 1 << bundle_align_p2 && !bundle_flex_lock)
as_bad (_("single instruction is %" PRIu64 " bytes long, "
"but .bundle_align_mode limit is %" PRIu64 " bytes"),
(uint64_t) insn_size, (uint64_t) 1 << bundle_align_p2);
@@ -6792,6 +6794,20 @@ s_bundle_align_mode (int arg ATTRIBUTE_UNUSED)
void
s_bundle_lock (int arg ATTRIBUTE_UNUSED)
{
+ /* Parse the optional flex argument for the outermost lock. */
+ unsigned int flex = 0;
+ SKIP_WHITESPACE ();
+ if (!is_it_end_of_statement ())
+ {
+ char *name;
+ char c = get_symbol_name (&name);
+ if (strcmp (name, "flex") == 0)
+ flex = 1;
+ else
+ as_bad (_("unknown .bundle_lock argument `%s' (expected `flex')"),
+ name);
+ restore_line_pointer (c);
+ }
demand_empty_rest_of_line ();
if (bundle_align_p2 == 0)
@@ -6802,6 +6818,7 @@ s_bundle_lock (int arg ATTRIBUTE_UNUSED)
if (bundle_lock_depth == 0)
{
+ bundle_flex_lock = flex;
bundle_lock_frchain = frchain_now;
bundle_lock_frag = start_bundle ();
}
@@ -6829,7 +6846,7 @@ s_bundle_unlock (int arg ATTRIBUTE_UNUSED)
size = pending_bundle_size (bundle_lock_frag);
- if (size > (valueT) 1 << bundle_align_p2)
+ if (size > (valueT) 1 << bundle_align_p2 && !bundle_flex_lock)
as_bad (_(".bundle_lock sequence is %" PRIu64 " bytes, "
"but bundle size is only %" PRIu64 " bytes"),
(uint64_t) size, (uint64_t) 1 << bundle_align_p2);
new file mode 100644
@@ -0,0 +1,11 @@
+#objdump: -drw
+#name: i386 .bundle_lock does not pad a non-straddling sequence
+
+.*: +file format .*i386.*
+
+Disassembly of section \.text:
+
+0+0000 <\.text>:
+ *0:\s+aa\s+.*
+ *1:\s+11 22\s+.*
+ *3:\s+bb\s+.*
new file mode 100644
@@ -0,0 +1,8 @@
+ .text
+ .bundle_align_mode 5
+ .p2align 5
+ .byte 0xaa
+ .bundle_lock
+ .byte 0x11, 0x22
+ .bundle_unlock
+ .byte 0xbb
new file mode 100644
@@ -0,0 +1,11 @@
+#objdump: -drw
+#name: i386 flex .bundle_lock does not pad an oversize sequence
+
+.*: +file format .*i386.*
+
+Disassembly of section \.text:
+
+0+0000 <\.text>:
+ *0:\s+aa\s+.*
+#...
+ *29:\s+bb\s+.*
new file mode 100644
@@ -0,0 +1,11 @@
+ .text
+ .bundle_align_mode 5
+ .bundle_lock flex
+ .byte 0xaa
+ .bundle_lock
+ .rept 40
+ .byte 0x90
+ .endr
+ .bundle_unlock
+ .bundle_unlock
+ .byte 0xbb
new file mode 100644
@@ -0,0 +1,13 @@
+#objdump: -drw
+#name: i386 .bundle_lock aligns a straddling sequence
+
+.*: +file format .*i386.*
+
+Disassembly of section \.text:
+
+0+0000 <\.text>:
+ *0:\s+aa\s+.*
+#...
+ *1f:\s+90\s+.*
+ *20:\s+11 22\s+.*
+ *22:\s+bb\s+.*
new file mode 100644
@@ -0,0 +1,9 @@
+ .text
+ .bundle_align_mode 5
+ .p2align 5
+ .byte 0xaa
+ .space 30, 0xcc
+ .bundle_lock
+ .byte 0x11, 0x22
+ .bundle_unlock
+ .byte 0xbb
@@ -373,6 +373,9 @@ if [gas_32_check] then {
run_dump_test "rdrnd-intel"
run_dump_test "bundle"
run_dump_test "bundle-lock"
+ run_dump_test "bundle-align-straddle"
+ run_dump_test "bundle-align-nostraddle"
+ run_dump_test "bundle-align-oversize"
run_dump_test "bundle-bad"
run_dump_test "adx"
run_dump_test "adx-intel"