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

[PowerPC] Support PIC Secure PLT for CALL_RM #121281

Merged

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Dec 29, 2024

https://reviews.llvm.org/D111433 introduced PPCISD::CALL_RM for
-frounding-math. -msecure-plt -frounding-math {-fpic,-fPIC} codegen for
PPC32 became incorrect when a function contains function calls but no
global variable references (GlobalBaseReg).

As reported by @q66 , musl/src/dirent/closedir.c implements such a
function, which is miscompiled.

PPCISD::CALL has custom logic to set up the base register
(https://reviews.llvm.org/D42112). Add an extra case for CALL_RM.

While here, improve the test to

  • actually test case PPCISD::CALL: we need a non-leaf function that
    doesn't access global variables (global variables lead to
    GlobalBaseReg, which call getGlobalBaseReg() as well).
  • test ExternalSymbolSDNode with a memset.

Supersedes: #72758

Created using spr 1.3.5-bogner
@llvmbot
Copy link
Member

llvmbot commented Dec 29, 2024

@llvm/pr-subscribers-backend-powerpc

Author: Fangrui Song (MaskRay)

Changes

https://reviews.llvm.org/D111433 introduced PPCISD::CALL_RM for
-frounding-math. -msecure-plt -frounding-math {-fpic,-fPIC} codegen for
PPC32 became incorrect when a function contains function calls but no
global variable references (GlobalBaseReg).

As reported by @q66 , musl/src/dirent/closedir.c implements such a
function, which is miscompiled.

PPCISD::CALL has custom logic to set up the base register
(https://reviews.llvm.org/D42112). Add an extra case for CALL_RM.

While here, improve the test to

  • actually test case PPCISD::CALL: we need a non-leaaf function that
    doesn't access global variables (global variables lead to
    GlobalBaseReg, which call getGlobalBaseReg() as well).
  • test ExternalSymbolSDNode with a memset.

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

2 Files Affected:

  • (modified) llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp (+5-6)
  • (modified) llvm/test/CodeGen/PowerPC/ppc32-pic-large.ll (+44)
diff --git a/llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp b/llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
index 2e0ee59d901b82..d1daf7cd3d0d83 100644
--- a/llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
+++ b/llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
@@ -5473,10 +5473,10 @@ void PPCDAGToDAGISel::Select(SDNode *N) {
     // generate secure plt code for TLS symbols.
     getGlobalBaseReg();
   } break;
-  case PPCISD::CALL: {
-    if (PPCLowering->getPointerTy(CurDAG->getDataLayout()) != MVT::i32 ||
-        !TM.isPositionIndependent() || !Subtarget->isSecurePlt() ||
-        !Subtarget->isTargetELF())
+  case PPCISD::CALL:
+  case PPCISD::CALL_RM: {
+    if (Subtarget->isPPC64() || !TM.isPositionIndependent() ||
+        !Subtarget->isSecurePlt() || !Subtarget->isTargetELF())
       break;
 
     SDValue Op = N->getOperand(1);
@@ -5489,8 +5489,7 @@ void PPCDAGToDAGISel::Select(SDNode *N) {
       if (ES->getTargetFlags() == PPCII::MO_PLT)
         getGlobalBaseReg();
     }
-  }
-    break;
+  } break;
 
   case PPCISD::GlobalBaseReg:
     ReplaceNode(N, getGlobalBaseReg());
diff --git a/llvm/test/CodeGen/PowerPC/ppc32-pic-large.ll b/llvm/test/CodeGen/PowerPC/ppc32-pic-large.ll
index 025a5ad787fbe0..2f0b92964c13b2 100644
--- a/llvm/test/CodeGen/PowerPC/ppc32-pic-large.ll
+++ b/llvm/test/CodeGen/PowerPC/ppc32-pic-large.ll
@@ -13,6 +13,8 @@ $bar1 = comdat any
 @bar2 = global i32 0, align 4, comdat($bar1)
 
 declare i32 @call_foo(i32, ...)
+declare i32 @call_strictfp() strictfp
+declare void @llvm.memset.p0.i64(ptr nocapture writeonly, i8, i64, i1 immarg)
 
 define i32 @foo() {
 entry:
@@ -21,6 +23,23 @@ entry:
   ret i32 %0
 }
 
+define i32 @foo1() strictfp {
+entry:
+  %call = call i32 (i32, ...) @call_foo(i32 0)
+  ret i32 %call
+}
+
+define i32 @foo1_strictfp() strictfp {
+entry:
+  %call = call i32 () @call_strictfp()
+  ret i32 %call
+}
+
+define void @foo2(ptr %a) {
+  call void @llvm.memset.p0.i64(ptr align 1 %a, i8 1, i64 1000, i1 false)
+  ret void
+}
+
 define i32 @load() {
 entry:
   %0 = load i32, ptr @bar1
@@ -49,6 +68,31 @@ entry:
 ; LARGE-SECUREPLT:   addi 30, 30, .LTOC-.L0$pb@l
 ; LARGE-SECUREPLT:   bl call_foo@PLT+32768
 
+; LARGE-SECUREPLT-LABEL: foo1:
+; LARGE-SECUREPLT:       .L1$pb:
+; LARGE-SECUREPLT-NEXT:    crxor 6, 6, 6
+; LARGE-SECUREPLT-NEXT:    mflr 30
+; LARGE-SECUREPLT-NEXT:    addis 30, 30, .LTOC-.L1$pb@ha
+; LARGE-SECUREPLT-NEXT:    addi 30, 30, .LTOC-.L1$pb@l
+; LARGE-SECUREPLT-NEXT:    li 3, 0
+; LARGE-SECUREPLT-NEXT:    bl call_foo@PLT+32768
+
+; LARGE-SECUREPLT-LABEL: foo1_strictfp:
+; LARGE-SECUREPLT:       .L2$pb:
+; LARGE-SECUREPLT-NEXT:    mflr 30
+; LARGE-SECUREPLT-NEXT:    addis 30, 30, .LTOC-.L2$pb@ha
+; LARGE-SECUREPLT-NEXT:    addi 30, 30, .LTOC-.L2$pb@l
+; LARGE-SECUREPLT-NEXT:    bl call_strictfp@PLT+32768
+
+; LARGE-SECUREPLT-LABEL: foo2:
+; LARGE-SECUREPLT:       .L3$pb:
+; LARGE-SECUREPLT:         mflr 30
+; LARGE-SECUREPLT-NEXT:    addis 30, 30, .LTOC-.L3$pb@ha
+; LARGE-SECUREPLT-NEXT:    addi 30, 30, .LTOC-.L3$pb@l
+; LARGE-SECUREPLT:         bl memset@PLT+32768
+
+; LARGE-SECUREPLT-LABEEL: load:
+
 ; LARGE:      .section .bss.bar1,"awG",@nobits,bar1,comdat
 ; LARGE:      bar1:
 ; LARGE:      .section .bss.bar2,"awG",@nobits,bar1,comdat

@brad0
Copy link
Contributor

brad0 commented Dec 29, 2024

#72758

There was also a PR for the same fix. But for some reason George stopped responding.

@MaskRay
Copy link
Member Author

MaskRay commented Dec 29, 2024

#72758

There was also a PR for the same fix. But for some reason George stopped responding.

Thanks for bringing up the old thread. I did not know similar code change was brought up before. I do prefer the tests in this patch, tho...

@brad0
Copy link
Contributor

brad0 commented Dec 30, 2024

#72758
There was also a PR for the same fix. But for some reason George stopped responding.

Thanks for bringing up the old thread. I did not know similar code change was brought up before. I do prefer the tests in this patch, tho...

I just mean I think you can add a closes tag or whatever as this supersedes that PR.

@MaskRay
Copy link
Member Author

MaskRay commented Dec 30, 2024

#72758
There was also a PR for the same fix. But for some reason George stopped responding.

Thanks for bringing up the old thread. I did not know similar code change was brought up before. I do prefer the tests in this patch, tho...

I just mean I think you can add a closes tag or whatever as this supersedes that PR.

Changed the PR description:) Thanks

@brad0 brad0 requested a review from chenzheng1030 December 30, 2024 11:33
@MaskRay
Copy link
Member Author

MaskRay commented Jan 4, 2025

Ping:)

Copy link
Contributor

@lei137 lei137 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Thank-you for the fix.

@MaskRay MaskRay merged commit 5e0be96 into main Jan 6, 2025
10 checks passed
@MaskRay MaskRay deleted the users/MaskRay/spr/powerpc-support-pic-secure-plt-for-call_rm branch January 6, 2025 16:59
github-actions bot pushed a commit to arm/arm-toolchain that referenced this pull request Jan 10, 2025
https://reviews.llvm.org/D111433 introduced PPCISD::CALL_RM for
-frounding-math. -msecure-plt -frounding-math {-fpic,-fPIC} codegen for
PPC32 became incorrect when a function contains function calls but no
global variable references (GlobalBaseReg).

As reported by @q66 , musl/src/dirent/closedir.c implements such a
function, which is miscompiled.

PPCISD::CALL has custom logic to set up the base register
(https://reviews.llvm.org/D42112). Add an extra case for CALL_RM.

While here, improve the test to

* actually test `case PPCISD::CALL`: we need a non-leaf function that
  doesn't access global variables (global variables lead to
  GlobalBaseReg, which call `getGlobalBaseReg()` as well).
* test `ExternalSymbolSDNode` with a memset.

Supersedes: #72758

Pull Request: llvm/llvm-project#121281
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants