Page MenuHomePhabricator

[X86] Call locally defined function directly for PIE
Needs ReviewPublic

Authored by hjl.tools on Oct 6 2015, 1:07 PM.

Details

Summary

There is no need to use PLT to call locally defined function for PIE
since locally defined function in PIE can't be preempted.

Fixes PR 24970.

Diff Detail

Repository
rL LLVM

Event Timeline

hjl.tools updated this revision to Diff 36648.Oct 6 2015, 1:07 PM
hjl.tools retitled this revision from to [X86] Call locally defined function directly for PIE .
hjl.tools updated this object.
hjl.tools set the repository for this revision to rL LLVM.
hjl.tools added a subscriber: llvm-commits.
DavidKreitzer added inline comments.Oct 8 2015, 9:34 AM
test/CodeGen/X86/pie-2.ll
1 ↗(On Diff #36648)

This test is almost identical to pie-1.ll. Can you combine them?

I tried putting

; LINUX-NOT: call{{l|q}} foo@PLT
; LINUX: call{{l|q}} foo

in the same file. But test passed even if llvm wasn't fixed. It seems that

; LINUX: call{{l|q}} foo

matches

call{{l|q}} foo@PLT

and cancels

; LINUX-NOT: call{{l|q}} foo@PLT

DavidKreitzer edited edge metadata.Oct 9 2015, 6:19 AM

How about something like this?

; LINUX: call{{l|q}} foo{{ |$}}

There are other alternatives as well. Just about anything would be better than replicating the entire test.

hjl.tools updated this revision to Diff 36958.Oct 9 2015, 9:28 AM
hjl.tools edited edge metadata.
hjl.tools removed rL LLVM as the repository for this revision.

Update to a single testcase with

; LINUX: call{{l|q}} foo{{$}}

hjl.tools set the repository for this revision to rL LLVM.Oct 9 2015, 9:29 AM
rafael added inline comments.
lib/Target/X86/X86ISelLowering.cpp
3314

This needs to be refactored to a helper function that is used in both places.

Should this really be isStrongDefinitionForLinker? If this is a weak_odr or linkonce_odr, can't we avoid the plt? Add a test for that one way or the other.

hjl.tools updated this revision to Diff 38729.Oct 29 2015, 6:24 AM

I added a helper function, ClassifyGlobalFunctionReference, and used
it in both places. I was looking for a predicate to tell me if a symbol is
defined. The symbol can be weak or linkonce. Should I use
!isDeclarationForLinker instead?

This seems to work:

diff --git a/lib/Target/X86/X86Subtarget.cpp b/lib/Target/X86/X86Subtarget.cpp
index d94e91b..34f88b9 100644
--- a/lib/Target/X86/X86Subtarget.cpp
+++ b/lib/Target/X86/X86Subtarget.cpp
@@ -154,8 +154,8 @@ ClassifyGlobalFunctionReference(const GlobalValue *GV,
   // we don't need to use the PLT - we can directly call it.
   if (isTargetELF() &&
       TM.getRelocationModel() == Reloc::PIC_ &&
-      !(TM.Options.PositionIndependentExecutable &&
-        GV->isStrongDefinitionForLinker()) &&
+      (!TM.Options.PositionIndependentExecutable ||
+       GV->isDeclarationForLinker()) &&
       GV->hasDefaultVisibility() && !GV->hasLocalLinkage()) {
     return X86II::MO_PLT;
   } else if (isPICStyleStubAny() &&

This is still missing a testcase for other linkages (weak_odr and weak at least).

Why do we need to use a PLT for calling those?

Even for regular weak and linkonce, why do we need a plt? We know there is *a* definition in the executable. The static linker might resolve it to a definition in some other .o, but it will still be defined in the executable and cannot be preempted out of it.

lib/Target/X86/X86Subtarget.cpp
147

Don't duplicate comments from the .h file.

lib/Target/X86/X86Subtarget.h
504

Don't repeat the name in the comment

506

Function names should start with a lowercase letter.

test/CodeGen/X86/pie.ll
1

Drop the -check-prefix=LINUX and just use the default (CHECK)

10

You don't need the comments.

Simplify foo to return null

define void @foo() {

ret void

}

dexonsmith resigned from this revision.Oct 6 2020, 3:44 PM