Page MenuHomePhabricator

[ELF] - -pie/--pic-executable option implemented
ClosedPublic

Authored by grimar on Mar 15 2016, 6:48 AM.

Details

Summary

-pie
--pic-executable

Create a position independent executable.  This is currently only
 supported on ELF platforms.  Position independent executables are
 similar to shared libraries in that they are relocated by the
 dynamic linker to the virtual address the OS chooses for them
 (which can vary between invocations).  Like normal dynamically
 linked executables they can be executed and symbols defined in the
 executable cannot be overridden by shared libraries.

This fixes the https://llvm.org/bugs/show_bug.cgi?id=26923

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 50723.Mar 15 2016, 6:48 AM
grimar retitled this revision from to [ELF] - -pie/--pic-executable option implemented.
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
emaste added a subscriber: emaste.Mar 15 2016, 10:52 AM
ruiu added inline comments.Mar 15 2016, 11:23 AM
ELF/Driver.cpp
150–151 ↗(On Diff #50723)

Why is -pie incompatible with -static?

ELF/SymbolTable.cpp
21 ↗(On Diff #50723)

isPIO is a single line function. I don't think including the entire header file just for that function is a good idea.

127 ↗(On Diff #50723)

What does PIO stand for?

Anyways, I think this function is probably a bit too much. How about this: add Config->Pic and set it to true if either Config->Pie or Config->Shared.

grimar updated this revision to Diff 50808.Mar 16 2016, 2:47 AM
  • Addressed review comments.
grimar added inline comments.Mar 16 2016, 2:49 AM
ELF/Driver.cpp
150–151 ↗(On Diff #50723)

It was done in bfd/gold to be consistent with gcc driver:
https://gcc.gnu.org/ml/gcc/2014-01/msg00125.html
My motivation was to be consistent with gold here.

ELF/SymbolTable.cpp
21 ↗(On Diff #50723)

I can't agree here, because using of declarations instead of including header in cpp files does not look clean for me.
Fortunately we don't need that after introducing Config->Pic anymore.

127 ↗(On Diff #50723)

It was Position Independent Output. I saw that term in gold sources.
Applied your suggestion about Config->Pic, that should work.

grimar updated this revision to Diff 50810.Mar 16 2016, 2:52 AM
  • Removed excessive include file
ruiu added inline comments.Mar 16 2016, 6:47 AM
ELF/Config.h
71 ↗(On Diff #50810)

Remove " = false".

ELF/Driver.cpp
150–151 ↗(On Diff #50810)

I don't see any reason to reject that, so I believe they are simply wrong. And I don't see a reason to copy a wrong behavior.

ELF/SymbolTable.cpp
120–121 ↗(On Diff #50810)

Can you revert this change?

ELF/Target.cpp
339 ↗(On Diff #50810)

I like these renamings from Shared to Pic as it conveys more accurate meaning.

grimar updated this revision to Diff 50821.Mar 16 2016, 7:17 AM
  • Addressed review comments
ruiu added inline comments.Mar 16 2016, 7:28 AM
ELF/Driver.cpp
148–149 ↗(On Diff #50821)

I think this is also something that we are checking for errors too eagerly. -shared implies -pie, so they are not incompatible. It is better to ignore this combination.

150–151 ↗(On Diff #50821)

Please remove this code unless you have a valid reason to do so.

grimar added inline comments.Mar 16 2016, 9:00 AM
ELF/Config.h
71 ↗(On Diff #50810)

Done.

ELF/Driver.cpp
150–151 ↗(On Diff #50810)

This patch does not support this.
I did not investigate where lld needs change to support that. Let me leave it as is now ? If someone needs that - it would be his responsibility to tweak lld here.

ELF/SymbolTable.cpp
120–121 ↗(On Diff #50810)

Done.

ELF/Target.cpp
339 ↗(On Diff #50810)

true.

grimar added inline comments.Mar 16 2016, 9:03 AM
ELF/Driver.cpp
148–149 ↗(On Diff #50821)

-shared implies pic, but not -pie. As pie is Position Independent Executable. So I can't agree here. Gold do the same check.

ruiu added inline comments.Mar 16 2016, 9:09 AM
ELF/Driver.cpp
150–151 ↗(On Diff #50821)

Adding code which you cannot justify is a bad idea. "Leaving it as is" means not adding that code because that code didn't exist before. Add this piece of code when you find you actually need it.

grimar updated this revision to Diff 50833.Mar 16 2016, 9:54 AM
  • Addressed review comments
ELF/Driver.cpp
150–151 ↗(On Diff #50821)

I removed it just because you insisted. But I can justify that. You`ll never get this using gcc and that is enough IMO to not support that atm.
You often say something like "lets not support that until we need". So why we enable support here for something that even gold does not do ?

?>I respect compatibility as long as it matters and makes sense. In this case, we don't have a good explanation on why this combination of option is banned by gold/bfd. Adding that >error check would increase the complexity of the linker, and it copies the artificial limitation that we can't explain. I don't want to say when someone asked me why this is banned >"because gold did this, so I don't know."

Ok, I respect and understand your point here.

George.

grimar added inline comments.Mar 16 2016, 1:35 PM
ELF/Options.td
102 ↗(On Diff #50833)

btw, not about this patch, but in general.
If we have errors/warnings in lowercase. Do we want HelpText here to be lowercase as well ? I guess yes.

ruiu added inline comments.Mar 16 2016, 1:48 PM
ELF/Options.td
102 ↗(On Diff #50833)

I don't know which is better, but this is where we just want to follow the convention. What do other command do for --help?

ruiu edited edge metadata.Mar 16 2016, 1:51 PM

LGTM

test/ELF/undef.s
3–4 ↗(On Diff #50833)

Move CHECK after the following RUN. (Basically, we want to keep the source code not being "patchy". New code should look as if it were there from beginning. I don't think you wrote this way if you have written these two RUN lines)

grimar added inline comments.Mar 16 2016, 2:01 PM
ELF/Options.td
102 ↗(On Diff #50833)

Hmm. clang, ld and gold has uppercase for help text. So seems no need to change anything here.

This revision was automatically updated to reflect the committed changes.
grimar added inline comments.Mar 16 2016, 11:05 PM
test/ELF/undef.s
3–4 ↗(On Diff #50833)

Done.
But I think we sometimes re-use the previously declared CHECKs when adding new RUN lines in tests. Actually If I would wrote that from beginning it would be:

# REQUIRES: x86
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t

# RUN: not ld.lld %t -o %t2 2>&1 | FileCheck %s
# CHECK: undefined symbol: bar in {{.*}}
# CHECK: undefined symbol: foo in {{.*}}

# RUN: not ld.lld -pie %t -o %t2 2>&1 | FileCheck %s

ruiu added inline comments.

================
Comment at: ELF/Driver.cpp:150-151
@@ +149,4 @@
+ error("-shared and -pie may not be used together");
+ if (Config->Static)
+ error("-static and -pie may not be used together");
+ }


Why is -pie incompatible with -static?

It is not fundamentally so. A static program can be self relocating
and openbsd has static pie binaries.

Hmm after that I answered a question to myself - what linker is used to produce that binaries.
I knew that gold does not support mix of -pie and -static. That was the reason I added this lines initially.
I just for some reason was sure that bfd also restricts it.

I checked it for bfd and found that options are working fine together. So now I think that was correct to remove
that check from final patch.

Cheers,
Rafael

George.