Re: [PATCH RFC] RISC-V: alter the special character used in FAKE_LABEL_NAME

[Jan Beulich <jbeulich@suse.com>]

On 15.03.2023 17:05, Palmer Dabbelt wrote:
> On Fri, 10 Mar 2023 01:36:31 PST (-0800), jbeulich@suse.com wrote:
>> The use of a space char there collides with anything using temp_ilp(),
>> i.e. at present at least with the special % operator available in
>> alternate macro mode. Further uses of the function may appear at any
>> time. This likely is a result of write.h's comment not properly
>> spelling out all the constraints placed on the selection of the special
>> character used. Then again RISC-V anyway violates a constraint which has
>> been properly spelled out there: Such labels _do_ appear in assembler
>> output.
>> ---
>> RFC: Of course this breaks interoperability between older gas / new
>>      objdump and vice versa. But I don't see a way to resolve the issue
>>      at hand without introducing such a discontinuity. To limit "damage"
>>      a little, riscv_symbol_is_valid() could of course be tought to also
>>      ignore old style fake label names. (Personally I view this tying of
>>      functionality between assembler and disassembler as problematic
>>      anyway.) Thoughts?
> 
> This wouldn't be the first time we've made a change around here in 
> RISC-V land, see 2469b3c5844 ("Riscv ld-elf/stab failure and fake label 
> cleanup.").  Unless I'm missing something we maintained compatibility 
> with the old binaries there, I'd prefer if we can do that here too.  

That commit didn't touch the binutils/ subtree at all, so "existing"
binaries would no longer have had their "helper" symbols suppressed. And
indeed current code is merely

bool
riscv_symbol_is_valid (asymbol * sym,
                       struct disassemble_info * info ATTRIBUTE_UNUSED)
{
  const char * name;

  if (sym == NULL)
    return false;

  name = bfd_asymbol_name (sym);

  return (strcmp (name, RISCV_FAKE_LABEL_NAME) != 0
	  && !riscv_elf_is_mapping_symbols (name));
}

i.e. no check for the earlier used special name. Yet the RFC question
is specifically to figure out whether riscv_symbol_is_valid() should
also be adjusted (at which point I could of course go and make it
recognize the earlier name as well as the one that is in use prior to
the patch here. (The RFC question also extends in the other direction,
as we can't really do anything about old objdump then no longer
recognizing the new label name. But that again would be no different
from the earlier discontinuity, afaict.)

> That said...
> 
>> I question the use of FAKE_LABEL_NAME in make_internal_label(). The
>> comment in tc-riscv.h isn't correct anyway because of (at least) this
>> use - the symbols generated there are never used for Dwarf. And them all
>> being the same makes it rather hard to associate relocations resolved to
>> symbol names (e.g. "objdump -dr" output) with the actual instance that's
>> referenced. Their naming should imo rather follow the model of
>> {fb,dollar}_label_name().
>>
>> Considering the special casing of FAKE_LABEL_CHAR in read_symbol_name()
>> and get_symbol_name() I wonder in how far LOCAL_LABEL_CHAR and/or
>> DOLLAR_LABEL_CHAR don't also need recognizing there (and then also be
>> marked in lex[] just like done here). I can't point out a specific case
>> though where there could be a problem.
>>
>> The checking for *_LABEL_CHAR in S_IS_LOCAL() looks to collide with uses
>> of these characters in quoted symbol names.
> 
> ... if this is all broken anyway then I guess compatibility doesn't 
> matter that much?  I've definately seen some oddness around here, but I 
> think it generally works.

The latter two of these remarks were general observations while doing the
work, the first of which Nick meanwhile has reassured me is not an issue.
Dealing with the first remark above would (aiui) introduce another
discontinuity, though, so dealing with both aspects at (about) the same
time (i.e. at least within a single release cycle) might be best. Provided
of course it is acceptable in the first place to change
make_internal_label().

Jan