[PATCH] opcodes/i386: remove trailing whitespace from insns with zero operands

[Jan Beulich]

On 26.05.2022 14:45, Andrew Burgess via Binutils wrote:
> While working on another patch[1] I had need to touch this code in
> i386-dis.c:
> 
>   ins->obufp = ins->mnemonicendp;
>   for (i = strlen (ins->obuf) + prefix_length; i < 6; i++)
>     oappend (ins, " ");
>   oappend (ins, " ");
>   (*ins->info->fprintf_styled_func)
>     (ins->info->stream, dis_style_mnemonic, "%s", ins->obuf);
> 
> What this code does is add whitespace after the instruction mnemonic
> and before the instruction operands.
> 
> The problem I ran into when working on this code can be seen by
> assembling this input file:
> 
>     .text
>     nop
>     retq
> 
> Now, when I disassemble, here's the output.  I've replaced trailing
> whitespace with '_' so that the issue is clearer:
> 
>     Disassembly of section .text:
> 
>     0000000000000000 <.text>:
>        0:	90                   	nop
>        1:	c3                   	retq___
> 
> Notice that there's no trailing whitespace after 'nop', but there are
> three spaces after 'retq'!
> 
> What happens is that instruction mnemonics are emitted into a buffer
> instr_info::obuf, then instr_info::mnemonicendp is setup to point to
> the '\0' character at the end of the mnemonic.
> 
> When we emit the whitespace, this is then added starting at the
> mnemonicendp position.  Lets consider 'retq', first the buffer is
> setup like this:
> 
>   'r' 'e' 't' 'q' '\0'
> 
> Then we add whitespace characters at the '\0', converting the buffer
> to this:
> 
>   'r' 'e' 't' 'q' ' ' ' ' ' ' '\0'
> 
> However, 'nop' is actually an alias for 'xchg %rax,%rax', so,
> initially, the buffer is setup like this:
> 
>   'x' 'c' 'h' 'g' '\0'
> 
> Then in NOP_Fixup we spot that we have an instruction that is an alias
> for 'nop', and adjust the buffer to this:
> 
>   'n' 'o' 'p' '\0' '\0'
> 
> The second '\0' is left over from the original buffer contents.
> However, when we rewrite the buffer, we don't afjust mnemonicendp,
> which still points at the second '\0' character.
> 
> Now, when we insert whitespace we get:
> 
>   'n' 'o' 'p' '\0' ' ' ' ' ' ' ' ' '\0'
> 
> Notice the whitespace is inserted after the first '\0', so, when we
> print the buffer, the whitespace is not printed.
> 
> The fix for this is pretty easy, I can change NOP_Fixup to adjust
> mnemonicendp, but now a bunch of tests start failing, we now produce
> whitespace after the 'nop', which the tests don't expect.
> 
> So, I could update the tests to expect the whitespace....
> 
> ...except I'm not a fan of trailing whitespace, so I'd really rather
> not.
> 
> Turns out, I can pretty easily update the whitespace emitting code to
> spot instructions that have zero operands and just not emit any
> whitespace in this case.  So this is what I've done.

I appreciate this; I never understood what the trailing blanks were
good for (and I hadn't noticed the NOP anomaly so far). I'll leave
approving of the change to H.J., though, just indicating here that
all looks good to me.

> I've left in the fix for NOP_Fixup, I think updating mnemonicendp is
> probably a good thing, though this is not really required any more.

I agree with keeping it.

> I've then updated all the tests that I saw failing to adjust the
> expected patterns to account for the change in whitespace.

You could have left alone the ones using " *", reducing overall
patch size, but since you went farther then that - even better.

> For reviewing, start with opcodes/i386-dis.c, that's the actual change
> to how whitespace is emitted.  If you're happy with that then
> everything else is testsuite updates.

I think this doesn't belong in the commit message, but would better
go ...

> [1] https://sourceware.org/pipermail/binutils/2022-April/120610.html
> ---

... below such a marker.

Jan