[LoongArch] Use R_LARCH_ALIGN with section symbol #84741

MQ-mengqing · 2024-03-11T10:26:08Z

In LoongArch psABI v2.30, the R_LARCH_ALIGN requires symbol index to support the third parameter of alignment directive. Create symbol for each section is redundant because they have section symbol which can also be used as symbol index. So use section symbol directly for R_LARCH_ALIGN.

llvmbot · 2024-03-11T10:26:37Z

@llvm/pr-subscribers-lld
@llvm/pr-subscribers-lld-elf
@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-loongarch

Author: Jinyang He (MQ-mengqing)

Changes

In LoongArch psABI v2.30, the R_LARCH_ALIGN requires symbol index to support the third parameter of alignment directive. Create symbol for each section is redundant because they have section symbol which can also be used as symbol index. So use section symbol directly for R_LARCH_ALIGN.

Full diff: https://github.com/llvm/llvm-project/pull/84741.diff

6 Files Affected:

(modified) lld/ELF/InputSection.cpp (+5-1)
(added) lld/test/ELF/loongarch-relax-align-ldr.s (+27)
(modified) lld/test/ELF/loongarch-relax-emit-relocs.s (+3-2)
(modified) llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp (+2-5)
(modified) llvm/test/MC/LoongArch/Relocations/relax-addsub.s (+1-1)
(modified) llvm/test/MC/LoongArch/Relocations/relax-align.s (+8-6)

diff --git a/lld/ELF/InputSection.cpp b/lld/ELF/InputSection.cpp
index e033a715b59214..313a19426a5e20 100644
--- a/lld/ELF/InputSection.cpp
+++ b/lld/ELF/InputSection.cpp
@@ -462,7 +462,11 @@ void InputSection::copyRelocations(uint8_t *buf,
         addend += sec->getFile<ELFT>()->mipsGp0;
       }
 
-      if (RelTy::IsRela)
+      if (config->emachine == EM_LOONGARCH && type == R_LARCH_ALIGN)
+        // LoongArch psABI v2.30, the R_LARCH_ALIGN requires symbol index.
+        // If it use the section symbol, the addend should not be changed.
+        p->r_addend = addend;
+      else if (RelTy::IsRela)
         p->r_addend = sym.getVA(addend) - section->getOutputSection()->addr;
       // For SHF_ALLOC sections relocated by REL, append a relocation to
       // sec->relocations so that relocateAlloc transitively called by
diff --git a/lld/test/ELF/loongarch-relax-align-ldr.s b/lld/test/ELF/loongarch-relax-align-ldr.s
new file mode 100644
index 00000000000000..da4685579b72e5
--- /dev/null
+++ b/lld/test/ELF/loongarch-relax-align-ldr.s
@@ -0,0 +1,27 @@
+# REQUIRES: loongarch
+
+# RUN: llvm-mc --filetype=obj --triple=loongarch64 --mattr=+relax %s -o %t.64.o
+# RUN: ld.lld -r %t.64.o %t.64.o -o %t.64.r
+# RUN: llvm-objdump -dr --no-show-raw-insn %t.64.r | FileCheck %s
+
+# CHECK:      <.text>:
+# CHECK-NEXT:   break 1
+# CHECK-NEXT:   nop
+# CHECK-NEXT:   {{0*}}04:  R_LARCH_ALIGN        .text+0x804
+# CHECK-NEXT:   nop
+# CHECK-NEXT:   nop
+# CHECK-NEXT:   break 2
+# CHECK-NEXT:   break 0
+# CHECK-NEXT:   break 0
+# CHECK-NEXT:   break 0
+# CHECK-NEXT:   break 1
+# CHECK-NEXT:   nop
+# CHECK-NEXT:   {{0*}}24:  R_LARCH_ALIGN        .text+0x804
+# CHECK-NEXT:   nop
+# CHECK-NEXT:   nop
+# CHECK-NEXT:   break 2
+
+.text
+break 1
+.p2align 4, , 8
+break 2
diff --git a/lld/test/ELF/loongarch-relax-emit-relocs.s b/lld/test/ELF/loongarch-relax-emit-relocs.s
index 581fce8c95caa4..9007f8fcc114f0 100644
--- a/lld/test/ELF/loongarch-relax-emit-relocs.s
+++ b/lld/test/ELF/loongarch-relax-emit-relocs.s
@@ -25,7 +25,7 @@
 # CHECK-NEXT:     R_LARCH_PCALA_LO12 _start
 # CHECK-NEXT:     R_LARCH_RELAX *ABS*
 # CHECK-NEXT:   nop
-# CHECK-NEXT:     R_LARCH_ALIGN .Lla-relax-align0+0x4
+# CHECK-NEXT:     R_LARCH_ALIGN .text+0x4
 # CHECK-NEXT:   nop
 # CHECK-NEXT:   ret
 
@@ -37,11 +37,12 @@
 # CHECKR-NEXT:     R_LARCH_PCALA_LO12 _start
 # CHECKR-NEXT:     R_LARCH_RELAX *ABS*
 # CHECKR-NEXT:   nop
-# CHECKR-NEXT:     R_LARCH_ALIGN .Lla-relax-align0+0x4
+# CHECKR-NEXT:     R_LARCH_ALIGN .text+0x4
 # CHECKR-NEXT:   nop
 # CHECKR-NEXT:   nop
 # CHECKR-NEXT:   ret
 
+.text
 .global _start
 _start:
   la.pcrel $a0, _start
diff --git a/llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp b/llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
index de492f2b1f0a4f..98f5014a34b1de 100644
--- a/llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
+++ b/llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
@@ -226,11 +226,8 @@ bool LoongArchAsmBackend::shouldInsertFixupForCodeAlign(
       MCFixup::create(0, Dummy, MCFixupKind(LoongArch::fixup_loongarch_align));
   const MCSymbolRefExpr *MCSym = getSecToAlignSym()[Sec];
   if (MCSym == nullptr) {
-    // Create a symbol and make the value of symbol is zero.
-    MCSymbol *Sym = Ctx.createNamedTempSymbol("la-relax-align");
-    Sym->setFragment(&*Sec->getBeginSymbol()->getFragment());
-    Asm.registerSymbol(*Sym);
-    MCSym = MCSymbolRefExpr::create(Sym, Ctx);
+    // Use section symbol directly.
+    MCSym = MCSymbolRefExpr::create(Sec->getBeginSymbol(), Ctx);
     getSecToAlignSym()[Sec] = MCSym;
   }
 
diff --git a/llvm/test/MC/LoongArch/Relocations/relax-addsub.s b/llvm/test/MC/LoongArch/Relocations/relax-addsub.s
index 18e0ede5e29375..0e27d6301bb3cd 100644
--- a/llvm/test/MC/LoongArch/Relocations/relax-addsub.s
+++ b/llvm/test/MC/LoongArch/Relocations/relax-addsub.s
@@ -28,7 +28,7 @@
 
 # RELAX:       Relocations [
 # RELAX-NEXT:    Section ({{.*}}) .rela.text {
-# RELAX-NEXT:      0x4 R_LARCH_ALIGN {{.*}} 0x4
+# RELAX-NEXT:      0x4 R_LARCH_ALIGN .text 0x4
 # RELAX-NEXT:      0x10 R_LARCH_PCALA_HI20 .L1 0x0
 # RELAX-NEXT:      0x10 R_LARCH_RELAX - 0x0
 # RELAX-NEXT:      0x14 R_LARCH_PCALA_LO12 .L1 0x0
diff --git a/llvm/test/MC/LoongArch/Relocations/relax-align.s b/llvm/test/MC/LoongArch/Relocations/relax-align.s
index 294fd9fb916c75..0246d5b46431c9 100644
--- a/llvm/test/MC/LoongArch/Relocations/relax-align.s
+++ b/llvm/test/MC/LoongArch/Relocations/relax-align.s
@@ -63,17 +63,19 @@ ret
 ## Test the symbol index is different from .text.
 .section .text2, "ax"
 .p2align 4
+.p2align 4, , 4
 break 7
 
 # RELOC:            Relocations [
 # RELAX-RELOC-NEXT:   Section ({{.*}}) .rela.text {
-# RELAX-RELOC-NEXT:     0x24 R_LARCH_ALIGN .Lla-relax-align0 0x4
-# RELAX-RELOC-NEXT:     0x34 R_LARCH_ALIGN .Lla-relax-align0 0x5
-# RELAX-RELOC-NEXT:     0x50 R_LARCH_ALIGN .Lla-relax-align0 0x4
-# RELAX-RELOC-NEXT:     0x60 R_LARCH_ALIGN .Lla-relax-align0 0xB04
-# RELAX-RELOC-NEXT:     0x70 R_LARCH_ALIGN .Lla-relax-align0 0x4
+# RELAX-RELOC-NEXT:     0x24 R_LARCH_ALIGN .text 0x4
+# RELAX-RELOC-NEXT:     0x34 R_LARCH_ALIGN .text 0x5
+# RELAX-RELOC-NEXT:     0x50 R_LARCH_ALIGN .text 0x4
+# RELAX-RELOC-NEXT:     0x60 R_LARCH_ALIGN .text 0xB04
+# RELAX-RELOC-NEXT:     0x70 R_LARCH_ALIGN .text 0x4
 # RELAX-RELOC-NEXT:   }
 # RELAX-RELOC-NEXT:   Section ({{.*}}) .rela.text2 {
-# RELAX-RELOC-NEXT:     0x0 R_LARCH_ALIGN .Lla-relax-align1 0x4
+# RELAX-RELOC-NEXT:     0x0 R_LARCH_ALIGN .text2 0x4
+# RELAX-RELOC-NEXT:     0xC R_LARCH_ALIGN .text2 0x404
 # RELAX-RELOC-NEXT:   }
 # RELOC-NEXT:       ]

MQ-mengqing · 2024-03-11T10:27:09Z

Add @SixWeining @MaskRay @wangleiat @xen0n

SixWeining · 2024-03-11T11:50:11Z

lld/ELF/InputSection.cpp

+        // LoongArch psABI v2.30, the R_LARCH_ALIGN requires symbol index.
+        // If it use the section symbol, the addend should not be changed.
+        p->r_addend = addend;
+      else if (RelTy::IsRela)


How about:

if (RelTy::IsRela && !(config->emachine == EM_LOONGARCH && type == R_LARCH_ALIGN)) ....

I am more convinced that the generic function should be moved to Arch/ as I did for relocateAlloc. You might not be interested in doing such generic improvement. I'll investigate it.

if (RelTy::IsRela && !(config->emachine == EM_LOONGARCH && type == R_LARCH_ALIGN)) ....

But it will go next if condition and do something wrong.

lld/test/ELF/loongarch-relax-align-ldr.s

MaskRay · 2024-03-12T17:02:51Z

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp

@@ -226,11 +226,8 @@ bool LoongArchAsmBackend::shouldInsertFixupForCodeAlign(
      MCFixup::create(0, Dummy, MCFixupKind(LoongArch::fixup_loongarch_align));


lld is not coupled with MC. Changes to both places are usually done in two patches.

But llvm-mc is used in lld's tests. If llvm-mc is changed, lld's test will fail (i.e. lld/test/ELF/loongarch-relax-emit-relocs.s).

In LoongArch psABI v2.30, the R_LARCH_ALIGN requires symbol index to support the third parameter of alignment directive. Create symbol for each section is redundant because they have section symbol which can also be used as symbol index. So use section symbol directly for R_LARCH_ALIGN.

SixWeining · 2024-04-03T07:03:55Z

binutils has made same change:

commit daeda14191c1710ce967259a47ef4e0a3fb6eebf
Author: mengqinggang <mengqinggang@loongson.cn>
Date:   Wed Jan 24 14:34:26 2024 +0800

    BFD: Fix the bug of R_LARCH_AGLIN caused by discard section
    
    To represent the first and third expression of .align, R_LARCH_ALIGN need to
    associate with a symbol. We define a local symbol for R_LARCH_AGLIN.
    But if the section of the local symbol is discarded, it may result in
    a undefined symbol error.
    
    Instead, we use the section name symbols, and this does not need to
    add extra symbols.
    
    During partial linking (ld -r), if the symbol associated with a relocation is
    STT_SECTION type, the addend of relocation needs to add the section output
    offset. We prevent it for R_LARCH_ALIGN.
    
    The elf_backend_data.rela_normal only can set all relocations of a target to
    rela_normal. Add a new function is_rela_normal to elf_backend_data, it can
    set part of relocations to rela_normal.

To maintain compatibility it'd better to merge this and cherry-pick to 18.x.

What do you think? @MaskRay

SixWeining

I'd accept this change and cherry-pick to 18.x to keep compatible with gcc/binutils. (18.1.4 will be released soon. We don't have too much time.)
For the generic improvement mentioned above, I think it can be done later.

SixWeining · 2024-04-16T13:20:31Z

/cherry-pick 01f7989

In LoongArch psABI v2.30, the R_LARCH_ALIGN requires symbol index to support the third parameter of alignment directive. Create symbol for each section is redundant because they have section symbol which can also be used as symbol index. So use section symbol directly for R_LARCH_ALIGN. (cherry picked from commit 01f7989)

llvmbot · 2024-04-16T13:25:36Z

/pull-request #88891

SixWeining · 2024-04-16T15:24:10Z

/cherry-pick 502ec2e 01f7989

In LoongArch psABI v2.30, the R_LARCH_ALIGN requires symbol index to support the third parameter of alignment directive. Create symbol for each section is redundant because they have section symbol which can also be used as symbol index. So use section symbol directly for R_LARCH_ALIGN. (cherry picked from commit 01f7989)

MaskRay · 2024-04-19T05:49:35Z

Sorry for the delay. LGTM. (I'd really appreciate that you gave me a bit more time and I might be able to suggest a better code comment and point out the grammatical mistake "not changes" in the test, which is probably not worth changing now).

This reverts commit 01f7989. This unusual special case has been discussed on the binutils mailing list. The approach will be revisited: https://sourceware.org/pipermail/binutils/2024-May/134092.html

This reverts commit 01f7989. This unusual special case has been discussed on the binutils mailing list. The approach will be revisited: https://sourceware.org/pipermail/binutils/2024-May/134092.html Pull Request: #92584

llvmbot added lld mc Machine (object) code lld:ELF backend:loongarch labels Mar 11, 2024

SixWeining requested review from MaskRay and SixWeining March 11, 2024 11:17

SixWeining reviewed Mar 11, 2024

View reviewed changes

xen0n mentioned this pull request Mar 11, 2024

Content suggestion for This Week in LoongArch newsletter / 《每周一龙》新闻线索信箱 loongson-community/areweloongyet#16

Open

MaskRay reviewed Mar 12, 2024

View reviewed changes

MQ-mengqing added 2 commits March 28, 2024 10:40

Add test description

3a40b2d

MQ-mengqing force-pushed the align-symbol branch from f1d9cab to 3a40b2d Compare March 28, 2024 07:52

SixWeining approved these changes Apr 15, 2024

View reviewed changes

SixWeining merged commit 01f7989 into llvm:main Apr 16, 2024
4 checks passed

SixWeining added this to the LLVM 18.X Release milestone Apr 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LoongArch] Use R_LARCH_ALIGN with section symbol #84741

[LoongArch] Use R_LARCH_ALIGN with section symbol #84741

MQ-mengqing commented Mar 11, 2024

llvmbot commented Mar 11, 2024 •

edited

MQ-mengqing commented Mar 11, 2024

SixWeining Mar 11, 2024

MaskRay Mar 12, 2024

MQ-mengqing Mar 28, 2024

MaskRay Mar 12, 2024

SixWeining Mar 18, 2024

SixWeining commented Apr 3, 2024

SixWeining left a comment

SixWeining commented Apr 16, 2024

llvmbot commented Apr 16, 2024

SixWeining commented Apr 16, 2024

MaskRay commented Apr 19, 2024

		@@ -226,11 +226,8 @@ bool LoongArchAsmBackend::shouldInsertFixupForCodeAlign(
		MCFixup::create(0, Dummy, MCFixupKind(LoongArch::fixup_loongarch_align));

[LoongArch] Use R_LARCH_ALIGN with section symbol #84741

[LoongArch] Use R_LARCH_ALIGN with section symbol #84741

Conversation

MQ-mengqing commented Mar 11, 2024

llvmbot commented Mar 11, 2024 • edited

MQ-mengqing commented Mar 11, 2024

SixWeining Mar 11, 2024

Choose a reason for hiding this comment

MaskRay Mar 12, 2024

Choose a reason for hiding this comment

MQ-mengqing Mar 28, 2024

Choose a reason for hiding this comment

MaskRay Mar 12, 2024

Choose a reason for hiding this comment

SixWeining Mar 18, 2024

Choose a reason for hiding this comment

SixWeining commented Apr 3, 2024

SixWeining left a comment

Choose a reason for hiding this comment

SixWeining commented Apr 16, 2024

llvmbot commented Apr 16, 2024

SixWeining commented Apr 16, 2024

MaskRay commented Apr 19, 2024

llvmbot commented Mar 11, 2024 •

edited