Page MenuHomePhabricator

[PowerPC] Option for secure plt mode
ClosedPublic

Authored by spetrovic on Mar 27 2018, 5:45 AM.

Details

Summary

This patch enables option for secure plt mode in clang (-msecure-plt). This feature is supported in backend also (https://reviews.llvm.org/D42112).

Diff Detail

Repository
rL LLVM

Event Timeline

spetrovic created this revision.Mar 27 2018, 5:45 AM

I'll let Justin give the actual ACK, but this looks fine to me. The only question that I have (since I don't know anything about secure PLT) is whether this is a PPC-specific thing (since the option is a PPC option).

include/clang/Driver/Options.td
1941 ↗(On Diff #139904)

Do we not want the negated option?

lib/Driver/ToolChains/Arch/PPC.cpp
116 ↗(On Diff #139904)

Just a style question out of curiosity. Why the temporary? Since there are just two possible values, why not just return the respective enumerator?

spetrovic updated this revision to Diff 140060.Mar 28 2018, 3:46 AM
spetrovic added inline comments.
include/clang/Driver/Options.td
1941 ↗(On Diff #139904)

Well, I'm not sure if we need negated option. This is just switch from default mode (BSS) to SECURE PLT mode, also I didn't see that gcc has negated option for SECURE-PLT.

lib/Driver/ToolChains/Arch/PPC.cpp
116 ↗(On Diff #139904)

You are right, we don't need the temporary.

Yes, secure PLT is PowerPC specific feature.

joerg added a subscriber: joerg.Mar 28 2018, 5:45 AM

GCC supports -mbss-plt to get the legacy behavior. Not sure if anyone actually uses it though.

GCC supports -mbss-plt to get the legacy behavior. Not sure if anyone actually uses it though.

@spetrovic Is this something we want to implement?

-mbss-plt is currently default in LLVM, once secure plt support is finished we can set secure plt as default in LLVM, but not for now.

-mbss-plt is currently default in LLVM, once secure plt support is finished we can set secure plt as default in LLVM, but not for now.

I was thinking in case the override is needed to override something specified in make files, etc. of some build systems. In any case, this all looks fine to me. Perhaps @jhibbits/@chmeee or @joerg should give the final ACK for this. If not, I'll approve it next time you ping the patch.

jhibbits accepted this revision.Apr 10 2018, 2:42 PM
This revision is now accepted and ready to land.Apr 10 2018, 2:42 PM
This revision was automatically updated to reflect the committed changes.