MIPS: fix emitDirectiveCpsetup on N32 #80534

wzssyqa · 2024-02-03T09:06:03Z

In the current code, we don't support -mno-shared yet, and for N32, .cpsetup is expand as the style -f -mno-shared.

It will generate bad code for PIC binaries like:

00000000 <t1>:
   0:   ffbc0008        sd      gp,8(sp)
   4:   3c1c0000        lui     gp,0x0
                        4: R_MIPS_HI16  __gnu_local_gp
   8:   279c0000        addiu   gp,gp,0
                        8: R_MIPS_LO16  __gnu_local_gp

In fact which style of .cpsetup is used should be determined by -m(no-)shared option instead of -mabi=n32 option.

Fixes: #52785

llvmbot · 2024-02-03T09:06:31Z

@llvm/pr-subscribers-mc

Author: YunQiang Su (wzssyqa)

Changes

In the current code, we don't support -mno-shared yet, and for N32, .cpsetup is expand as the style -f -mno-shared.

It will generate bad code for PIC binaries like:
00000000 <t1>:
0: ffbc0008 sd gp,8(sp)
4: 3c1c0000 lui gp,0x0
4: R_MIPS_HI16 __gnu_local_gp
8: 279c0000 addiu gp,gp,0
8: R_MIPS_LO16 __gnu_local_gp

In fact which style of .cpsetup is used should be determined by -m(no-)shared option instead of -mabi=n32 option.

Fixes: #52785

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

2 Files Affected:

(modified) llvm/lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp (+9-3)
(modified) llvm/test/MC/Mips/cpsetup.s (+15-22)

diff --git a/llvm/lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp b/llvm/lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp
index 27d7f0f261d10..adfcea7361583 100644
--- a/llvm/lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp
+++ b/llvm/lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp
@@ -1255,7 +1255,9 @@ void MipsTargetELFStreamer::emitDirectiveCpsetup(unsigned RegNo,
     emitRRI(Mips::SD, GPReg, Mips::SP, RegOrOffset, SMLoc(), &STI);
   }
 
-  if (getABI().IsN32()) {
+#if 0
+  // We haven't support -mabicalls -mno-shared yet.
+  if (-mno-shared) {
     MCSymbol *GPSym = MCA.getContext().getOrCreateSymbol("__gnu_local_gp");
     const MipsMCExpr *HiExpr = MipsMCExpr::create(
         MipsMCExpr::MEK_HI, MCSymbolRefExpr::create(GPSym, MCA.getContext()),
@@ -1273,6 +1275,7 @@ void MipsTargetELFStreamer::emitDirectiveCpsetup(unsigned RegNo,
 
     return;
   }
+#endif
 
   const MipsMCExpr *HiExpr = MipsMCExpr::createGpOff(
       MipsMCExpr::MEK_HI, MCSymbolRefExpr::create(&Sym, MCA.getContext()),
@@ -1288,8 +1291,11 @@ void MipsTargetELFStreamer::emitDirectiveCpsetup(unsigned RegNo,
   emitRRX(Mips::ADDiu, GPReg, GPReg, MCOperand::createExpr(LoExpr), SMLoc(),
           &STI);
 
-  // daddu  $gp, $gp, $funcreg
-  emitRRR(Mips::DADDu, GPReg, GPReg, RegNo, SMLoc(), &STI);
+  // (d)addu  $gp, $gp, $funcreg
+  if (getABI().IsN32())
+    emitRRR(Mips::ADDu, GPReg, GPReg, RegNo, SMLoc(), &STI);
+  else
+    emitRRR(Mips::DADDu, GPReg, GPReg, RegNo, SMLoc(), &STI);
 }
 
 void MipsTargetELFStreamer::emitDirectiveCpreturn(unsigned SaveLocation,
diff --git a/llvm/test/MC/Mips/cpsetup.s b/llvm/test/MC/Mips/cpsetup.s
index 8e587aea3e7e6..b24aaac29154a 100644
--- a/llvm/test/MC/Mips/cpsetup.s
+++ b/llvm/test/MC/Mips/cpsetup.s
@@ -4,8 +4,6 @@
 # RUN: llvm-mc -triple mips-unknown-linux -target-abi o32 %s | \
 # RUN:   FileCheck -check-prefixes=ASM,ASM-O32 %s
 
-# FIXME: Now we check .cpsetup expansion for `-mno-shared` case only.
-#        We also need to implement/check the `-mshared` case.
 # RUN: llvm-mc -triple mips64-unknown-linux -target-abi n32 -filetype=obj -o - %s | \
 # RUN:   llvm-objdump --no-print-imm-hex -d -r -z - | \
 # RUN:   FileCheck -check-prefixes=ALL,NXX,N32 %s
@@ -35,11 +33,10 @@ t1:
 
 # NXX-NEXT: sd       $gp, 8($sp)
 # NXX-NEXT: lui      $gp, 0
-# N32-NEXT: R_MIPS_HI16 __gnu_local_gp
-# N64-NEXT: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_HI16  __cerror
+# NXX-NEXT: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_HI16  __cerror
 # NXX-NEXT: addiu    $gp, $gp, 0
-# N32-NEXT: R_MIPS_LO16 __gnu_local_gp
-# N64-NEXT: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_LO16  __cerror
+# NXX-NEXT: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_LO16  __cerror
+# N32-NEXT: addu     $gp, $gp, $25
 # N64-NEXT: daddu    $gp, $gp, $25
 
 # ASM-NEXT: .cpsetup $25, 8, __cerror
@@ -64,11 +61,10 @@ t2:
 
 # NXX-NEXT: move     $2, $gp
 # NXX-NEXT: lui      $gp, 0
-# N32-NEXT: R_MIPS_HI16 __gnu_local_gp
-# N64-NEXT: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_HI16  __cerror
+# NXX-NEXT: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_HI16  __cerror
 # NXX-NEXT: addiu    $gp, $gp, 0
-# N32-NEXT: R_MIPS_LO16 __gnu_local_gp
-# N64-NEXT: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_LO16  __cerror
+# NXX-NEXT: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_LO16  __cerror
+# N32-NEXT: addu     $gp, $gp, $25
 # N64-NEXT: daddu    $gp, $gp, $25
 
 # ASM-NEXT: .cpsetup $25, $2, __cerror
@@ -101,11 +97,10 @@ t3:
 
 # NXX-NEXT: move     $2, $gp
 # NXX-NEXT: lui      $gp, 0
-# N32-NEXT: {{^ *0+}}38: R_MIPS_HI16 __gnu_local_gp
-# N64-NEXT: {{^ *0+}}40: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_HI16 .text
+# NXX-NEXT: {{^ *0+}}40: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_HI16 .text
 # NXX-NEXT: addiu    $gp, $gp, 0
-# N32-NEXT: {{^ *0+}}3c: R_MIPS_LO16 __gnu_local_gp
-# N64-NEXT: {{^ *0+}}44: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_LO16 .text
+# NXX-NEXT: {{^ *0+}}44: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_LO16 .text
+# N32-NEXT: addu     $gp, $gp, $25
 # N64-NEXT: daddu    $gp, $gp, $25
 # NXX-NEXT: nop
 # NXX-NEXT: sub $3, $3, $2
@@ -158,11 +153,10 @@ t5:
 
 # NXX-NEXT: sd       $gp, 8($sp)
 # NXX-NEXT: lui      $gp, 0
-# N32-NEXT: R_MIPS_HI16 __gnu_local_gp
-# N64-NEXT: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_HI16  __cerror
+# NXX-NEXT: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_HI16  __cerror
 # NXX-NEXT: addiu    $gp, $gp, 0
-# N32-NEXT: R_MIPS_LO16 __gnu_local_gp
-# N64-NEXT: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_LO16  __cerror
+# NXX-NEXT: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_LO16  __cerror
+# N64-NEXT: addu     $gp, $gp, $25
 # N64-NEXT: daddu    $gp, $gp, $25
 
 # ASM-NEXT: .cpsetup $25, 8, __cerror
@@ -184,11 +178,10 @@ IMM_8 = 8
 
 # NXX-NEXT: sd       $gp, 8($sp)
 # NXX-NEXT: lui      $gp, 0
-# N32-NEXT: R_MIPS_HI16 __gnu_local_gp
-# N64-NEXT: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_HI16  __cerror
+# NXX-NEXT: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_HI16  __cerror
 # NXX-NEXT: addiu    $gp, $gp, 0
-# N32-NEXT: R_MIPS_LO16 __gnu_local_gp
-# N64-NEXT: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_LO16  __cerror
+# NXX-NEXT: R_MIPS_GPREL16/R_MIPS_SUB/R_MIPS_LO16  __cerror
+# N32-NEXT: addu     $gp, $gp, $25
 # N64-NEXT: daddu    $gp, $gp, $25
 
 # ASM-NEXT: .cpsetup $25, 8, __cerror

wzssyqa · 2024-02-06T01:19:42Z

@MaskRay

In the current code, we don't support -mno-shared yet, and for N32, .cpsetup is expand as the style -f -mno-shared. It will generate bad code for PIC binaries like: 00000000 <t1>: 0: ffbc0008 sd gp,8(sp) 4: 3c1c0000 lui gp,0x0 4: R_MIPS_HI16 __gnu_local_gp 8: 279c0000 addiu gp,gp,0 8: R_MIPS_LO16 __gnu_local_gp In fact which style of .cpsetup is used should be determined by -m(no-)shared option instead of -mabi=n32 option. Fixes: llvm#52785

MaskRay · 2024-02-24T08:53:45Z

llvm/lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp

-  emitRRR(Mips::DADDu, GPReg, GPReg, RegNo, SMLoc(), &STI);
+  // (d)addu  $gp, $gp, $funcreg
+  if (getABI().IsN32())
+    emitRRR(Mips::ADDu, GPReg, GPReg, RegNo, SMLoc(), &STI);


Confirmed that this is correct.

MaskRay · 2024-02-24T08:54:17Z

llvm/lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp

-  if (getABI().IsN32()) {
+#if 0
+  // We haven't support -mabicalls -mno-shared yet.
+  if (-mno-shared) {


IIUC -mno-shared only makes sense for -fno-pic. Modern executables use PIE and cannot use -mno-shared.

MaskRay · 2024-02-24T09:03:13Z

#if 0

is not recommended. Just remove the code.

Please clarify the comment. Consider the following:

In gas, .cpsetup may expand to one of two code sequences (one is related to __gnu_local_gp), depending on -mno-shared and -msym32.
Since Clang doesn't support -mno-shared or -msym32, .cpsetup expands to one code sequence.
The N32 condition incorrectly leads to the incorrect __gnu_local_gp code sequence.

In gas, .cpsetup may expand to one of two code sequences (one is related to `__gnu_local_gp`), depending on -mno-shared and -msym32. Since Clang doesn't support -mno-shared or -msym32, .cpsetup expands to one code sequence. The N32 condition incorrectly leads to the incorrect `__gnu_local_gp` code sequence. ``` 00000000 <t1>: 0: ffbc0008 sd gp,8(sp) 4: 3c1c0000 lui gp,0x0 4: R_MIPS_HI16 __gnu_local_gp 8: 279c0000 addiu gp,gp,0 8: R_MIPS_LO16 __gnu_local_gp ``` Fixes: llvm#52785 (cherry picked from commit 860b6ed)

llvmbot added the mc Machine (object) code label Feb 3, 2024

wzssyqa force-pushed the n32-cpsetup branch from 46bc99d to f71a0e8 Compare February 3, 2024 13:13

wzssyqa force-pushed the n32-cpsetup branch from 7ce1c7f to 0e9ab65 Compare February 22, 2024 02:35

wzssyqa requested a review from MaskRay February 24, 2024 07:11

MaskRay reviewed Feb 24, 2024

View reviewed changes

MaskRay approved these changes Feb 24, 2024

View reviewed changes

MaskRay merged commit 860b6ed into llvm:main Feb 26, 2024
4 checks passed

brad0 mentioned this pull request Feb 27, 2024

Merge 860b6edfa9b344fbf8c500c17158c8212ea87d1c into 18.x #83197

Closed

pointhex mentioned this pull request May 7, 2024

getStyleDiagHandler #91314

Closed

aemerson mentioned this pull request May 9, 2024

release/18.x: [AArc64][GlobalISel] Fix legalizer assert for G_INSERT_VECTOR_ELT - manual merge #91672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MIPS: fix emitDirectiveCpsetup on N32 #80534

MIPS: fix emitDirectiveCpsetup on N32 #80534

wzssyqa commented Feb 3, 2024 •

edited

Loading

llvmbot commented Feb 3, 2024

wzssyqa commented Feb 6, 2024

MaskRay Feb 24, 2024

MaskRay Feb 24, 2024

MaskRay commented Feb 24, 2024

MIPS: fix emitDirectiveCpsetup on N32 #80534

MIPS: fix emitDirectiveCpsetup on N32 #80534

Conversation

wzssyqa commented Feb 3, 2024 • edited Loading

llvmbot commented Feb 3, 2024

wzssyqa commented Feb 6, 2024

MaskRay Feb 24, 2024

Choose a reason for hiding this comment

MaskRay Feb 24, 2024

Choose a reason for hiding this comment

MaskRay commented Feb 24, 2024

wzssyqa commented Feb 3, 2024 •

edited

Loading