Re: [PATCH] ARM: fix macro expansion with a pld insn as a macro argument

["Jan Beulich" <JBeulich at suse dot com>]

>>> On 12.11.12 at 19:34, Roland McGrath <mcgrathr@google.com> wrote:
> On Sun, Nov 11, 2012 at 11:45 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> Including the possibility of symbol names now suddenly being
>> allowed to include those characters?
> 
> The change has no such effect.  tc_symbol_chars only affects the APP pass
> (whitespace canonicalization).  It does not change the actual parsing of
> operands at all.
> 
>>> +/* An immediate operand can start with #, and ld*, st*, pld operands
>>> +   can contain [ and ].  We need to tell APP not to elide whitespace
>>> +   before a [, which can appear as the first operand for pld.  */
>>> +const char arm_symbol_chars[] = "#[]";
>>
>> For your purposes, wouldn't it suffice to just have "[" here? Or
>> otherwise, the comment appears inconsistent with the actual set
>> of characters.
> 
> tc_symbol_chars is described as "characters which may appear in an operand".
> For my test case, just "[" would be sufficient.  But as #[] are characters
> which may appear in an operand, it seemed the appropriate setting.  Since ]
> never appears somewhere where whitespace before it is relevant, it makes no
> difference to include it and it could be omitted if one wanted to explain
> the divergence from the simple documentation of what tc_symbol_chars is for.

Oh, right, it's been too long since I last looked at that. Sorry for
the confusion.

> There is an example where this is relevant for #, but its presence in
> line_comment_chars trumps tc_symbol_chars and so that is still broken:
> 
> 	.macro foo arg, rest:vararg
> 		\rest
> 	.endm
> 		foo r0, svc #123
> 
> But I'm not immediately concerned with this case, so I'm only sending the
> fix for the bug that I cited originally.

That's pretty unfortunate. I'd really like to see problems like this
to be addressed in their entirety, rather than just piecewise -
that's, in my opinion, part of the reason why there are so many
half baked things in binutils, which the unsuspecting way too
easily stumbles across.

>> Along with the first comment above, this seems the wrong
>> approach to me in any case. Instead, I would expect these
>> characters to be treated as operator ones, which ought to have
>> the effect of the elided white space to be benign. Or is there a
>> strict need to have white space between opcode and first
>> operand, even if the boundary is recognizable without?
> 
> This reaction seems to be based on some generally sensible theory of
> parsing, which has nothing to do with how GAS actaully behaves.  The APP
> pass canonicalizes whitespace in a string that still has not been parsed or
> tokenized in any general way.  The string then gets to the actual parsing
> of lines via md_assemble, and that relies on having a space between the
> mnemonic an the first operand.

Fact is that this pre-parsing has been causing problems every
once in a while, but I don't see anyone stepping up and fixing
this altogether (not the least because of how fragile this is, and
hence how high the potential of regressions would be).

Jan