This is an archive of the discontinued LLVM Phabricator instance.

Only throw -fPIC when building a shared library
ClosedPublic

Authored by asmith on Feb 6 2018, 4:08 PM.

Details

Summary

Update makefiles to specify -fPIC in Makefile.rules and only throw -fPIC when building a shared library. This change is necessary to allow building the lldb tests on Windows where -fPIC is not a valid option.

Update a few places to Python 3.x syntax

Diff Detail

Event Timeline

asmith created this revision.Feb 6 2018, 4:08 PM

In the future when you upload diffs can you include context? (i.e. git diff -U999999). It's nice to be able to see the surrounding code when I'm looking at a diff.

Is there ever a case where you would want to build a shared library without -fPIC? I'm wondering if we should just update the common Makefile.rules and if DYLIB_NAME is set (or something else indicating that this is a shared library), then we put the logic about -fPIC in that common file. Then people wouldn't have to remember to get this right in the future.

labath added a subscriber: labath.Feb 7 2018, 3:13 AM

In the future when you upload diffs can you include context? (i.e. git diff -U999999). It's nice to be able to see the surrounding code when I'm looking at a diff.

Is there ever a case where you would want to build a shared library without -fPIC? I'm wondering if we should just update the common Makefile.rules and if DYLIB_NAME is set (or something else indicating that this is a shared library), then we put the logic about -fPIC in that common file. Then people wouldn't have to remember to get this right in the future.

Adding -fPIC by default sounds like a great idea. Building a shared library without it will probably break compilation, but if some test still wants to try it, it can be turned off with -fno-PIC.

asmith updated this revision to Diff 133354.Feb 7 2018, 6:42 PM
asmith edited the summary of this revision. (Show Details)
asmith updated this revision to Diff 133362.Feb 7 2018, 7:54 PM
asmith edited the summary of this revision. (Show Details)
labath added inline comments.Feb 8 2018, 2:03 AM
packages/Python/lldbsuite/test/make/Makefile.rules
550

I think Zachary meant only when building the shared library, but you're doing it unconditionally. This would be a nice place to set the flag for shared library only.

asmith updated this revision to Diff 133467.Feb 8 2018, 11:49 AM
asmith marked an inline comment as done.
asmith retitled this revision from Stop passing -fPIC to lldb tests on Windows to Only throw -fPIC when building a shared library.Feb 8 2018, 1:07 PM
asmith edited the summary of this revision. (Show Details)
asmith edited the summary of this revision. (Show Details)
zturner accepted this revision.Feb 8 2018, 1:54 PM

This lgtm. If this causes some tests that were previously skipped or xfailed to start passing, you can unskip / unxfail them at the same time.

packages/Python/lldbsuite/test/functionalities/process_attach/TestProcessAttach.py
48–49

Weird, does that comma syntax even do anything or was that just a bug?

This revision is now accepted and ready to land.Feb 8 2018, 1:54 PM
stella.stamenova added inline comments.
packages/Python/lldbsuite/test/functionalities/process_attach/TestProcessAttach.py
48–49

In python 2.5 or earlier it meant the same as "as" but "as" was not supported. Then for a while (until 3.x) both the comma and "as" meant the same thing and then since 3.x "as" is the only option allowed.

asmith closed this revision.Feb 8 2018, 3:12 PM