x86: Pass "%s" to i386_dis_printf as the format string
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
On Fri, May 15, 2026 at 9:08 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> Model this after operand handling, such that comments can be emitted in
> the same order as operands. %rip-relative address comments remain
> separate for now. While there correct style for the symbols associated
> with immediates: These aren't "comment starts", but symbol names.
> ---
> As long as we want to continue to use the ->print_address_func() hook,
> properly unifying %rip-relative comments with others won't be possible.
> That, however, is in line with direct addresses also getting printed
> specially, similarly by using ->print_address_func().
>
> Was it really intended for the original -Mannotate test to be run only for
> Linux targets? It certainly isn't intended here, yet I'd like the new
> tests to be in the same directory as the original one. x86-64.exp,
> however, bails right away for non-Linux (and remote hosts). I guess at
> least the *.d based testing loop should be moved up. Tests truly only
> working for Linux can easily be constrained in the *.d files themselves.
>
> --- /dev/null
> +++ b/binutils/testsuite/binutils-all/x86-64/comments.d
> @@ -0,0 +1,14 @@
> +#name: disassembly comments (AT&T)
> +#ld:
> +#objdump: -dwMannotate,att
> +
> +.*: +file format .*
> +
> +
> +Disassembly of section .text:
> +
> +[0-9a-f]+ <_start>:
> +[ ]*[0-9a-f]+: 48 c7 05 [0-9a-f ]+ movq \$0x[0-9a-f]+,0x[0-9a-f]+\(%rip\) +# \[_start\], [0-9a-f]+ <fptr>
> +[ ]*[0-9a-f]+: 31 c0 xor %eax,%eax
> +[ ]*[0-9a-f]+: c3 ret
> +#pass
> --- /dev/null
> +++ b/binutils/testsuite/binutils-all/x86-64/comments.s
> @@ -0,0 +1,9 @@
> + .text
> + .global _start
> +_start:
> + movq $_start, fptr(%rip)
> + xor %eax, %eax
> + ret
> +
> + .data
> +fptr: .quad -1
> --- /dev/null
> +++ b/binutils/testsuite/binutils-all/x86-64/comments-intel.d
> @@ -0,0 +1,14 @@
> +#name: disassembly comments (Intel)
> +#source: comments.s
> +#ld:
> +#objdump: -dwMannotate,intel
> +
> +.*: +file format .*
> +
> +
> +Disassembly of section .text:
> +
> +[0-9a-f]+ <_start>:
> +[ ]*[0-9a-f]+: 48 c7 05 [0-9a-f ]+ mov QWORD PTR \[rip\+0x[0-9a-f]+\],0x[0-9a-f]+ +# [0-9a-f]+ <fptr>, \[_start\]
> +[ ]*[0-9a-f]+: 31 c0 xor eax,eax
> +[ ]*[0-9a-f]+: c3 ret
> --- a/opcodes/i386-dis.c
> +++ b/opcodes/i386-dis.c
> @@ -179,8 +179,7 @@ struct instr_info
>
> char obuf[MAX_OPERAND_BUFFER_SIZE];
> char *obufp;
> - char cbuf[COMMENT_BUFFER_SIZE];
> - char * cbufp;
> + char *cbufp;
> char *mnemonicendp;
> const uint8_t *start_codep;
> uint8_t *codep;
> @@ -258,6 +257,7 @@ struct instr_info
> signed char op_index[MAX_OPERANDS];
> bool op_riprel[MAX_OPERANDS];
> char *op_out[MAX_OPERANDS];
> + char *cm_out[MAX_OPERANDS];
> bfd_vma op_address[MAX_OPERANDS];
> bfd_vma start_pc;
>
> @@ -9749,7 +9749,6 @@ print_insn (bfd_vma pc, disassemble_info
> .start_codep = priv.the_buffer,
> .codep = priv.the_buffer,
> .obufp = ins.obuf,
> - .cbufp = ins.cbuf,
> .last_lock_prefix = -1,
> .last_repz_prefix = -1,
> .last_repnz_prefix = -1,
> @@ -9761,6 +9760,7 @@ print_insn (bfd_vma pc, disassemble_info
> .fwait_prefix = -1,
> };
> char op_out[MAX_OPERANDS][MAX_OPERAND_BUFFER_SIZE];
> + char cm_out[MAX_OPERANDS][COMMENT_BUFFER_SIZE];
>
> priv.orig_sizeflag = AFLAG | DFLAG;
> if ((info->mach & bfd_mach_i386_i386) != 0)
> @@ -9874,6 +9874,8 @@ print_insn (bfd_vma pc, disassemble_info
> {
> op_out[i][0] = 0;
> ins.op_out[i] = op_out[i];
> + cm_out[i][0] = 0;
> + ins.cm_out[i] = cm_out[i];
> }
>
> sizeflag = priv.orig_sizeflag;
> @@ -9992,6 +9994,7 @@ print_insn (bfd_vma pc, disassemble_info
> for (i = 0; i < MAX_OPERANDS; ++i)
> {
> ins.obufp = ins.op_out[i];
> + ins.cbufp = ins.cm_out[i];
> ins.op_ad = MAX_OPERANDS - 1 - i;
> if (dp->op[i].rtn
> && !dp->op[i].rtn (&ins, dp->op[i].bytemode, sizeflag))
> @@ -10317,6 +10320,10 @@ print_insn (bfd_vma pc, disassemble_info
> riprel = ins.op_riprel[i];
> ins.op_riprel[i] = ins.op_riprel[MAX_OPERANDS - 1 - i];
> ins.op_riprel[MAX_OPERANDS - 1 - i] = riprel;
> +
> + char *tmp = ins.cm_out[i];
> + ins.cm_out[i] = ins.cm_out[MAX_OPERANDS - 1 - i];
> + ins.cm_out[MAX_OPERANDS - 1 - i] = tmp;
> }
> }
> else
> @@ -10364,22 +10371,23 @@ print_insn (bfd_vma pc, disassemble_info
> needcomma = 1;
> }
>
> + const char *sep = " # ";
> for (i = 0; i < MAX_OPERANDS; i++)
> if (ins.op_index[i] != -1 && ins.op_riprel[i])
> {
> - i386_dis_printf (info, dis_style_comment_start, " # ");
> + i386_dis_printf (info, dis_style_comment_start, sep);
> + sep = ", ";
> (*info->print_address_func)
> ((bfd_vma)(ins.start_pc + (ins.codep - ins.start_codep)
> + ins.op_address[ins.op_index[i]]),
> - info);
> - break;
> + info);
> + }
> + else if (*ins.cm_out[i])
> + {
> + i386_dis_printf (info, dis_style_comment_start, sep);
> + sep = ", ";
> + i386_dis_printf (info, dis_style_symbol, "%s", ins.cm_out[i]);
> }
> - if (ins.cbufp != ins.cbuf)
> - {
> - if (i == MAX_OPERANDS)
> - i386_dis_printf (info, dis_style_comment_start, " # ");
> - i386_dis_printf (info, dis_style_comment_start, "%s", ins.cbuf);
> - }
>
> ret = ins.codep - priv.the_buffer;
> out:
> @@ -10760,6 +10768,7 @@ dofloat (instr_info *ins, int sizeflag)
commit aeced13ee0cd570d78cb37ab1f9bba840958515a
Author: Jan Beulich <jbeulich@suse.com>
Date: Fri May 22 08:49:12 2026 +0200
x86/disasm: rework comment handling
caused:
libtool: compile: gcc -DHAVE_CONFIG_H -I. -I../../opcodes -I.
-I../../opcodes -I../bfd -I../../opcodes/../include
-I../../opcodes/../bfd -W -Wall -Wstrict-prototypes
-Wmissing-prototypes -Wshadow -Wstack-usage=262144 -O2 -flto=auto
-ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall
-Wno-complain-wrong-lang -Werror=format-security
-Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS
-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
-fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1
-m32 -march=i686 -mtune=generic -msse2 -mfpmath=sse -mstackrealign
-fasynchronous-unwind-tables -fstack-clash-protection
-mtls-dialect=gnu -flto=8 -std=gnu11 -fprofile-generate
-flto=jobserver -ffat-lto-objects -MT i386-dis.lo -MD -MP -MF
.deps/i386-dis.Tpo -c ../../opcodes/i386-dis.c -fPIC -DPIC -o
.libs/i386-dis.o
../../opcodes/i386-dis.c: In function ‘print_insn’:
../../opcodes/i386-dis.c:10378:9: error: format not a string literal
and no format arguments [-Werror=format-security]
10378 | i386_dis_printf (info, dis_style_comment_start, sep);
| ^~~~~~~~~~~~~~~
../../opcodes/i386-dis.c:10387:9: error: format not a string literal
and no format arguments [-Werror=format-security]
10387 | i386_dis_printf (info, dis_style_comment_start, sep);
| ^~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
make[4]: *** [Makefile:1076: i386-dis.lo] Error 1
Since i386_dis_printf is declared as
static void ATTRIBUTE_PRINTF_3 i386_dis_printf (const disassemble_info *,
enum disassembler_style,
const char *, ...);
the 3rd argument should be a string literal as the format string. Pass
"%s" to i386_dis_printf as the format string to fix the regression.
PR binutils/34168
* i386-dis.c (print_insn): Pass "%s" to i386_dis_printf as the
format string.
Comments
On 24.05.2026 02:09, H.J. Lu wrote:
> On Fri, May 15, 2026 at 9:08 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> Model this after operand handling, such that comments can be emitted in
>> the same order as operands. %rip-relative address comments remain
>> separate for now. While there correct style for the symbols associated
>> with immediates: These aren't "comment starts", but symbol names.
>> ---
>> As long as we want to continue to use the ->print_address_func() hook,
>> properly unifying %rip-relative comments with others won't be possible.
>> That, however, is in line with direct addresses also getting printed
>> specially, similarly by using ->print_address_func().
>>
>> Was it really intended for the original -Mannotate test to be run only for
>> Linux targets? It certainly isn't intended here, yet I'd like the new
>> tests to be in the same directory as the original one. x86-64.exp,
>> however, bails right away for non-Linux (and remote hosts). I guess at
>> least the *.d based testing loop should be moved up. Tests truly only
>> working for Linux can easily be constrained in the *.d files themselves.
>>
>> --- /dev/null
>> +++ b/binutils/testsuite/binutils-all/x86-64/comments.d
>> @@ -0,0 +1,14 @@
>> +#name: disassembly comments (AT&T)
>> +#ld:
>> +#objdump: -dwMannotate,att
>> +
>> +.*: +file format .*
>> +
>> +
>> +Disassembly of section .text:
>> +
>> +[0-9a-f]+ <_start>:
>> +[ ]*[0-9a-f]+: 48 c7 05 [0-9a-f ]+ movq \$0x[0-9a-f]+,0x[0-9a-f]+\(%rip\) +# \[_start\], [0-9a-f]+ <fptr>
>> +[ ]*[0-9a-f]+: 31 c0 xor %eax,%eax
>> +[ ]*[0-9a-f]+: c3 ret
>> +#pass
>> --- /dev/null
>> +++ b/binutils/testsuite/binutils-all/x86-64/comments.s
>> @@ -0,0 +1,9 @@
>> + .text
>> + .global _start
>> +_start:
>> + movq $_start, fptr(%rip)
>> + xor %eax, %eax
>> + ret
>> +
>> + .data
>> +fptr: .quad -1
>> --- /dev/null
>> +++ b/binutils/testsuite/binutils-all/x86-64/comments-intel.d
>> @@ -0,0 +1,14 @@
>> +#name: disassembly comments (Intel)
>> +#source: comments.s
>> +#ld:
>> +#objdump: -dwMannotate,intel
>> +
>> +.*: +file format .*
>> +
>> +
>> +Disassembly of section .text:
>> +
>> +[0-9a-f]+ <_start>:
>> +[ ]*[0-9a-f]+: 48 c7 05 [0-9a-f ]+ mov QWORD PTR \[rip\+0x[0-9a-f]+\],0x[0-9a-f]+ +# [0-9a-f]+ <fptr>, \[_start\]
>> +[ ]*[0-9a-f]+: 31 c0 xor eax,eax
>> +[ ]*[0-9a-f]+: c3 ret
>> --- a/opcodes/i386-dis.c
>> +++ b/opcodes/i386-dis.c
>> @@ -179,8 +179,7 @@ struct instr_info
>>
>> char obuf[MAX_OPERAND_BUFFER_SIZE];
>> char *obufp;
>> - char cbuf[COMMENT_BUFFER_SIZE];
>> - char * cbufp;
>> + char *cbufp;
>> char *mnemonicendp;
>> const uint8_t *start_codep;
>> uint8_t *codep;
>> @@ -258,6 +257,7 @@ struct instr_info
>> signed char op_index[MAX_OPERANDS];
>> bool op_riprel[MAX_OPERANDS];
>> char *op_out[MAX_OPERANDS];
>> + char *cm_out[MAX_OPERANDS];
>> bfd_vma op_address[MAX_OPERANDS];
>> bfd_vma start_pc;
>>
>> @@ -9749,7 +9749,6 @@ print_insn (bfd_vma pc, disassemble_info
>> .start_codep = priv.the_buffer,
>> .codep = priv.the_buffer,
>> .obufp = ins.obuf,
>> - .cbufp = ins.cbuf,
>> .last_lock_prefix = -1,
>> .last_repz_prefix = -1,
>> .last_repnz_prefix = -1,
>> @@ -9761,6 +9760,7 @@ print_insn (bfd_vma pc, disassemble_info
>> .fwait_prefix = -1,
>> };
>> char op_out[MAX_OPERANDS][MAX_OPERAND_BUFFER_SIZE];
>> + char cm_out[MAX_OPERANDS][COMMENT_BUFFER_SIZE];
>>
>> priv.orig_sizeflag = AFLAG | DFLAG;
>> if ((info->mach & bfd_mach_i386_i386) != 0)
>> @@ -9874,6 +9874,8 @@ print_insn (bfd_vma pc, disassemble_info
>> {
>> op_out[i][0] = 0;
>> ins.op_out[i] = op_out[i];
>> + cm_out[i][0] = 0;
>> + ins.cm_out[i] = cm_out[i];
>> }
>>
>> sizeflag = priv.orig_sizeflag;
>> @@ -9992,6 +9994,7 @@ print_insn (bfd_vma pc, disassemble_info
>> for (i = 0; i < MAX_OPERANDS; ++i)
>> {
>> ins.obufp = ins.op_out[i];
>> + ins.cbufp = ins.cm_out[i];
>> ins.op_ad = MAX_OPERANDS - 1 - i;
>> if (dp->op[i].rtn
>> && !dp->op[i].rtn (&ins, dp->op[i].bytemode, sizeflag))
>> @@ -10317,6 +10320,10 @@ print_insn (bfd_vma pc, disassemble_info
>> riprel = ins.op_riprel[i];
>> ins.op_riprel[i] = ins.op_riprel[MAX_OPERANDS - 1 - i];
>> ins.op_riprel[MAX_OPERANDS - 1 - i] = riprel;
>> +
>> + char *tmp = ins.cm_out[i];
>> + ins.cm_out[i] = ins.cm_out[MAX_OPERANDS - 1 - i];
>> + ins.cm_out[MAX_OPERANDS - 1 - i] = tmp;
>> }
>> }
>> else
>> @@ -10364,22 +10371,23 @@ print_insn (bfd_vma pc, disassemble_info
>> needcomma = 1;
>> }
>>
>> + const char *sep = " # ";
>> for (i = 0; i < MAX_OPERANDS; i++)
>> if (ins.op_index[i] != -1 && ins.op_riprel[i])
>> {
>> - i386_dis_printf (info, dis_style_comment_start, " # ");
>> + i386_dis_printf (info, dis_style_comment_start, sep);
>> + sep = ", ";
>> (*info->print_address_func)
>> ((bfd_vma)(ins.start_pc + (ins.codep - ins.start_codep)
>> + ins.op_address[ins.op_index[i]]),
>> - info);
>> - break;
>> + info);
>> + }
>> + else if (*ins.cm_out[i])
>> + {
>> + i386_dis_printf (info, dis_style_comment_start, sep);
>> + sep = ", ";
>> + i386_dis_printf (info, dis_style_symbol, "%s", ins.cm_out[i]);
>> }
>> - if (ins.cbufp != ins.cbuf)
>> - {
>> - if (i == MAX_OPERANDS)
>> - i386_dis_printf (info, dis_style_comment_start, " # ");
>> - i386_dis_printf (info, dis_style_comment_start, "%s", ins.cbuf);
>> - }
>>
>> ret = ins.codep - priv.the_buffer;
>> out:
>> @@ -10760,6 +10768,7 @@ dofloat (instr_info *ins, int sizeflag)
>
> commit aeced13ee0cd570d78cb37ab1f9bba840958515a
> Author: Jan Beulich <jbeulich@suse.com>
> Date: Fri May 22 08:49:12 2026 +0200
>
> x86/disasm: rework comment handling
>
> caused:
>
> libtool: compile: gcc -DHAVE_CONFIG_H -I. -I../../opcodes -I.
> -I../../opcodes -I../bfd -I../../opcodes/../include
> -I../../opcodes/../bfd -W -Wall -Wstrict-prototypes
> -Wmissing-prototypes -Wshadow -Wstack-usage=262144 -O2 -flto=auto
> -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall
> -Wno-complain-wrong-lang -Werror=format-security
> -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS
> -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
> -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1
> -m32 -march=i686 -mtune=generic -msse2 -mfpmath=sse -mstackrealign
> -fasynchronous-unwind-tables -fstack-clash-protection
> -mtls-dialect=gnu -flto=8 -std=gnu11 -fprofile-generate
> -flto=jobserver -ffat-lto-objects -MT i386-dis.lo -MD -MP -MF
> .deps/i386-dis.Tpo -c ../../opcodes/i386-dis.c -fPIC -DPIC -o
> .libs/i386-dis.o
> ../../opcodes/i386-dis.c: In function ‘print_insn’:
> ../../opcodes/i386-dis.c:10378:9: error: format not a string literal
> and no format arguments [-Werror=format-security]
> 10378 | i386_dis_printf (info, dis_style_comment_start, sep);
> | ^~~~~~~~~~~~~~~
> ../../opcodes/i386-dis.c:10387:9: error: format not a string literal
> and no format arguments [-Werror=format-security]
> 10387 | i386_dis_printf (info, dis_style_comment_start, sep);
> | ^~~~~~~~~~~~~~~
> cc1: some warnings being treated as errors
> make[4]: *** [Makefile:1076: i386-dis.lo] Error 1
>
> Since i386_dis_printf is declared as
>
> static void ATTRIBUTE_PRINTF_3 i386_dis_printf (const disassemble_info *,
> enum disassembler_style,
> const char *, ...);
>
> the 3rd argument should be a string literal as the format string. Pass
> "%s" to i386_dis_printf as the format string to fix the regression.
What exactly does "should" here mean? I.e. what exactly is wrong with the
format string not being a literal? Unless that can be explained, it feels as
if first of all a gcc bug wants opening. That would then want referencing
here.
Jan
On Sun, May 24, 2026 at 11:11 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 24.05.2026 02:09, H.J. Lu wrote:
> > On Fri, May 15, 2026 at 9:08 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> Model this after operand handling, such that comments can be emitted in
> >> the same order as operands. %rip-relative address comments remain
> >> separate for now. While there correct style for the symbols associated
> >> with immediates: These aren't "comment starts", but symbol names.
> >> ---
> >> As long as we want to continue to use the ->print_address_func() hook,
> >> properly unifying %rip-relative comments with others won't be possible.
> >> That, however, is in line with direct addresses also getting printed
> >> specially, similarly by using ->print_address_func().
> >>
> >> Was it really intended for the original -Mannotate test to be run only for
> >> Linux targets? It certainly isn't intended here, yet I'd like the new
> >> tests to be in the same directory as the original one. x86-64.exp,
> >> however, bails right away for non-Linux (and remote hosts). I guess at
> >> least the *.d based testing loop should be moved up. Tests truly only
> >> working for Linux can easily be constrained in the *.d files themselves.
> >>
> >> --- /dev/null
> >> +++ b/binutils/testsuite/binutils-all/x86-64/comments.d
> >> @@ -0,0 +1,14 @@
> >> +#name: disassembly comments (AT&T)
> >> +#ld:
> >> +#objdump: -dwMannotate,att
> >> +
> >> +.*: +file format .*
> >> +
> >> +
> >> +Disassembly of section .text:
> >> +
> >> +[0-9a-f]+ <_start>:
> >> +[ ]*[0-9a-f]+: 48 c7 05 [0-9a-f ]+ movq \$0x[0-9a-f]+,0x[0-9a-f]+\(%rip\) +# \[_start\], [0-9a-f]+ <fptr>
> >> +[ ]*[0-9a-f]+: 31 c0 xor %eax,%eax
> >> +[ ]*[0-9a-f]+: c3 ret
> >> +#pass
> >> --- /dev/null
> >> +++ b/binutils/testsuite/binutils-all/x86-64/comments.s
> >> @@ -0,0 +1,9 @@
> >> + .text
> >> + .global _start
> >> +_start:
> >> + movq $_start, fptr(%rip)
> >> + xor %eax, %eax
> >> + ret
> >> +
> >> + .data
> >> +fptr: .quad -1
> >> --- /dev/null
> >> +++ b/binutils/testsuite/binutils-all/x86-64/comments-intel.d
> >> @@ -0,0 +1,14 @@
> >> +#name: disassembly comments (Intel)
> >> +#source: comments.s
> >> +#ld:
> >> +#objdump: -dwMannotate,intel
> >> +
> >> +.*: +file format .*
> >> +
> >> +
> >> +Disassembly of section .text:
> >> +
> >> +[0-9a-f]+ <_start>:
> >> +[ ]*[0-9a-f]+: 48 c7 05 [0-9a-f ]+ mov QWORD PTR \[rip\+0x[0-9a-f]+\],0x[0-9a-f]+ +# [0-9a-f]+ <fptr>, \[_start\]
> >> +[ ]*[0-9a-f]+: 31 c0 xor eax,eax
> >> +[ ]*[0-9a-f]+: c3 ret
> >> --- a/opcodes/i386-dis.c
> >> +++ b/opcodes/i386-dis.c
> >> @@ -179,8 +179,7 @@ struct instr_info
> >>
> >> char obuf[MAX_OPERAND_BUFFER_SIZE];
> >> char *obufp;
> >> - char cbuf[COMMENT_BUFFER_SIZE];
> >> - char * cbufp;
> >> + char *cbufp;
> >> char *mnemonicendp;
> >> const uint8_t *start_codep;
> >> uint8_t *codep;
> >> @@ -258,6 +257,7 @@ struct instr_info
> >> signed char op_index[MAX_OPERANDS];
> >> bool op_riprel[MAX_OPERANDS];
> >> char *op_out[MAX_OPERANDS];
> >> + char *cm_out[MAX_OPERANDS];
> >> bfd_vma op_address[MAX_OPERANDS];
> >> bfd_vma start_pc;
> >>
> >> @@ -9749,7 +9749,6 @@ print_insn (bfd_vma pc, disassemble_info
> >> .start_codep = priv.the_buffer,
> >> .codep = priv.the_buffer,
> >> .obufp = ins.obuf,
> >> - .cbufp = ins.cbuf,
> >> .last_lock_prefix = -1,
> >> .last_repz_prefix = -1,
> >> .last_repnz_prefix = -1,
> >> @@ -9761,6 +9760,7 @@ print_insn (bfd_vma pc, disassemble_info
> >> .fwait_prefix = -1,
> >> };
> >> char op_out[MAX_OPERANDS][MAX_OPERAND_BUFFER_SIZE];
> >> + char cm_out[MAX_OPERANDS][COMMENT_BUFFER_SIZE];
> >>
> >> priv.orig_sizeflag = AFLAG | DFLAG;
> >> if ((info->mach & bfd_mach_i386_i386) != 0)
> >> @@ -9874,6 +9874,8 @@ print_insn (bfd_vma pc, disassemble_info
> >> {
> >> op_out[i][0] = 0;
> >> ins.op_out[i] = op_out[i];
> >> + cm_out[i][0] = 0;
> >> + ins.cm_out[i] = cm_out[i];
> >> }
> >>
> >> sizeflag = priv.orig_sizeflag;
> >> @@ -9992,6 +9994,7 @@ print_insn (bfd_vma pc, disassemble_info
> >> for (i = 0; i < MAX_OPERANDS; ++i)
> >> {
> >> ins.obufp = ins.op_out[i];
> >> + ins.cbufp = ins.cm_out[i];
> >> ins.op_ad = MAX_OPERANDS - 1 - i;
> >> if (dp->op[i].rtn
> >> && !dp->op[i].rtn (&ins, dp->op[i].bytemode, sizeflag))
> >> @@ -10317,6 +10320,10 @@ print_insn (bfd_vma pc, disassemble_info
> >> riprel = ins.op_riprel[i];
> >> ins.op_riprel[i] = ins.op_riprel[MAX_OPERANDS - 1 - i];
> >> ins.op_riprel[MAX_OPERANDS - 1 - i] = riprel;
> >> +
> >> + char *tmp = ins.cm_out[i];
> >> + ins.cm_out[i] = ins.cm_out[MAX_OPERANDS - 1 - i];
> >> + ins.cm_out[MAX_OPERANDS - 1 - i] = tmp;
> >> }
> >> }
> >> else
> >> @@ -10364,22 +10371,23 @@ print_insn (bfd_vma pc, disassemble_info
> >> needcomma = 1;
> >> }
> >>
> >> + const char *sep = " # ";
> >> for (i = 0; i < MAX_OPERANDS; i++)
> >> if (ins.op_index[i] != -1 && ins.op_riprel[i])
> >> {
> >> - i386_dis_printf (info, dis_style_comment_start, " # ");
> >> + i386_dis_printf (info, dis_style_comment_start, sep);
> >> + sep = ", ";
> >> (*info->print_address_func)
> >> ((bfd_vma)(ins.start_pc + (ins.codep - ins.start_codep)
> >> + ins.op_address[ins.op_index[i]]),
> >> - info);
> >> - break;
> >> + info);
> >> + }
> >> + else if (*ins.cm_out[i])
> >> + {
> >> + i386_dis_printf (info, dis_style_comment_start, sep);
> >> + sep = ", ";
> >> + i386_dis_printf (info, dis_style_symbol, "%s", ins.cm_out[i]);
> >> }
> >> - if (ins.cbufp != ins.cbuf)
> >> - {
> >> - if (i == MAX_OPERANDS)
> >> - i386_dis_printf (info, dis_style_comment_start, " # ");
> >> - i386_dis_printf (info, dis_style_comment_start, "%s", ins.cbuf);
> >> - }
> >>
> >> ret = ins.codep - priv.the_buffer;
> >> out:
> >> @@ -10760,6 +10768,7 @@ dofloat (instr_info *ins, int sizeflag)
> >
> > commit aeced13ee0cd570d78cb37ab1f9bba840958515a
> > Author: Jan Beulich <jbeulich@suse.com>
> > Date: Fri May 22 08:49:12 2026 +0200
> >
> > x86/disasm: rework comment handling
> >
> > caused:
> >
> > libtool: compile: gcc -DHAVE_CONFIG_H -I. -I../../opcodes -I.
> > -I../../opcodes -I../bfd -I../../opcodes/../include
> > -I../../opcodes/../bfd -W -Wall -Wstrict-prototypes
> > -Wmissing-prototypes -Wshadow -Wstack-usage=262144 -O2 -flto=auto
> > -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall
> > -Wno-complain-wrong-lang -Werror=format-security
> > -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS
> > -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
> > -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1
> > -m32 -march=i686 -mtune=generic -msse2 -mfpmath=sse -mstackrealign
> > -fasynchronous-unwind-tables -fstack-clash-protection
> > -mtls-dialect=gnu -flto=8 -std=gnu11 -fprofile-generate
> > -flto=jobserver -ffat-lto-objects -MT i386-dis.lo -MD -MP -MF
> > .deps/i386-dis.Tpo -c ../../opcodes/i386-dis.c -fPIC -DPIC -o
> > .libs/i386-dis.o
> > ../../opcodes/i386-dis.c: In function ‘print_insn’:
> > ../../opcodes/i386-dis.c:10378:9: error: format not a string literal
> > and no format arguments [-Werror=format-security]
> > 10378 | i386_dis_printf (info, dis_style_comment_start, sep);
> > | ^~~~~~~~~~~~~~~
> > ../../opcodes/i386-dis.c:10387:9: error: format not a string literal
> > and no format arguments [-Werror=format-security]
> > 10387 | i386_dis_printf (info, dis_style_comment_start, sep);
> > | ^~~~~~~~~~~~~~~
> > cc1: some warnings being treated as errors
> > make[4]: *** [Makefile:1076: i386-dis.lo] Error 1
> >
> > Since i386_dis_printf is declared as
> >
> > static void ATTRIBUTE_PRINTF_3 i386_dis_printf (const disassemble_info *,
> > enum disassembler_style,
> > const char *, ...);
> >
> > the 3rd argument should be a string literal as the format string. Pass
> > "%s" to i386_dis_printf as the format string to fix the regression.
>
> What exactly does "should" here mean? I.e. what exactly is wrong with the
> format string not being a literal? Unless that can be explained, it feels as
> if first of all a gcc bug wants opening. That would then want referencing
> here.
>
> Jan
Binutils may be compiled with -Werror=format-security:
‘-Wformat-security’
If ‘-Wformat’ is specified, also warn about uses of format
functions that represent possible security problems. At present,
this warns about calls to ‘printf’ and ‘scanf’ functions where the
format string is not a string literal and there are no format
arguments, as in ‘printf (foo);’. This may be a security hole if
the format string came from untrusted input and contains ‘%n’.
(This is currently a subset of what ‘-Wformat-nonliteral’ warns
about, but in future warnings may be added to ‘-Wformat-security’
that are not included in ‘-Wformat-nonliteral’.)
Your commit breaks it.
--
H.J.
On 25.05.2026 00:31, H.J. Lu wrote:
> On Sun, May 24, 2026 at 11:11 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 24.05.2026 02:09, H.J. Lu wrote:
>>> On Fri, May 15, 2026 at 9:08 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> Model this after operand handling, such that comments can be emitted in
>>>> the same order as operands. %rip-relative address comments remain
>>>> separate for now. While there correct style for the symbols associated
>>>> with immediates: These aren't "comment starts", but symbol names.
>>>> ---
>>>> As long as we want to continue to use the ->print_address_func() hook,
>>>> properly unifying %rip-relative comments with others won't be possible.
>>>> That, however, is in line with direct addresses also getting printed
>>>> specially, similarly by using ->print_address_func().
>>>>
>>>> Was it really intended for the original -Mannotate test to be run only for
>>>> Linux targets? It certainly isn't intended here, yet I'd like the new
>>>> tests to be in the same directory as the original one. x86-64.exp,
>>>> however, bails right away for non-Linux (and remote hosts). I guess at
>>>> least the *.d based testing loop should be moved up. Tests truly only
>>>> working for Linux can easily be constrained in the *.d files themselves.
>>>>
>>>> --- /dev/null
>>>> +++ b/binutils/testsuite/binutils-all/x86-64/comments.d
>>>> @@ -0,0 +1,14 @@
>>>> +#name: disassembly comments (AT&T)
>>>> +#ld:
>>>> +#objdump: -dwMannotate,att
>>>> +
>>>> +.*: +file format .*
>>>> +
>>>> +
>>>> +Disassembly of section .text:
>>>> +
>>>> +[0-9a-f]+ <_start>:
>>>> +[ ]*[0-9a-f]+: 48 c7 05 [0-9a-f ]+ movq \$0x[0-9a-f]+,0x[0-9a-f]+\(%rip\) +# \[_start\], [0-9a-f]+ <fptr>
>>>> +[ ]*[0-9a-f]+: 31 c0 xor %eax,%eax
>>>> +[ ]*[0-9a-f]+: c3 ret
>>>> +#pass
>>>> --- /dev/null
>>>> +++ b/binutils/testsuite/binutils-all/x86-64/comments.s
>>>> @@ -0,0 +1,9 @@
>>>> + .text
>>>> + .global _start
>>>> +_start:
>>>> + movq $_start, fptr(%rip)
>>>> + xor %eax, %eax
>>>> + ret
>>>> +
>>>> + .data
>>>> +fptr: .quad -1
>>>> --- /dev/null
>>>> +++ b/binutils/testsuite/binutils-all/x86-64/comments-intel.d
>>>> @@ -0,0 +1,14 @@
>>>> +#name: disassembly comments (Intel)
>>>> +#source: comments.s
>>>> +#ld:
>>>> +#objdump: -dwMannotate,intel
>>>> +
>>>> +.*: +file format .*
>>>> +
>>>> +
>>>> +Disassembly of section .text:
>>>> +
>>>> +[0-9a-f]+ <_start>:
>>>> +[ ]*[0-9a-f]+: 48 c7 05 [0-9a-f ]+ mov QWORD PTR \[rip\+0x[0-9a-f]+\],0x[0-9a-f]+ +# [0-9a-f]+ <fptr>, \[_start\]
>>>> +[ ]*[0-9a-f]+: 31 c0 xor eax,eax
>>>> +[ ]*[0-9a-f]+: c3 ret
>>>> --- a/opcodes/i386-dis.c
>>>> +++ b/opcodes/i386-dis.c
>>>> @@ -179,8 +179,7 @@ struct instr_info
>>>>
>>>> char obuf[MAX_OPERAND_BUFFER_SIZE];
>>>> char *obufp;
>>>> - char cbuf[COMMENT_BUFFER_SIZE];
>>>> - char * cbufp;
>>>> + char *cbufp;
>>>> char *mnemonicendp;
>>>> const uint8_t *start_codep;
>>>> uint8_t *codep;
>>>> @@ -258,6 +257,7 @@ struct instr_info
>>>> signed char op_index[MAX_OPERANDS];
>>>> bool op_riprel[MAX_OPERANDS];
>>>> char *op_out[MAX_OPERANDS];
>>>> + char *cm_out[MAX_OPERANDS];
>>>> bfd_vma op_address[MAX_OPERANDS];
>>>> bfd_vma start_pc;
>>>>
>>>> @@ -9749,7 +9749,6 @@ print_insn (bfd_vma pc, disassemble_info
>>>> .start_codep = priv.the_buffer,
>>>> .codep = priv.the_buffer,
>>>> .obufp = ins.obuf,
>>>> - .cbufp = ins.cbuf,
>>>> .last_lock_prefix = -1,
>>>> .last_repz_prefix = -1,
>>>> .last_repnz_prefix = -1,
>>>> @@ -9761,6 +9760,7 @@ print_insn (bfd_vma pc, disassemble_info
>>>> .fwait_prefix = -1,
>>>> };
>>>> char op_out[MAX_OPERANDS][MAX_OPERAND_BUFFER_SIZE];
>>>> + char cm_out[MAX_OPERANDS][COMMENT_BUFFER_SIZE];
>>>>
>>>> priv.orig_sizeflag = AFLAG | DFLAG;
>>>> if ((info->mach & bfd_mach_i386_i386) != 0)
>>>> @@ -9874,6 +9874,8 @@ print_insn (bfd_vma pc, disassemble_info
>>>> {
>>>> op_out[i][0] = 0;
>>>> ins.op_out[i] = op_out[i];
>>>> + cm_out[i][0] = 0;
>>>> + ins.cm_out[i] = cm_out[i];
>>>> }
>>>>
>>>> sizeflag = priv.orig_sizeflag;
>>>> @@ -9992,6 +9994,7 @@ print_insn (bfd_vma pc, disassemble_info
>>>> for (i = 0; i < MAX_OPERANDS; ++i)
>>>> {
>>>> ins.obufp = ins.op_out[i];
>>>> + ins.cbufp = ins.cm_out[i];
>>>> ins.op_ad = MAX_OPERANDS - 1 - i;
>>>> if (dp->op[i].rtn
>>>> && !dp->op[i].rtn (&ins, dp->op[i].bytemode, sizeflag))
>>>> @@ -10317,6 +10320,10 @@ print_insn (bfd_vma pc, disassemble_info
>>>> riprel = ins.op_riprel[i];
>>>> ins.op_riprel[i] = ins.op_riprel[MAX_OPERANDS - 1 - i];
>>>> ins.op_riprel[MAX_OPERANDS - 1 - i] = riprel;
>>>> +
>>>> + char *tmp = ins.cm_out[i];
>>>> + ins.cm_out[i] = ins.cm_out[MAX_OPERANDS - 1 - i];
>>>> + ins.cm_out[MAX_OPERANDS - 1 - i] = tmp;
>>>> }
>>>> }
>>>> else
>>>> @@ -10364,22 +10371,23 @@ print_insn (bfd_vma pc, disassemble_info
>>>> needcomma = 1;
>>>> }
>>>>
>>>> + const char *sep = " # ";
>>>> for (i = 0; i < MAX_OPERANDS; i++)
>>>> if (ins.op_index[i] != -1 && ins.op_riprel[i])
>>>> {
>>>> - i386_dis_printf (info, dis_style_comment_start, " # ");
>>>> + i386_dis_printf (info, dis_style_comment_start, sep);
>>>> + sep = ", ";
>>>> (*info->print_address_func)
>>>> ((bfd_vma)(ins.start_pc + (ins.codep - ins.start_codep)
>>>> + ins.op_address[ins.op_index[i]]),
>>>> - info);
>>>> - break;
>>>> + info);
>>>> + }
>>>> + else if (*ins.cm_out[i])
>>>> + {
>>>> + i386_dis_printf (info, dis_style_comment_start, sep);
>>>> + sep = ", ";
>>>> + i386_dis_printf (info, dis_style_symbol, "%s", ins.cm_out[i]);
>>>> }
>>>> - if (ins.cbufp != ins.cbuf)
>>>> - {
>>>> - if (i == MAX_OPERANDS)
>>>> - i386_dis_printf (info, dis_style_comment_start, " # ");
>>>> - i386_dis_printf (info, dis_style_comment_start, "%s", ins.cbuf);
>>>> - }
>>>>
>>>> ret = ins.codep - priv.the_buffer;
>>>> out:
>>>> @@ -10760,6 +10768,7 @@ dofloat (instr_info *ins, int sizeflag)
>>>
>>> commit aeced13ee0cd570d78cb37ab1f9bba840958515a
>>> Author: Jan Beulich <jbeulich@suse.com>
>>> Date: Fri May 22 08:49:12 2026 +0200
>>>
>>> x86/disasm: rework comment handling
>>>
>>> caused:
>>>
>>> libtool: compile: gcc -DHAVE_CONFIG_H -I. -I../../opcodes -I.
>>> -I../../opcodes -I../bfd -I../../opcodes/../include
>>> -I../../opcodes/../bfd -W -Wall -Wstrict-prototypes
>>> -Wmissing-prototypes -Wshadow -Wstack-usage=262144 -O2 -flto=auto
>>> -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall
>>> -Wno-complain-wrong-lang -Werror=format-security
>>> -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS
>>> -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
>>> -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1
>>> -m32 -march=i686 -mtune=generic -msse2 -mfpmath=sse -mstackrealign
>>> -fasynchronous-unwind-tables -fstack-clash-protection
>>> -mtls-dialect=gnu -flto=8 -std=gnu11 -fprofile-generate
>>> -flto=jobserver -ffat-lto-objects -MT i386-dis.lo -MD -MP -MF
>>> .deps/i386-dis.Tpo -c ../../opcodes/i386-dis.c -fPIC -DPIC -o
>>> .libs/i386-dis.o
>>> ../../opcodes/i386-dis.c: In function ‘print_insn’:
>>> ../../opcodes/i386-dis.c:10378:9: error: format not a string literal
>>> and no format arguments [-Werror=format-security]
>>> 10378 | i386_dis_printf (info, dis_style_comment_start, sep);
>>> | ^~~~~~~~~~~~~~~
>>> ../../opcodes/i386-dis.c:10387:9: error: format not a string literal
>>> and no format arguments [-Werror=format-security]
>>> 10387 | i386_dis_printf (info, dis_style_comment_start, sep);
>>> | ^~~~~~~~~~~~~~~
>>> cc1: some warnings being treated as errors
>>> make[4]: *** [Makefile:1076: i386-dis.lo] Error 1
>>>
>>> Since i386_dis_printf is declared as
>>>
>>> static void ATTRIBUTE_PRINTF_3 i386_dis_printf (const disassemble_info *,
>>> enum disassembler_style,
>>> const char *, ...);
>>>
>>> the 3rd argument should be a string literal as the format string. Pass
>>> "%s" to i386_dis_printf as the format string to fix the regression.
>>
>> What exactly does "should" here mean? I.e. what exactly is wrong with the
>> format string not being a literal? Unless that can be explained, it feels as
>> if first of all a gcc bug wants opening. That would then want referencing
>> here.
>>
>> Jan
>
> Binutils may be compiled with -Werror=format-security:
>
> ‘-Wformat-security’
> If ‘-Wformat’ is specified, also warn about uses of format
> functions that represent possible security problems. At present,
> this warns about calls to ‘printf’ and ‘scanf’ functions where the
> format string is not a string literal and there are no format
> arguments, as in ‘printf (foo);’. This may be a security hole if
> the format string came from untrusted input and contains ‘%n’.
> (This is currently a subset of what ‘-Wformat-nonliteral’ warns
> about, but in future warnings may be added to ‘-Wformat-security’
> that are not included in ‘-Wformat-nonliteral’.)
>
> Your commit breaks it.
It breaks the overly strong requirements by the compiler warning, but it
doesn't break anything in reality, nor does it break any standard C
requirements. Hence "should" is ambiguous / potentially misleading.
Jan
From 13bc2a41289071dad06f27330e30ec00b6122ba2 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Sun, 24 May 2026 08:04:26 +0800
Subject: [PATCH] x86: Pass "%s" to i386_dis_printf as the format string
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
commit aeced13ee0cd570d78cb37ab1f9bba840958515a
Author: Jan Beulich <jbeulich@suse.com>
Date: Fri May 22 08:49:12 2026 +0200
x86/disasm: rework comment handling
caused:
libtool: compile: gcc -DHAVE_CONFIG_H -I. -I../../opcodes -I. -I../../opcodes -I../bfd -I../../opcodes/../include -I../../opcodes/../bfd -W -Wall -Wstrict-prototypes -Wmissing-prototypes -Wshadow -Wstack-usage=262144 -O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Wno-complain-wrong-lang -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m32 -march=i686 -mtune=generic -msse2 -mfpmath=sse -mstackrealign -fasynchronous-unwind-tables -fstack-clash-protection -mtls-dialect=gnu -flto=8 -std=gnu11 -fprofile-generate -flto=jobserver -ffat-lto-objects -MT i386-dis.lo -MD -MP -MF .deps/i386-dis.Tpo -c ../../opcodes/i386-dis.c -fPIC -DPIC -o .libs/i386-dis.o
../../opcodes/i386-dis.c: In function ‘print_insn’:
../../opcodes/i386-dis.c:10378:9: error: format not a string literal and no format arguments [-Werror=format-security]
10378 | i386_dis_printf (info, dis_style_comment_start, sep);
| ^~~~~~~~~~~~~~~
../../opcodes/i386-dis.c:10387:9: error: format not a string literal and no format arguments [-Werror=format-security]
10387 | i386_dis_printf (info, dis_style_comment_start, sep);
| ^~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
make[4]: *** [Makefile:1076: i386-dis.lo] Error 1
Since i386_dis_printf is declared as
static void ATTRIBUTE_PRINTF_3 i386_dis_printf (const disassemble_info *,
enum disassembler_style,
const char *, ...);
the 3rd argument should be a string literal as the format string. Pass
"%s" to i386_dis_printf as the format string to fix the regression.
PR binutils/34168
* i386-dis.c (print_insn): Pass "%s" to i386_dis_printf as the
format string.
Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
---
opcodes/i386-dis.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -10375,7 +10375,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
for (i = 0; i < MAX_OPERANDS; i++)
if (ins.op_index[i] != -1 && ins.op_riprel[i])
{
- i386_dis_printf (info, dis_style_comment_start, sep);
+ i386_dis_printf (info, dis_style_comment_start, "%s", sep);
sep = ", ";
(*info->print_address_func)
((bfd_vma)(ins.start_pc + (ins.codep - ins.start_codep)
@@ -10384,7 +10384,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
}
else if (*ins.cm_out[i])
{
- i386_dis_printf (info, dis_style_comment_start, sep);
+ i386_dis_printf (info, dis_style_comment_start, "%s", sep);
sep = ", ";
i386_dis_printf (info, dis_style_symbol, "%s", ins.cm_out[i]);
}
--
2.54.0