Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MIPS: .cpsetup broken for n32 #52785

Closed
chmeeedalf opened this issue Dec 18, 2021 · 5 comments · Fixed by #80534
Closed

MIPS: .cpsetup broken for n32 #52785

chmeeedalf opened this issue Dec 18, 2021 · 5 comments · Fixed by #80534

Comments

@chmeeedalf
Copy link
Contributor

MIPS n32 ABI requires '.cpsetup , <off | reg>, ' to generate the following sequence:

sd $gp, off ($sp)
lui $gp, label    // top half
addiu $gp, label  // bottom half
addu $gp, $gp, r

The MIPS code generator currently does not include the final addu, leading to crashes in n32 ABI position independent code.

@chmeeedalf
Copy link
Contributor Author

Looking at it more, n32 is supposed to use whatever label, not fixed at __gnu_local_gp

@atanasyan
Copy link
Collaborator

Could you check that this patch fixes the issue? The patch adds the addu instruction only. __gnu_local_gp is a separate problem.
pr52785.patch.txt

@rasul247
Copy link

rasul247 commented Dec 18, 2021 via email

@chmeeedalf
Copy link
Contributor Author

Unfortunately that's incomplete. It's adding the "absolute" address of __gnu_local_gp, which won't work to get a relative address. It really should be doing what is done for N64 below in the function, from what I can tell.

@wzssyqa
Copy link
Contributor

wzssyqa commented Feb 3, 2024

It's due to a typo in MipsTargetELFStreamer::emitDirectiveCpsetup
This patch should work:

-  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

wzssyqa added a commit to wzssyqa/llvm-project that referenced this issue Feb 3, 2024
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
wzssyqa added a commit to wzssyqa/llvm-project that referenced this issue Feb 3, 2024
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
wzssyqa added a commit to wzssyqa/llvm-project that referenced this issue Feb 22, 2024
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 pushed a commit that referenced this issue Feb 26, 2024
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: #52785
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Feb 27, 2024
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 pushed a commit to llvmbot/llvm-project that referenced this issue Mar 11, 2024
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants