This is an archive of the discontinued LLVM Phabricator instance.

Support for the mno-stack-arg-probe flag
ClosedPublic

Authored by nruslan on Feb 8 2018, 6:00 PM.

Details

Summary

Adds support for this flag. There is also another piece for llvm (separate review). More info: https://bugs.llvm.org/show_bug.cgi?id=36221

Related: D43107

Diff Detail

Event Timeline

nruslan created this revision.Feb 8 2018, 6:00 PM

@craig.topper, @MatzeB, @hans: Can someone take a look at this change?

hans added a comment.Feb 12 2018, 1:44 AM

I see in the PR that matches a MinGW flag, but I'm curious to the motivation here. In what scenario would the user want to use this, i.e. how do they know it's safe to drop the probes?

lib/CodeGen/TargetInfo.cpp
2358

I'd suggest perhaps "addStackProbeTargetAttributes" as a name instead, since I'm not sure what Params is for.

2361

This could be written as

if (llvm::Function *Fn = dyn_cast_or_null<llvm::Function>(GV)) {
lib/Driver/ToolChains/Clang.cpp
4038

I think you can just do

Args.AddLastArg(CmdArgs, options::OPT_mno_stack_arg_probe)

which avoids the if-statement and having to spell out the flag.

  • No test.
  • What about -mstack-arg-probe, shouldn't we have that for consistency?
  • I'd prefer not to review clang changes myself as I don't know that part of the code too well.

@hans: One real-world example is when it is used to compile UEFI code using PE/COFF targets natively. Obviously, UEFI uses ABI which is basically almost the same as MS ABI, except that chkstk is not used. It mostly works (I actually was able to get it running) except the cases when the code contains variable-sized arrays allocated on stacks. Unfortunately, stack-probe-size will only help with fixed sized array but will not help to solve the problem described in the bug description since stack usage is unknown at compile time. MinGW does not have this problem because it provides this flag.

@MatzeB : there is a test on LLVM side (related review). Do you think the test is needed for clang side? If so, please let me know, what kind of test it is supposed to be.

hans added a comment.Feb 13 2018, 6:31 AM

@hans: One real-world example is when it is used to compile UEFI code using PE/COFF targets natively. Obviously, UEFI uses ABI which is basically almost the same as MS ABI, except that chkstk is not used. It mostly works (I actually was able to get it running) except the cases when the code contains variable-sized arrays allocated on stacks. Unfortunately, stack-probe-size will only help with fixed sized array but will not help to solve the problem described in the bug description since stack usage is unknown at compile time. MinGW does not have this problem because it provides this flag.

I see, interesting. Might be worth mentioning in the commit message for others wondering what the flag is useful for.

@MatzeB : there is a test on LLVM side (related review). Do you think the test is needed for clang side? If so, please let me know, what kind of test it is supposed to be.

Yes please, I think think there should be on in test/Driver/ to check that forwarding the flag to cc1 (and if we have a -mno-foo, there should maybe be a -mfoo variant too?), and a test in test/CodeGen/ to check that the attribute gets put on the functions correctly. Perhaps r321992 is a good example to look at.

nruslan updated this revision to Diff 134377.Feb 14 2018, 9:33 PM
nruslan marked 3 inline comments as done.

Added mstack-arg-probe, tests, and changes related to other comments of the reviewers

nruslan added inline comments.Feb 14 2018, 9:37 PM
lib/Driver/ToolChains/Clang.cpp
4038

I added mstack_arg_probe also, and this check now contains two options. I am not sure how AddLastArg behaves in this case (especially because only one attribute needs to be defined: mno-stack-arg-probe, and a positive case is the default). I found some other examples similar to this one, and they used hasArg with two options.

nruslan marked an inline comment as done.Feb 14 2018, 9:37 PM

@hans: I responded to the comments and also updated diff for clang with new changes.

hans accepted this revision.Feb 16 2018, 1:57 AM

This looks good to me.

This revision is now accepted and ready to land.Feb 16 2018, 1:57 AM

@hans, @craig.topper, @MatzeB: Can someone commit D43108 (this) and D43107 on my behalf? I do not think, I have commit access.

This revision was automatically updated to reflect the committed changes.

Can we clarify the meaning of this option a bit. The doc you've added here is saying that -mno-stack-arg-probe disables stack probes. Then what does -mstack-arg-probe mean specifically? Does it mean that only stack probes for ABI required reasons are enabled, or probes are done even in cases where the ABI doesn't require them? Either way, the doc needs to be clearer on the exact purpose.

I'm currently working on enabling stack probes for reasons other than ABI, and so if the answer is that this option is only concerned with ABI, we will need another option like -fstack-check.

@aemerson : I am not 100% sure, but I think you are talking about different flag. This commit is for supporting a flag to disable stack probes (identical flag can be found in MinGW), it basically only applies to PE/COFF (Windows) targets. It can be useful to compile UEFI code. By default, Windows will use check probes of 4K in x86. A related stack-probe-size allows to vary the size of stack probes. This one completely disables them.

By default, stack probes are enabled (i.e., -mstack-arg-probe is the default behavior) and have the size of 4K in x86.

By default, stack probes are enabled (i.e., -mstack-arg-probe is the default behavior) and have the size of 4K in x86.

This part what I wanted to clarify, -mstack-probe-arg is enabling stack probes if the ABI requires it only, not for other reasons like security.

@aemerson : yes, it is just part of MS ABI