This is an archive of the discontinued LLVM Phabricator instance.

[safestack] Support safestack in stack size diagnostics
ClosedPublic

Authored by paulkirth on Feb 16 2022, 5:44 PM.

Details

Summary

Current stack size diagnostics ignore the size of the unsafe stack.
This patch attaches the size of the static portion of the unsafe stack
to the function as metadata, which can be used by the backend to emit
diagnostics regarding stack usage.

Diff Detail

Event Timeline

paulkirth created this revision.Feb 16 2022, 5:44 PM

Fix ARM test breakage

paulkirth updated this revision to Diff 409797.Feb 17 2022, 2:48 PM

Actually fix the ARM test

paulkirth edited the summary of this revision. (Show Details)Mar 4 2022, 5:41 PM
paulkirth added reviewers: phosek, mcgrathr, tstellar.
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2022, 5:41 PM
paulkirth published this revision for review.Mar 4 2022, 5:41 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 4 2022, 5:41 PM
mcgrathr added inline comments.Mar 4 2022, 6:54 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1211–1212

Should we also update emitStackSizeSection to match?
I'm not sure whether that's meant to be used for diagnostic-like cases or for more concrete backend uses where the distinction between the two stacks still matters.

paulkirth added inline comments.Mar 4 2022, 7:07 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1211–1212

I am not exactly sure, but https://reviews.llvm.org/D39788, which I believe adds the feature seems like it is only used for diagnostic information according to the RFC they mention: http://lists.llvm.org/pipermail/llvm-dev/2017-August/117028.html.

Changing the behavior to be aware of the safe-stack, however, does seem consistent with the original goal of the feature, so I'm happy to update the patch to reflect that.

paulkirth updated this revision to Diff 413556.Mar 7 2022, 10:41 AM

Update emitStackSection to emit the size of the combined stack

@MatzeB @seaneveson Can either of you confirm that the logic we're inserting here is not in conflict with the stack size section? Based on D39788 and the original RFC, I would say accounting for safe stack usage is the correct thing to do, but I am not certain.

clang/test/Frontend/stack-usage-safestack.c
2

I think this should be a bitcode-level test à la test/CodeGen/X86/warn-stack.ll
Or at least it should use fixed-size integers so that the checked values are more explicit (a.k.a the test here works because of the triple that implies sizeof(int) == 4).

paulkirth updated this revision to Diff 419493.Mar 31 2022, 9:23 AM

Update tests.

  • Dont rely on size of int based on platform
  • Add checking to backend tests for warn-stack-size to ensure the behavior is consistant when safestack is enabled
mcgrathr accepted this revision.Apr 19 2022, 4:20 PM
This revision is now accepted and ready to land.Apr 19 2022, 4:20 PM
phosek accepted this revision.Apr 19 2022, 4:47 PM

LGTM

clang/test/Frontend/stack-usage-safestack.c
4

Is there any particular reason for using the i386-apple-darwin triple here and below? I'm not even sure if SafeStack is officially supported and tested on Darwin, x86_64-linux may be a safer choice.

This revision was landed with ongoing or failed builds.Apr 20 2022, 11:30 AM
This revision was automatically updated to reflect the committed changes.
paulkirth marked an inline comment as done.

Hi @paulkirth,

using of specific triple within stack-usage-safestack.c test causes a failure for the compilers, which don't support these triples (arm/aarch64 in my case).
Such as:

error: unable to create target: 'No available targets are compatible with triple "i386-apple-darwin"'

see more details in https://lab.llvm.org/buildbot/#/builders/119/builds/8169/steps/9/logs/FAIL__Clang__stack-usage-safestack_c result for the failed build.

would you fix the test by removing these triples from the command line or by isolating this test for specific target with // REQUIRES: directive?

Hi @paulkirth,

using of specific triple within stack-usage-safestack.c test causes a failure for the compilers, which don't support these triples (arm/aarch64 in my case).
Such as:

error: unable to create target: 'No available targets are compatible with triple "i386-apple-darwin"'

see more details in https://lab.llvm.org/buildbot/#/builders/119/builds/8169/steps/9/logs/FAIL__Clang__stack-usage-safestack_c result for the failed build.

would you fix the test by removing these triples from the command line or by isolating this test for specific target with // REQUIRES: directive?

Oh, that's surprising. I followed the procedure from another test, so I'm surprised that this is failing when that one is not, but maybe I missed the REQUIRES directive. I can probably get a change out fairly quickly, but if its blocking you, feel free to revert this and we can re-land later.