Re: [PATCH] LoongArch: Make align symbol be in same section with alignment directive

[Jinyang He <hejinyang@loongson.cn>]

On 2024-01-06 19:18, Xi Ruoyao wrote:

> On Sat, 2024-01-06 at 15:23 +0800, Jinyang He wrote:
>> R_LARCH_ALIGN (psABI v2.30) requires a symbol index. The symbol is only
>> created at the first time to handle alignment directive. This means that
>> all other sections may use this symbol. If the section of this symbol is
>> discarded, there may be problems. Search it in its own section.
>>
>> gas/ChangeLog:
>>
>> 	* config/tc-loongarch.c: (get_align_symbol) New function.
>> 	* gas/testsuite/gas/loongarch/relax_align2.d: New test.
>> 	* gas/testsuite/gas/loongarch/relax_align2.s: Likewise.
>>
>> Reported-by: WANG Xuerui <git@xen0n.name>
>> Link: https://lore.kernel.org/loongarch/2abbb633-a10e-71cc-a5e1-4d9e39074066@loongson.cn/T/#t
> Hmm, but shouldn't we avoid using R_LARCH_ALIGN in the first place if -
> mno-relax?

That is true, but currently there is no test that R_LARCH_ALIGN is used
in -mno-relax. I suspect that is not the key about this modpost problem.
I used "make V=1" to find the error command, which was
   "scripts/mod/modpost -M -m -o Module.symvers -T modules.order vmlinux.o".
R_LARCH_ALIGN is present in vmlinux.o because the kernel does not add
arguments to the Makefile to avoid relax. We can simply modify the
modpost to avoid reporting error. But the real reason is that the section
of the symbol is inconsistent with the section of the alignment directive.
Fix it in GNU assembler can reduce other potential errors.

Thanks,

Jinyang

>> ---
>>   gas/config/tc-loongarch.c                  | 53 ++++++++++++++++++++--
>>   gas/testsuite/gas/loongarch/relax_align2.d | 24 ++++++++++
>>   gas/testsuite/gas/loongarch/relax_align2.s | 11 +++++
>>   3 files changed, 84 insertions(+), 4 deletions(-)
>>   create mode 100644 gas/testsuite/gas/loongarch/relax_align2.d
>>   create mode 100644 gas/testsuite/gas/loongarch/relax_align2.s
>>
>> diff --git a/gas/config/tc-loongarch.c b/gas/config/tc-loongarch.c
>> index 5348371fa2c..29e540df429 100644
>> --- a/gas/config/tc-loongarch.c
>> +++ b/gas/config/tc-loongarch.c
>> @@ -392,6 +392,50 @@ loongarch_target_format ()
>>     return LARCH_opts.ase_lp64 ? "elf64-loongarch" : "elf32-loongarch";
>>   }
>>   
>> +typedef struct
>> +{
>> +  segT sec;
>> +  symbolS *s;
>> +} align_sec_sym;
>> +
>> +static htab_t align_hash;
>> +
>> +static hashval_t
>> +align_sec_sym_hash (const void *entry)
>> +{
>> +  const align_sec_sym *e = entry;
>> +  return (hashval_t) e->sec;
>> +}
>> +
>> +static int
>> +align_sec_sym_eq (const void *entry1, const void *entry2)
>> +{
>> +  const align_sec_sym *e1 = entry1, *e2 = entry2;
>> +  return e1->sec == e2->sec;
>> +}
>> +
>> +static symbolS *get_align_symbol(segT sec)
>> +{
>> +  align_sec_sym search = { sec, NULL };
>> +  align_sec_sym *pentry = htab_find (align_hash, &search);
>> +  if (pentry)
>> +    return pentry->s;
>> +
>> +  /* If we not find the symbol in this section. Create and insert it. */
>> +  symbolS *s = (symbolS *)local_symbol_make (".Lla-relax-align", sec,
>> +					     &zero_address_frag, 0);
>> +  align_sec_sym entry = { sec, s };
>> +  align_sec_sym **slot =
>> +      (align_sec_sym **) htab_find_slot (align_hash, &entry, INSERT);
>> +  if (slot == NULL)
>> +    return NULL;
>> +  *slot = (align_sec_sym *) xmalloc (sizeof (align_sec_sym));
>> +  if (*slot == NULL)
>> +    return NULL;
>> +  **slot = entry;
>> +  return entry.s;
>> +}
>> +
>>   void
>>   md_begin ()
>>   {
>> @@ -413,6 +457,8 @@ md_begin ()
>>   		    it->name, it->format, it->macro);
>>         }
>>   
>> +  align_hash = htab_create (10, align_sec_sym_hash, align_sec_sym_eq, free);
>> +
>>     /* FIXME: expressionS use 'offsetT' as constant,
>>      * we want this is 64-bit type.  */
>>     assert (8 <= sizeof (offsetT));
>> @@ -1721,10 +1767,9 @@ loongarch_frag_align_code (int n, int max)
>>   
>>     nops = frag_more (worst_case_bytes);
>>   
>> -  s = symbol_find (".Lla-relax-align");
>> -  if (s == NULL)
>> -    s = (symbolS *)local_symbol_make (".Lla-relax-align", now_seg,
>> -				      &zero_address_frag, 0);
>> +  s = get_align_symbol(now_seg);
>> +  if (!s)
>> +    as_fatal (_("internal error: cannot get align symbol"));
>>   
>>     ex.X_add_symbol = s;
>>     ex.X_op = O_symbol;
>> diff --git a/gas/testsuite/gas/loongarch/relax_align2.d b/gas/testsuite/gas/loongarch/relax_align2.d
>> new file mode 100644
>> index 00000000000..d647d9b7eef
>> --- /dev/null
>> +++ b/gas/testsuite/gas/loongarch/relax_align2.d
>> @@ -0,0 +1,24 @@
>> +#as: --no-warn
>> +#readelf: -rsW
>> +#skip: loongarch32-*-*
>> +
>> +Relocation section '\.rela\.text' at offset .* contains 2 entries:
>> +.*
>> +0+04[ 	]+0000000500000066[ 	]+R_LARCH_ALIGN[ 	]+0+[ 	]+\.Lla-relax-align \+ 4
>> +0+14[ 	]+0000000500000066[ 	]+R_LARCH_ALIGN[ 	]+0+[ 	]+\.Lla-relax-align \+ 404
>> +
>> +Relocation section '\.rela\.text2' at offset .* contains 2 entries:
>> +.*
>> +0+04[ 	]+0000000600000066[ 	]+R_LARCH_ALIGN[ 	]+0+[ 	]+\.Lla-relax-align \+ 4
>> +0+14[ 	]+0000000600000066[ 	]+R_LARCH_ALIGN[ 	]+0+[ 	]+\.Lla-relax-align \+ 404
>> +
>> +Symbol table '\.symtab' contains .* entries:
>> +#...
>> +[ 	]+.*:[ 	]+0+[ 	]+0[ 	]+SECTION[ 	]+LOCAL[ 	]+DEFAULT[ 	]+1[ 	]+\.text
>> +#...
>> +[ 	]+.*:[ 	]+0+[ 	]+0[ 	]+SECTION[ 	]+LOCAL[ 	]+DEFAULT[ 	]+5[ 	]+\.text2
>> +#...
>> +[ 	]+.*:[ 	]+0+[ 	]+0[ 	]+NOTYPE[ 	]+LOCAL[ 	]+DEFAULT[ 	]+1[ 	]+\.Lla-relax-align
>> +#...
>> +[ 	]+.*:[ 	]+0+[ 	]+0[ 	]+NOTYPE[ 	]+LOCAL[ 	]+DEFAULT[ 	]+5[ 	]+\.Lla-relax-align
>> +#pass
>> diff --git a/gas/testsuite/gas/loongarch/relax_align2.s b/gas/testsuite/gas/loongarch/relax_align2.s
>> new file mode 100644
>> index 00000000000..6cd6bd8731b
>> --- /dev/null
>> +++ b/gas/testsuite/gas/loongarch/relax_align2.s
>> @@ -0,0 +1,11 @@
>> +.section ".text", "ax"
>> +nop
>> +.align 4
>> +nop
>> +.align 4, , 4
>> +
>> +.section ".text2", "ax"
>> +nop
>> +.align 4
>> +nop
>> +.align 4, , 4