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

[rsanthir.quic]

Summary

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

[dblaikie]

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]

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.

[rsanthir.quic]

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

[bruno]

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

[rsanthir.quic]

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.

[rsanthir.quic]

rebase

[rsanthir.quic]

Updated test

[rsanthir.quic]

Updated test and aligned negative flag with gcc

[rsanthir.quic]

Updated Release Notes and rebased

[rsanthir.quic]

ping

[rsanthir.quic]

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

[• Quuxplusone]

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]

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.

[rsanthir.quic]

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.

[rsanthir.quic]

ping

[MaskRay]

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.

[rsanthir.quic]

@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]

In D102782#2862065, @rsanthir.quic wrote:

@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)?

[rsanthir.quic]

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?

clang/docs/ReleaseNotes.rst

 
 - ``-Wreserved-identifier`` emits warning when user code uses reserved
   identifiers.
 
 - ``-fstack-usage`` generates an extra .su file per input source file. The .su
   file contains frame size information for each function defined in the source
   file.
 
+- ``-Wstack-usage=<byte-size>`` warn if stack usage of user functions might
+  exceed <byte-size>.
+
 Deprecated Compiler Flags
 -------------------------
 
 - ...
 
 Modified Compiler Flags
 -----------------------
 

clang/include/clang/Driver/Options.td

 def Wlarge_by_value_copy_EQ : Joined<["-"], "Wlarge-by-value-copy=">, Flags<[CC1Option]>,
   MarshallingInfoInt<LangOpts<"NumLargeByValueCopy">>;
 
 // These "special" warning flags are effectively processed as f_Group flags by the driver:
 // Just silence warnings about -Wlarger-than for now.
 def Wlarger_than_EQ : Joined<["-"], "Wlarger-than=">, Group<clang_ignored_f_Group>;
 def Wlarger_than_ : Joined<["-"], "Wlarger-than-">, Alias<Wlarger_than_EQ>;
 def Wframe_larger_than_EQ : Joined<["-"], "Wframe-larger-than=">, Group<f_Group>, Flags<[NoXarchOption]>;
+def Wno_frame_larger_than : Joined<["-"], "Wno-frame-larger-than">, Group<f_Group>, Flags<[NoXarchOption]>;
+def Wstack_stack_usage_EQ : Joined<["-"], "Wstack-usage=">, Flags<[NoXarchOption]>,
+  Alias<Wframe_larger_than_EQ>;
+def Wno_stack_stack_usage : Joined<["-"], "Wno-stack-usage">, Flags<[NoXarchOption]>,
+  Alias<Wno_frame_larger_than>;
 
 def : Flag<["-"], "fterminated-vtables">, Alias<fapple_kext>;
 defm threadsafe_statics : BoolFOption<"threadsafe-statics",
   LangOpts<"ThreadsafeStatics">, DefaultTrue,
   NegFlag<SetFalse, [CC1Option], "Do not emit code to make initialization of local statics thread safe">,
   PosFlag<SetTrue>>;
 def ftime_report : Flag<["-"], "ftime-report">, Group<f_Group>, Flags<[CC1Option]>,
   MarshallingInfoFlag<CodeGenOpts<"TimePasses">>;

clang/lib/Driver/ToolChains/Clang.cpp

if (!Triple.isOSAIX())
       D.Diag(diag::err_drv_unsupported_opt_for_target)
           << A->getSpelling() << RawTriple.str();
     if (A->getOption().getID() == options::OPT_mabi_EQ_vec_extabi)
       CmdArgs.push_back("-mabi=vec-extabi");
     else
       D.Diag(diag::err_aix_default_altivec_abi);
   }
 
+  if (Args.hasFlag(options::OPT_Wframe_larger_than_EQ,
+                   options::OPT_Wno_frame_larger_than, false))
     if (Arg *A = Args.getLastArg(options::OPT_Wframe_larger_than_EQ)) {
       StringRef v = A->getValue();
       CmdArgs.push_back("-mllvm");
       CmdArgs.push_back(Args.MakeArgString("-warn-stack-size=" + v));
       A->claim();
     }
 
   if (!Args.hasFlag(options::OPT_fjump_tables, options::OPT_fno_jump_tables,
                     true))
     CmdArgs.push_back("-fno-jump-tables");
 
   if (Args.hasFlag(options::OPT_fprofile_sample_accurate,
                    options::OPT_fno_profile_sample_accurate, false))
     CmdArgs.push_back("-fprofile-sample-accurate");

clang/test/Frontend/backend-stack-usage-diagnostic.c

+// RUN: %clang -Wstack-usage=0 -o /dev/null -c %s 2> %t.err
+// RUN: FileCheck < %t.err %s --check-prefix=WARN
+
+// RUN: %clang -Wno-stack-usage -Wstack-usage=0 -o /dev/null -c %s 2> %t.err
+// RUN: FileCheck < %t.err %s --check-prefix=WARN
+
+// RUN: %clang -Wstack-usage=0 -Wno-stack-usage -o /dev/null -c %s 2> %t.err
+// RUN: FileCheck < %t.err %s --check-prefix=IGNORE --allow-empty
+
+// RUN: %clang -Wstack-usage=0 -w -o /dev/null -c %s 2> %t.err
+// RUN: FileCheck < %t.err %s --check-prefix=IGNORE --allow-empty
+
+// RUN: %clang -Wstack-usage=3 -o /dev/null -c %s 2> %t.err
+// RUN: FileCheck < %t.err %s --check-prefix=WARN
+
+// RUN: %clang -Wstack-usage=100 -o /dev/null -c %s 2> %t.err
+// RUN: FileCheck < %t.err %s --check-prefix=IGNORE --allow-empty
+
+// WARN: warning: stack frame size of {{[0-9]+}} bytes in function 'test_square'
+// IGNORE-NOT:  stack frame size of {{[0-9]+}} bytes in function 'test_square'
+int test_square(int num) {
+  return num * num;
+}
+