This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] add secure plt support for TLS symbols
ClosedPublic

Authored by spetrovic on Apr 11 2018, 7:59 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

spetrovic created this revision.Apr 11 2018, 7:59 AM

I think overall, it would be good to explain the semantics of this patch. Maybe even provide a reference to a document that explains it if it isn't something that can be summarized into a comment in the review (or a comment in the code).

Also, I imagine you'll want to add some code to getGlobalBaseReg() to disable shrink-wrapping since you're adding a clobber of the LR without introducing any frame references, so shrink-wrapping may move the prologue out of the entry.

lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp
445–453

Nit: naming convention - variables start with an upper case letter. I realize it was that way, but since you're changing its type, might as well change its name.

lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4011

A comment explaining why this is needed would be great.

4012

I think it's pertinent to add a comment to calls like this one outlining why the virtual SDNode returned by this function does not need to have a use and it still won't end up being DCE'd.

test/CodeGen/PowerPC/ppc32-secure-plt-tls.ll
14

I imagine there's supposed to be code up here that sets up the base pointer. Do we not want to check for that code?

spetrovic marked 2 inline comments as done.Apr 20 2018, 5:09 AM

Regarding secure plt in general I was using this document: http://devpit.org/wiki/Debugging_PowerPC_ELF_Binaries. This patch just expands secure-plt support for TLS symbols, basically the same thing as for functions symbols or global symbols in the last patch.
Also as a reference I'm using gcc.
I haven't run into example that fails because of shrink-wrapping. If this is a potential issue would not it have appeared so far, since
getGlobalBaseReg() is existing function and it was used before secure-plt support? Do you have some test case that potentially can reproduce issue related with shrink-wrapping?

test/CodeGen/PowerPC/ppc32-secure-plt-tls.ll
14

These 3 instructions (mflr,addis,addi) are setting up the base pointer. There is one more instruction in this sequence that I forgot to check (bl). I have added it in updated patch.

spetrovic updated this revision to Diff 143295.Apr 20 2018, 5:11 AM
hfinkel added inline comments.
lib/Target/PowerPC/PPCAsmPrinter.cpp
494

Please add a comment here explaining why we're adding 32768.

lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4021

Period at the end of the comment.

spetrovic marked 2 inline comments as done.May 28 2018, 2:47 AM
spetrovic updated this revision to Diff 148797.May 28 2018, 2:49 AM
hfinkel accepted this revision.Jun 5 2018, 4:58 AM

LGTM

lib/Target/PowerPC/PPCAsmPrinter.cpp
493

Can you please reference a document and a section number, or similar?

This revision is now accepted and ready to land.Jun 5 2018, 4:58 AM
spetrovic added inline comments.Jun 18 2018, 8:49 AM
lib/Target/PowerPC/PPCAsmPrinter.cpp
493

I was referencing on source from binutils tools and gcc. Is that OK with you ?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2019, 7:01 AM