Page MenuHomePhabricator

Add support for Warning Flag "-Wstack-usage="
AbandonedPublic

Authored by rsanthir.quic on May 19 2021, 9:03 AM.

Details

Summary

With this implementation "-Wstack-usage" acts as an alias to
"-Wframe-larger-than"

Diff Detail

Event Timeline

rsanthir.quic created this revision.May 19 2021, 9:03 AM
rsanthir.quic requested review of this revision.May 19 2021, 9:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2021, 9:03 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Generally looks reasonable to me - but I'll leave signoff to some other folks who have been more involved in the discussion so far.

apazos added inline comments.May 19 2021, 1:51 PM
clang/lib/Basic/Warnings.cpp
101 ↗(On Diff #346482)

Since GCC supports the negative option -Wno-stack-usage, I think it is best to add the negative option Wno-frame-larger-than in the td file and alias it too, to avoid this C code in here.

Localized changes to Options.td, needed to explicitly add negative
flag for -Wframe-larger-than

bruno added a comment.May 20 2021, 2:45 PM

Overall looks good, sounds like it's failing tests though?

Thanks for reviewing @bruno doesn't look like the failure is related to my change:
https://buildkite.com/llvm-project/premerge-checks/builds/39905#5f70c261-ae54-451b-b771-7012bcee7387
"No space left on device"

Unless I am looking at the wrong thing.

Updated test and aligned negative flag with gcc

Updated Release Notes and rebased

@bruno all tests are passing, could you take another look when you have a chance?

Quuxplusone added inline comments.
clang/docs/ReleaseNotes.rst
79–80

Does this mean:

  • Warn if the size of any single function's stack frame (including temporaries and local variables, but not parameters or return addresses) exceeds <byte-size>.
  • Warn if the size of any single function's stack frame (including parameters and return addresses) exceeds <byte-size>.
  • Warn if the total stack usage of the longest visible call chain in this translation unit might exceed <byte-size>.

?

clang/test/Frontend/backend-stack-usage-diagnostic.c
19–23

This function has no "stack frame" in the usual sense of the word, because it's just

movl    %edi, %eax
imull   %edi, %eax
ret

So am I correct to infer that the -Wstack-usage= option includes the size of the return address in its computations? So the stack usage of this function would be computed as "8 bytes" because the callq instruction pushes 8 bytes on the stack?
This seems eminently reasonable to me, but non-obvious, and should get some user-facing documentation.

rsanthir.quic added inline comments.Jun 9 2021, 3:17 PM
clang/docs/ReleaseNotes.rst
79–80

Looking at how the value for this flag is obtained, which is an alias of "-Wframe-larger-than", I see that it is all objects placed on the stack by each function plus any space reserved for arguments of callsites in the function.

There is no precise definition of what is included as it depends on the target as well as what is stored on the stack or not.

I guess in simplest terms, for each function this will report how much stack space is used by that function.

clang/test/Frontend/backend-stack-usage-diagnostic.c
19–23

When a function stores the return address on the stack, that will be included in that functions stack usage.

ping @Quuxplusone this is being used as a diagnostic flag that aliases "-Wframe-larger-than", I think if we want to add more user-facing documentation to clarify things we should revisit what the "-Wframe-larger-than=" tracks as well. Currently there isn't too much info on that flag either.

MaskRay added a comment.EditedJul 2 2021, 8:32 PM

Does adding this alias unblock some applications?

In GCC -Wstack-usage= is more powerful. It can check warning: stack usage might be unbounded.
Having this alias could give false impression that we have implemented the powerful detection usage.
If we don't actually implement the -Wstack-usage= functionality, I'd hope that applications consciously and gracefully degrade to -Wframe-larger-than=, instead of using the half-functional -Wstack-usage.

@MaskRay Yes this would unblock applications. Regarding your concern, the information from this implementation as well as GCC's should be used conservatively as both are approximate.

@MaskRay Yes this would unblock applications. Regarding your concern, the information from this implementation as well as GCC's should be used conservatively as both are approximate.

Do your applications parse the diagnostic to take some actions? Note that clang diagnostic format is different from GCC's.
If you need any adaptation, can you just change the -Wstack-usage= option to -Wframe-larger-than= (the former is stronger but Clang doesn't implement the full functionality)?

This was discussed in llvm-dev mailing list, and originally we had a change that was closer to what GCC was reporting however there was no consensus on what was needed. The purpose of this change is to bring parity in terms of available options with GCC. @lebedev.ri could you chime in on what your specific use for this flag is?

rsanthir.quic abandoned this revision.Tue, Jul 27, 12:11 PM