This is an archive of the discontinued LLVM Phabricator instance.

[mips] Prevent PIC to be set to level 2
ClosedPublic

Authored by abeserminji on Mar 12 2018, 6:45 AM.

Details

Summary

MIPS does not use PIC level 2 for historical reasons, even with -fPIC/-mxgot/multigot options.
This patch makes Clang behave more like GCC.

Diff Detail

Repository
rC Clang

Event Timeline

abeserminji created this revision.Mar 12 2018, 6:45 AM

This patch looks mostly ok, but I think there are some small issues with it.

Can you separate the new warnings/error relating to -fno-pic / -mabicalls into another patch from the change which alters the behaviour of PIC / pic ?

The title of the patch is misleading, as you're adding warnings related to -fno-pic / -mabicalls and changing how pic is defined.

Resolved a comment.

abeserminji retitled this revision from [mips] Change the way how Clang chooses relocation mode to [mips] Force PIC to level 1.
abeserminji edited the summary of this revision. (Show Details)

Updated the patch. Now this patch only forces PIC to level 1.

abeserminji retitled this revision from [mips] Force PIC to level 1 to [mips] Prevent PIC to be set to level 2.Mar 20 2018, 5:32 AM
abeserminji changed the repository for this revision from rL LLVM to rC Clang.Mar 21 2018, 9:15 AM
abeserminji removed a subscriber: llvm-commits.
sdardis added inline comments.Mar 29 2018, 1:59 AM
lib/Driver/ToolChains/CommonArgs.cpp
1017

Can you reformulate this comment to say that MIPS does not use PIC level 2? Also, please add a note stating that even with -mxgot / -fPIC / multigot , MIPS does not use PIC level 2 unlike other architecutres for historical reasons.

sdardis requested changes to this revision.Apr 6 2018, 3:10 AM

You should also update the description of the patch.

This revision now requires changes to proceed.Apr 6 2018, 3:10 AM
abeserminji marked an inline comment as done.
abeserminji edited the summary of this revision. (Show Details)

Comments resolved.

sdardis accepted this revision.Apr 13 2018, 8:12 AM

LGTM.

This revision is now accepted and ready to land.Apr 13 2018, 8:12 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.