Page MenuHomePhabricator

[SSP] Remove llvm.stackprotectorcheck.
ClosedPublic

Authored by timshen on Feb 29 2016, 1:52 PM.

Details

Summary

This is a cleanup patch for SSP support in LLVM. There is no functional change.
llvm.stackprotectorcheck is not needed, because SelectionDAG isn't
actually lowering it in SelectBasicBlock; rather, it adds check code in
FinishBasicBlock, ignoring the position where the intrinsic is inserted
(See FindSplitPointForStackProtector()).

Diff Detail

Event Timeline

timshen updated this revision to Diff 49418.Feb 29 2016, 1:52 PM
timshen retitled this revision from to [SSP, 1/2] Refactor to support customizable stack guard load from IR level..
timshen updated this object.
timshen added a reviewer: echristo.
timshen updated this object.
timshen added subscribers: iteratee, kbarton, joerg and 2 others.
timshen updated this object.Feb 29 2016, 1:55 PM
echristo edited edge metadata.Mar 3 2016, 4:47 PM

I don't really understand why we need new intrinsics here. Can you explain the design a bit more? Can you also comment the intrinsics with what they do and how they're different from the existing ones?

Thanks!

-eric

timshen updated this revision to Diff 49788.Mar 3 2016, 5:35 PM
timshen edited edge metadata.

tl;dr: because we have a million code paths. :)

The customization point in backends is the opcode LOAD_STACK_GUARD. It already exists for other reasons, but suitable for our purpose.

Previously we have two code paths to handle SSP, SelectionDAG path and IR path.

To customize stack guard loading in IR SSP, we need llvm.stackguardvalue. It lowers to LOAD_STACK_GUARD directly.

To customize stack guard loading in SelectionDAG, llvm.stackprotectorcheck has to take a value, so it doesn't have to be an address (e.g. could be a llvm.stackguardvalue call). To not break existing user code (it's documented) and tests, I created llvm.stackprotectorcheckvalue instead. llvm.stackprotectorcheck is not generated by LLVM anymore. See http://lists.llvm.org/pipermail/llvm-dev/2016-February/095760.html. We may want to delete llvm.stackprotectorcheck?

I haven't push all generic stack guard loading (typically loading the global variable __stack_chk_guard) to the backends. Once we push them all down, we can remove target-independent code path that deals with global variables. But we still need these two intrinsics for supporting both SelectionDAG and IR approach.

I added some comments for these two intrinsics.

Overall I think we're going to want a path that takes us through AutoUpgrade so that going forward we have a single path for all targets.

<insert about 30-45 minutes of discussion>

We're looking at a major design overhaul here, I think, so getting some notes down:

TLI->

lowerStackGuardStore
lowerStackGuardLoad
lowerStackCanaryLoad

We already have a guard node, do we need a separate SDag node for each of these?

Do we need this, effectively, duplicated 3x with SDag, FastIsel, Post-RA handling?

How do we keep the analysis information in the analysis pass and the rest of it in the IR? This would be the other side information like how to sort, strong, etc.

Need to continue to handle:

Coloring reuse
Sibcall optimizations
Avoid stack spill of canary values
Architecture neutral base case

and of course all of this with auto upgrade because I like my ponies to have unicorn horns.

timshen updated this revision to Diff 51512.Mar 23 2016, 9:58 PM

I looked at LLVM manual and fortunately both llvm.stackprotector and
llvm.stackprotectorcheck is required to be passed in @__stack_chk_guard.
This limited flexibility allows us to directly change stackprotectorcheck's
signature and make an auto-upgrade for it.

In long term, I'd like to keep a diamond code path:

  1. An IR pass that generates IR that calls llvm.stackprotector, llvm.stackprotectorcheck and llvm.experimental_stackguardvalue.
  2. Either FastISel, or SelectionDAG handles the generated IR. Their shouldn't be an "IR approach" when FastISel isn't turned on (see the comment on TLI::supportsSelectionDAGSP).
  3. Optional backend lowering is supported by overriding TLI::forceLoadStackGuardNode and TLI::getStackGuardAddr.

Notice that forceLoadStackGuardNode and getStackGuardAddr serves following
different purposes:

  • Some backends want to lower the global variable manually, in such case forceLoadStackGuardNode should return true and getStackGuardAddr returns the global variable (search LOAD_STACK_GUARD in lib/Target).
  • Other platforms, e.g. PowerPC 64 bit wants to lower the stack guard loading completely manually, getStackGuardAddr should returns nullptr.
  • For the rest forceLoadStackGuardNode returns false and getStackGuardAddr returns the global variable.
timshen updated this object.Mar 23 2016, 10:01 PM
timshen added a reviewer: iteratee.
timshen removed a subscriber: iteratee.

Starting to look better, needs autoupgrade support and tests. I'm uncertain about putting one aspect of the set of intrinsics into experimental since the other two aren't already, but it's a simple change once we get the rest of it.

Can you add to the patch description a lot of the comments in the review? Also talk about the change of useLoadStackGuardNode to forceLoadStackGuardNode, etc.

Thanks!

-eric

timshen updated this revision to Diff 51614.Mar 24 2016, 3:56 PM

As discussed, added documentation for StackProtector module. Also added a test case for auto-upgrade.

echristo added inline comments.Mar 24 2016, 4:00 PM
test/CodeGen/Generic/upgrade-stackprotectorcheck.ll
1 ↗(On Diff #51614)

Should probably be in test/Assembler/auto_upgrade_intrinsics.ll or some such?

And you should check that the correct IR is generated at autoupgrade time.

timshen updated this revision to Diff 51616.Mar 24 2016, 4:15 PM

Done. test/Bitcode seems to be a suitable place as well.

But there's already a large test that covers all sorts of intrinsics where I mentioned?

timshen updated this revision to Diff 51617.Mar 24 2016, 4:36 PM

The last function created/touched in test/Assembler/auto_upgrade_intrinsics.ll is at 2013-10-07. This file has only three tests - seems not popular :).

What's the benefit of putting all test cases in one file?

For all of these? Because we're just parsing and disassembling, nothing complicated that'd be helped by multiple files and multiple process overhead.

timshen updated this revision to Diff 51619.Mar 24 2016, 5:04 PM

Ah never mind, I guess test/Assembler/auto_upgrade_intrinsics.ll is a good place to hold target-independent intrinsic upgrades - there are actually not that many.

Cool. This is OK with me then.

Thanks for all of the hard work here.

iteratee edited edge metadata.Mar 25 2016, 1:41 PM

Some comments, mostly about comments and documentation.

include/llvm/IR/Intrinsics.td
329

I agree with eric that this shouldn't be experimental.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5525

I'm not sure I understand this comment.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h
541–542

Is this still accurate? Are we keeping support for loading an address?

lib/CodeGen/StackProtector.cpp
342

I think this comment should read %1 = <load or intrinsic to read stack guard>

lib/Target/AArch64/AArch64ISelLowering.cpp
10082

Can you add a comment about why this needs to be overridden for this platform?

timshen updated this revision to Diff 51688.Mar 25 2016, 2:39 PM
timshen marked 3 inline comments as done.
timshen edited edge metadata.
This comment was removed by timshen.
timshen added inline comments.Mar 25 2016, 2:41 PM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5525

Updated - simply forward the Value* to visitSPDescriptorParent, and inspect if it's a CallInst or a Load there.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h
541–542

According to the documentation, we never support loading an arbitrary address. We only support loading @__stack_chk_guard. I would say in visitSPDescriptorParent, it doesn't have to check if the LoadInst is actually loading @__stack_chk_guard, because we are already doing more than that (loads @__guard_local on OpenBSD).

timshen added inline comments.Mar 25 2016, 2:43 PM
include/llvm/IR/Intrinsics.td
329

Why do you think removing experimental is safer? Isn't keeping it in any situation a safer move?

timshen added inline comments.Mar 25 2016, 2:56 PM
include/llvm/IR/Intrinsics.td
329

First safer -> better

timshen updated this revision to Diff 51834.Mar 28 2016, 1:50 PM

Hi,

I realized that we don't really need llvm.stackguardcheck, so I try to come up with this patch as the first step of cleanup.

timshen updated this revision to Diff 51835.Mar 28 2016, 1:51 PM

Also deleted documentation.

timshen retitled this revision from [SSP, 1/2] Refactor to support customizable stack guard load from IR level. to [SSP] Remove llvm.stackprotectorcheck..Mar 28 2016, 1:52 PM
timshen updated this object.

Mostly looks good.

lib/CodeGen/StackProtector.cpp
357

Comment seems to belong to the instruction below. A comment about HasIRCheck would be nice too.

lib/Target/X86/X86ISelLowering.cpp
2203

The address space will go away in a later patch, correct?

timshen updated this revision to Diff 51844.Mar 28 2016, 2:42 PM
timshen marked 2 inline comments as done.

Updated HasIRCheck comment.

lib/Target/X86/X86ISelLowering.cpp
2203

Yes, finally X86 should use LOAD_STACK_GUARD as well, but before that I'd like to introduce llvm.stackguard() intrinsic. After that point getStackGuardAddr isn't even needed.

I have no more problems, and generally like your approach. It would be nice to get someone who has worked on this code before to sign off.

Hi Michael, since you wrote the SSP code (including stackprotectorcheck, according to git history), can you take a look at this cleanup?

Thanks!

Friendly ping. :)

Few inline comments and requests for more comments. Otherwise it's looking pretty nice.

Thanks!

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
2019

Typical llvm style is: assert((cond) && "Some explanatory text")

(This happens in other places in your patch, I'll just include the first one)

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
1482

Comment.

lib/CodeGen/StackProtector.cpp
205

Standard style for this would be no braces for any of the conditionals or block.

lib/CodeGen/TargetLoweringBase.cpp
1750

Block comments on all of these if there aren't any in the .h file please? (I prefer more detailed comments on the implementation, but still).

test/Assembler/auto_upgrade_intrinsics.ll
57

Hmm?

echristo accepted this revision.Apr 7 2016, 4:16 PM
echristo edited edge metadata.

Oh, and long as you add the inline comments, fix the style stuff, etc, I think we're good to go.

-eric

test/Assembler/auto_upgrade_intrinsics.ll
57

Nevermind, I see it now. That's frustrating, can you add a comment that the declare is for the objectsize upgrade and why?

This revision is now accepted and ready to land.Apr 7 2016, 4:16 PM
timshen updated this revision to Diff 53077.Apr 8 2016, 1:19 PM
timshen marked 6 inline comments as done.
timshen edited edge metadata.

Updated patch.

Thanks for the style check! It's a big patch and I clearly missed those.

Also rebased to include D18632's change.

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
1482

As suggested by gottesmm, moved this condition into StackProtector::shouldEmitSDCheck(), and documented.

timshen updated this revision to Diff 53079.Apr 8 2016, 1:26 PM

Updated patch for fixing a typo.

Hi eugenis, I changed the function name getStackCookieLocation to getIRStackGuard, since there are two SSP code paths and such a name seems to cause less confusion. Do you think it's a fine name?

eugenis edited edge metadata.Apr 8 2016, 1:43 PM

Hi eugenis, I changed the function name getStackCookieLocation to getIRStackGuard, since there are two SSP code paths and such a name seems to cause less confusion. Do you think it's a fine name?

Absolutely.

include/llvm/Target/TargetLowering.h
1016–1025

This comment makes it sound like this function makes the decision to use SDAG vs IR stack protector.

timshen updated this revision to Diff 53084.Apr 8 2016, 1:50 PM
timshen marked an inline comment as done.
timshen edited edge metadata.

Updated comment for getIRStackGuard.

include/llvm/Target/TargetLowering.h
1016–1025

Moved the comment to StackProtector::CreatePrologue. At that point I was not aware that other code will use this function.

timshen closed this revision.Apr 8 2016, 2:32 PM