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

[Palmer Dabbelt <palmer@dabbelt.com>]

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 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.

> --- a/gas/app.c
> +++ b/gas/app.c
> @@ -200,6 +200,11 @@ do_scrub_begin (int m68k_mri ATTRIBUTE_U
>    lex['-'] = LEX_IS_SYMBOL_COMPONENT;
>  #endif
>
> +  /* If not otherwise used (which it shouldn't be - see write.h), mark the
> +     fake label character as a possible part of symbol names.  */
> +  if (!lex[(unsigned char) FAKE_LABEL_CHAR])
> +    lex[(unsigned char) FAKE_LABEL_CHAR] = LEX_IS_SYMBOL_COMPONENT;
> +
>  #ifdef H_TICK_HEX
>    if (enable_h_tick_hex)
>      {
> --- a/gas/config/tc-riscv.h
> +++ b/gas/config/tc-riscv.h
> @@ -38,8 +38,7 @@ struct expressionS;
>  #define LOCAL_LABELS_FB 	1
>
>  /* Symbols named FAKE_LABEL_NAME are emitted when generating DWARF, so make
> -   sure FAKE_LABEL_NAME is printable.  It still must be distinct from any
> -   real label name.  So, append a space, which other labels can't contain.  */
> +   sure FAKE_LABEL_NAME is printable.  See write.h for constraints.  */
>  #define FAKE_LABEL_NAME RISCV_FAKE_LABEL_NAME
>  /* Changing the special character in FAKE_LABEL_NAME requires changing
>     FAKE_LABEL_CHAR too.  */
> --- a/gas/testsuite/gas/all/altmacro.d
> +++ b/gas/testsuite/gas/all/altmacro.d
> @@ -8,4 +8,4 @@
>
>  Contents of section .data:
>   0000 01020912 61626331 32332121 3c3e2721 .*
> - 0010 3c3e27.*
> + 0010 3c3e27a2 11.*
> --- a/gas/testsuite/gas/all/altmacro.s
> +++ b/gas/testsuite/gas/all/altmacro.s
> @@ -33,3 +33,9 @@ m4	"!!<>'"
>  	.altmacro
>
>  m3	"!!<>'"
> +
> +	.macro	m2b v1, v2
> +	m1 %v2-v1 17
> +	.endm
> +
> +	m2b 81 243
> --- a/gas/write.h
> +++ b/gas/write.h
> @@ -30,7 +30,8 @@
>  /* This is a special character that is used to indicate a fake label.
>     It must be present in FAKE_LABEL_NAME, although it does not have to
>     be the first character.  It must not be a character that would be
> -   found in a valid symbol name.
> +   found in a valid symbol name, nor one that starts an operator, nor
> +   one that's a separator of some kind.
>
>     Also be aware that the function _bfd_elf_is_local_label_name in
>     bfd/elf.c has an implicit assumption that FAKE_LABEL_CHAR is '\001'.
> --- a/include/opcode/riscv.h
> +++ b/include/opcode/riscv.h
> @@ -323,9 +323,10 @@ static inline unsigned int riscv_insn_le
>
>  /* These fake label defines are use by both the assembler, and
>     libopcodes.  The assembler uses this when it needs to generate a fake
> -   label, and libopcodes uses it to hide the fake labels in its output.  */
> -#define RISCV_FAKE_LABEL_NAME ".L0 "
> -#define RISCV_FAKE_LABEL_CHAR ' '
> +   label, and libopcodes uses it to hide the fake labels in its output.
> +   See gas/write.h for constraints.  */
> +#define RISCV_FAKE_LABEL_NAME ".L0?"
> +#define RISCV_FAKE_LABEL_CHAR '?'
>
>  /* Replace bits MASK << SHIFT of STRUCT with the equivalent bits in
>     VALUE << SHIFT.  VALUE is evaluated exactly once.  */