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

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

[Jinyang He]

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

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

[Xi Ruoyao]

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?

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

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

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

[Jinyang He]

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

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

[Xi Ruoyao]

On Sun, 2024-01-07 at 18:27 +0800, Jinyang He wrote:
> 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.

Yes, we'll disable relax for kernel as well (currently we only disable
it for modules) in the next kernel release but if we can make objtool
support relax we may enable it again in the future.

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

Agree.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University