This is an archive of the discontinued LLVM Phabricator instance.

[safestack] Use non-thread-local unsafe stack pointer for Contiki OS
ClosedPublic

Authored by mlemay-intel on May 2 2016, 10:23 PM.

Details

Summary

This patch causes a non-thread-local unsafe stack pointer to be used
with SafeStack for Contiki OS. To avoid generating a redundant copy
of the code for looking up or creating the unsafe stack pointer
variable, this patch moves that code from SafeStack to
TargetLoweringBase.

This patch removes the -mllvm -safe-stack-usp-storage option, since it
is no longer needed.

Diff Detail

Repository
rL LLVM

Event Timeline

mlemay-intel retitled this revision from to [safestack] Use non-thread-local unsafe stack pointer for Contiki OS.
mlemay-intel updated this object.
mlemay-intel added reviewers: pcc, eugenis.
mlemay-intel added a subscriber: llvm-commits.

This patch and D19854 are useful independent of my segmentation-related patches. I also have a Contiki OS patch that is blocked on these. So, I would appreciate having these reviewed separately from my other patches. Thanks!

eugenis added inline comments.Jun 9 2016, 12:02 PM
include/llvm/ADT/Triple.h
166 ↗(On Diff #55942)

Please add to the end of the list.

lib/Target/X86/X86ISelLowering.cpp
1940 ↗(On Diff #55942)

Why duplicate all this code instead of just checking for isTargetContiki is SafeStack.cpp?

mlemay-intel added inline comments.Jun 9 2016, 1:44 PM
include/llvm/ADT/Triple.h
166 ↗(On Diff #55942)

Will do.

lib/Target/X86/X86ISelLowering.cpp
1940 ↗(On Diff #55942)

@pcc commented on D19483:

You should be using -target x86-unknown-contiki or similar. That should tune the behaviour to what is required for that OS. See what we have in TargetLoweringBase::getSafeStackPointerLocation to provide Android-specific behaviour for example.

My Contiki OS patches only add SafeStack support on X86, so that's another reason to put this code here.

Moved new OS type definition to end of list.

mlemay-intel marked 2 inline comments as done.Sep 2 2016, 2:28 PM

ping. Please keep in mind that this is independent of my patches for supporting a separate stack segment. This patch is needed to enable SafeStack for Contiki OS.

pcc added inline comments.Sep 23 2016, 3:22 PM
lib/Target/X86/X86ISelLowering.cpp
2025 ↗(On Diff #70072)

You might also consider moving the SafeStack::getOrCreateUnsafeStackPtr implementation into a utility function in TargetLoweringBase that takes a bool UseTLS. Then guarantee that getSafeStackPointerLocation always returns non-null and you can remove SafeStack::getOrCreateUnsafeStackPtr altogehter.

mlemay-intel added inline comments.Sep 23 2016, 4:00 PM
lib/Target/X86/X86ISelLowering.cpp
2025 ↗(On Diff #70072)

That would be a nice way to eliminate this redundancy. However, the SafeStack pass is written to support TL being null. How would your suggested approach handle that case?

pcc added inline comments.Sep 23 2016, 4:04 PM
lib/Target/X86/X86ISelLowering.cpp
2025 ↗(On Diff #70072)

What happens if you assert(TL);? Do any tests fail?

mlemay-intel added inline comments.Sep 23 2016, 4:52 PM
lib/Target/X86/X86ISelLowering.cpp
2025 ↗(On Diff #70072)

I added assert(TL) right after the initialization of TL in runOnFunction and ran check-llvm and check-clang. There were no unexpected failures.

pcc added inline comments.Sep 23 2016, 4:59 PM
lib/Target/X86/X86ISelLowering.cpp
2025 ↗(On Diff #70072)

In that case, I would suggest that you change the pass to require TL to be available. There's precedent for this in the CodeGen pipeline -- see the StackProtector pass.

Refactored to avoid duplicating code.

pcc accepted this revision.Sep 26 2016, 2:59 PM
pcc edited edge metadata.

LGTM

include/llvm/Target/TargetLowering.h
1115 ↗(On Diff #72401)

You might want to reword this more simply as something like "Returns the target-specific address of the unsafe stack pointer."

This revision is now accepted and ready to land.Sep 26 2016, 2:59 PM
mlemay-intel edited edge metadata.

Reworded a comment.

mlemay-intel marked an inline comment as done.Sep 26 2016, 9:00 PM
echristo accepted this revision.Sep 28 2016, 2:31 PM
echristo added a reviewer: echristo.
echristo added a subscriber: echristo.

Seems reasonable.

lib/Target/X86/X86ISelLowering.cpp
2027 ↗(On Diff #72600)

Not sure that adding isTargetContiki to the subtarget is worth it here, but ok.

mlemay-intel added inline comments.Oct 6 2016, 2:00 PM
lib/Target/X86/X86ISelLowering.cpp
2027 ↗(On Diff #72600)

TM is private in TargetLoweringBase, so I can't call TM.getTargetTriple().isOSContiki() from here.

echristo added inline comments.Oct 6 2016, 7:24 PM
lib/Target/X86/X86ISelLowering.cpp
2027 ↗(On Diff #72600)

Subtarget.getTargetTriple().isOSContiki()?

pcc added inline comments.Oct 7 2016, 11:42 AM
include/llvm/Target/TargetLowering.h
1111 ↗(On Diff #72600)

Doesn't need to be virtual.

mlemay-intel edited edge metadata.

Use Subtarget.getTargetTriple().isOSContiki instead of Subtarget.isTargetContiki.
Make TargetLoweringBase::getDefaultSafeStackPointerLocation non-virtual.

At this point if Peter and Evgenii are happy so am I.

This revision was automatically updated to reflect the committed changes.