Skip to content

Conversation

@kernigh
Copy link
Contributor

@kernigh kernigh commented Nov 18, 2023

If a function call has the strictfp attribute, its opcode changes from CALL to CALL_RM. If the call uses the secure PLT for 32-bit ELF, then it must getGlobalBaseReg() to set r30.


This fixes a bug that I described in (OpenBSD) macppc clang-16 -ftrapping-math crashes Xorg. The short version is that clang -ftrapping-math broke code like int main(void) { time(NULL); } by forgetting to set r30 before using the secure PLT.

@github-actions
Copy link

github-actions bot commented Nov 18, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

If a function call has the strictfp attribute, its opcode changes from
CALL to CALL_RM.  If the call uses the secure PLT for 32-bit ELF, then
it must getGlobalBaseReg() to set r30.
bob-beck pushed a commit to openbsd/src that referenced this pull request Nov 19, 2023
Handle CALL_RM like CALL for 32-bit ELF.  If a function call has the
strictfp attribute, its opcode changes from CALL to CALL_RM.  If a
call uses the secure PLT, then it must getGlobalBaseReg() to set r30.

After I rebuilt xenocara/lib/pixman with this change, Xorg stopped
crashing on my macppc.  pixman uses cc -ftrapping-math which puts
strictfp on each function call.

llvm/llvm-project#72758

ok jca@ tobhe@ deraadt@
@brad0
Copy link
Contributor

brad0 commented Nov 25, 2023

Ping.

@brad0
Copy link
Contributor

brad0 commented Dec 2, 2023

Ping.

@@ -0,0 +1,21 @@
; RUN: llc < %s -mtriple=powerpc-unknown-linux-gnu -mattr=+secure-plt -relocation-model=pic | FileCheck %s
Copy link
Member

Choose a reason for hiding this comment

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

Can we use auto generate script for it?

@brad0
Copy link
Contributor

brad0 commented Dec 8, 2023

@kernigh Ping.

2 similar comments
@brad0
Copy link
Contributor

brad0 commented Dec 20, 2023

@kernigh Ping.

@brad0
Copy link
Contributor

brad0 commented Jan 10, 2024

@kernigh Ping.

} break;
case PPCISD::CALL: {
case PPCISD::CALL:
case PPCISD::CALL_RM: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are your targeting this for SPE? SPE should have no floating point instructions? So round mode should not be a concern? For SPE, we may stop generate CALL_RM where it is generated. Note that CALL_RM performs worse than normal CALL even after instruction selection. They are selected to different instructions.

If for other PPC32 targets, the fix looks reasonable to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are your targeting this for SPE?

This is not for SPE.

Copy link
Collaborator

@chenzheng1030 chenzheng1030 left a comment

Choose a reason for hiding this comment

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

LGTM as long as @ecnelises's comment about the LIT case is addressed.

mordak pushed a commit to mordak/llvm-project that referenced this pull request Feb 11, 2024
Handle CALL_RM like CALL for 32-bit ELF.  If a function call has the
strictfp attribute, its opcode changes from CALL to CALL_RM.  If a
call uses the secure PLT, then it must getGlobalBaseReg() to set r30.

After I rebuilt xenocara/lib/pixman with this change, Xorg stopped
crashing on my macppc.  pixman uses cc -ftrapping-math which puts
strictfp on each function call.

llvm#72758

ok jca@ tobhe@ deraadt@
@brad0
Copy link
Contributor

brad0 commented Mar 3, 2024

It would be nice to get this in.

@chenzheng1030
Copy link
Collaborator

It would be nice to get this in.

Please let us know if need us to commit this pr. @kernigh

@brad0
Copy link
Contributor

brad0 commented Mar 4, 2024

Please let us know if need us to commit this pr. @kernigh

He does, but there was the issue of the test case being updated.

MaskRay added a commit that referenced this pull request Jan 6, 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: #121281
@brad0 brad0 closed this Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants