Page MenuHomePhabricator

Support GCC's -fstack-usage flag
ClosedPublic

Authored by pzheng on Apr 14 2021, 2:03 PM.

Details

Summary

This patch adds support for GCC's -fstack-usage flag. With this flag, a stack
usage file (i.e., .su file) is generated for each input source file. The format
of the stack usage file is also similar to what is used by GCC. For each
function defined in the source file, a line with the following information is
produced in the .su file.

<source_file>:<line_number>:<function_name> <size_in_byte> <static/dynamic>

"Static" means that the function's frame size is static and the size info is an
accurate reflection of the frame size. While "dynamic" means the function's
frame size can only be determined at run-time because the function manipulates
the stack dynamically (e.g., due to variable size objects). The size info only
reflects the size of the fixed size frame objects in this case and therefore is
not a reliable measure of the total frame size.

Diff Detail

Event Timeline

pzheng created this revision.Apr 14 2021, 2:03 PM
pzheng requested review of this revision.Apr 14 2021, 2:03 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 14 2021, 2:03 PM

Might you be interested in implementing [[ https://bugs.llvm.org/show_bug.cgi?id=44418 | -Wstack-usage= ]] diag as a follow-up?

pzheng added a comment.EditedApr 14 2021, 2:17 PM

Might you be interested in implementing [[ https://bugs.llvm.org/show_bug.cgi?id=44418 | -Wstack-usage= ]] diag as a follow-up?

Thanks for bringing it up, @lebedev.ri. Yes, I will look into supporting that next.

I left a couple of comments inline on the command-line parsing aspect of the patch. For more info, check https://clang.llvm.org/docs/InternalsManual.html#adding-new-command-line-option.

clang/include/clang/Driver/Options.td
2717

This argument is not necessary, MarshallingInfoString defaults to an empty string by default.

clang/lib/Frontend/CompilerInvocation.cpp
1934

Since OPT_fstack_usage has already been parsed into Opts.StackUsage at this point, it would make more sense to check that instead.

1938

This has already been parsed through MarshallingInfoString<CodeGenOpts<"StackUsageOutput">>, no need to duplicate it here.

pzheng updated this revision to Diff 337778.Apr 15 2021, 8:28 AM

Address @jansvoboda11's comments.

pzheng marked 3 inline comments as done.Apr 15 2021, 8:30 AM

Thanks for your feedback, @jansvoboda11!

It would be great to also update release notes for clang to inform users about this new feature.

pzheng updated this revision to Diff 337802.Apr 15 2021, 9:51 AM

Mention the flag in release notes based on @xbolva00's comment.

<source_file>:<line_number>:<function_name> <size_in_byte> <static/dynamic>

gcc also supports "bounded" - do you plan to somehow handle it? (https://gcc.gnu.org/onlinedocs/gcc/Developer-Options.html#Developer-Options), eg: parser.c:918:5:parse_statement 48 dynamic,bounded

clang/test/CodeGen/stack-usage.c
8

-fstack-usage=file.su is Clang only, right? Tried with GCC, gcc error: unrecognized command-line option ‘-fstack-usage=xxx’,

Do we need to also specify extra -fstack-usage if -fstack-usage=file.su is used? Seems quite redudant and -fstack-usage=file.su alone should be enough.

<source_file>:<line_number>:<function_name> <size_in_byte> <static/dynamic>

gcc also supports "bounded" - do you plan to somehow handle it? (https://gcc.gnu.org/onlinedocs/gcc/Developer-Options.html#Developer-Options), eg: parser.c:918:5:parse_statement 48 dynamic,bounded

I actually tried some test cases using GCC and never got it to output "dynamic,bounded." So, not sure how (or when) GCC actually determines a function is dynamic, but bounded. Any idea?

clang/test/CodeGen/stack-usage.c
8

Yes, -fstack-usage=file.su is Clang only and it is only a cc1 option, not a driver option. The main reason why I added both flags is to distinguish between the case where the user specified "-o" and the case where "-o" is not specified.

If only -fstack-usage is passed to cc1, we know the user did not pass "-o" on the command line and the name (with the extension removed) of the source file should be used to name the .su file. For example, with "clang -fstack-usage -c foo.c", foo.su is generated.

If both -fstack-usage and -fstack-usage= are present, we know the user specified "-o" and the name of the .su file should be based on that name instead. For example, with "clang -fstack-usage -c foo.c -o bar.o", bar.su is generated instead of foo.su.

Set current_function_has_unbounded_dynamic_stack_size to 1 when pushing a variable-sized argument onto the stack. 

      if (current_function_has_unbounded_dynamic_stack_size)
	stack_usage_kind = DYNAMIC;
      else
	stack_usage_kind = DYNAMIC_BOUNDED;

https://github.com/gcc-mirror/gcc/commit/d3c12306971946ab9a9d644ddf7b26e9383d2391

You can compile eg. zstd project with "CC="gcc -fstack-usage" make -j6 -B" and then grep "bounded" . -R and you will find some examples of dynamic,bounded.

Set current_function_has_unbounded_dynamic_stack_size to 1 when pushing a variable-sized argument onto the stack. 

      if (current_function_has_unbounded_dynamic_stack_size)
	stack_usage_kind = DYNAMIC;
      else
	stack_usage_kind = DYNAMIC_BOUNDED;

https://github.com/gcc-mirror/gcc/commit/d3c12306971946ab9a9d644ddf7b26e9383d2391

You can compile eg. zstd project with "CC="gcc -fstack-usage" make -j6 -B" and then grep "bounded" . -R and you will find some examples of dynamic,bounded.

Thanks, @xbolva00! This is really helpful. I tried compiling the zstd project with both gcc and clang. I found that in all those cases I checked where gcc outputs "dynamic,bounded", clang actually outputs "static" instead. Looks like LLVM already does a better job of determining the frame size statically. So, maybe there is no need to add the "dynamic,bounded" case to clang? Thoughts?

Great if better :)

Please consider adding a small testcase (eg from zstd) where llvm says static and gcc says dynamic,bound.

I checked some of the functions in zstd where gcc outputs "dynamic,bounded", but did not find any straightforward way to simplify them into standalone tests. If anyone happen to have a simple test case, I would be more than happy to add here.

I checked some of the functions in zstd where gcc outputs "dynamic,bounded", but did not find any straightforward way to simplify them into standalone tests. If anyone happen to have a simple test case, I would be more than happy to add here.

Me neither, no problem. Not a blocker.

Thanks for all the great feedbacks, @jansvoboda11, @xbolva00 and @lebedev.ri! Is there any other concern that needs to be addressed?

xbolva00 added a comment.EditedApr 17 2021, 4:44 AM

Only nit. Added two more potential reviewers.

clang/include/clang/Basic/CodeGenOptions.h
374–379

Nit: comment? Like one in TargetOptions.h

pzheng updated this revision to Diff 338551.Apr 19 2021, 10:23 AM

Add missing comments. Thanks for spotting it, @xbolva00.

pzheng marked an inline comment as done.Apr 19 2021, 10:23 AM

This LGTM from command-line perspective, but I'll let others judge the rest of the patch.

MaskRay added inline comments.Apr 20 2021, 3:35 PM
clang/lib/Driver/ToolChains/Clang.cpp
5496

CC1 needs two options?

If you infer the filename in the driver, CC1 can use "whether StackUsageOutput is empty".

5502

"-fstack-usage=" + OutputFilename.str()

MakeArgString works with a Twine.

clang/test/CodeGen/stack-usage.c
2

Such llvm specific tests break layering but we don't have better way for testing so this might be ok...

However, not sure we need three targets. Usually one is sufficient.

4

%T is a deprecated lit replacement. Use rm -rf %t && mkdir %t

clang/test/Driver/stack-usage.c
2

Not sure we need three. One suffices.

10

Include quotes

llvm/include/llvm/CodeGen/AsmPrinter.h
186

Delete = nullptr

llvm/include/llvm/Target/TargetOptions.h
348

Delete = ""

MaskRay requested changes to this revision.Apr 26 2021, 8:28 PM
This revision now requires changes to proceed.Apr 26 2021, 8:28 PM
pzheng updated this revision to Diff 343794.May 7 2021, 6:18 PM

Addess @MaskRay's comments

pzheng marked 7 inline comments as done.May 7 2021, 6:21 PM
pzheng added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
5496

Thanks for the suggestion! Addressed this in the latest update.

pzheng marked an inline comment as done.May 7 2021, 6:22 PM
pzheng marked an inline comment as done.
pzheng updated this revision to Diff 343795.May 7 2021, 6:34 PM

Minor update to some comments.

MaskRay added inline comments.May 10 2021, 9:10 AM
clang/include/clang/Basic/CodeGenOptions.def
110 ↗(On Diff #343795)

Unneeded

clang/test/CodeGen/stack-usage.c
8

8 -> [[#@LINE+1]]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1212

"" -> ''

pzheng updated this revision to Diff 344135.May 10 2021, 11:55 AM

Address a few more comments from @MaskRay.

pzheng marked 2 inline comments as done.May 10 2021, 11:59 AM
pzheng added inline comments.
clang/include/clang/Basic/CodeGenOptions.def
110 ↗(On Diff #343795)

I tried removing this line, but TableGen doesn't seem to generate the fstack_usage flag anymore. Looks like this needs to be kept. Thoughts?

aykevl added a subscriber: aykevl.May 10 2021, 12:25 PM
MaskRay added inline comments.May 11 2021, 1:13 PM
clang/lib/Driver/ToolChains/Clang.cpp
5496

You can compute the .su filename here and pass -fstack-usage=.... to CC1.

Then you can remove the boolean codegen option Opts.StackUsage.

pzheng updated this revision to Diff 345470.May 14 2021, 9:44 AM

Address @MaskRay's comments.

pzheng marked an inline comment as done.May 14 2021, 9:46 AM
MaskRay accepted this revision.May 14 2021, 9:52 AM

LG with some nits. After fixes, please wait one day or so in case there are further comments.

clang/lib/Driver/ToolChains/Clang.cpp
5500

remove llvm::Twine

clang/test/CodeGen/stack-usage.c
5

%t/b.su can be simplified to b.su since you have changed cwd.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1189

const std::string &

otherwise there will be an unneeded copy.

This revision is now accepted and ready to land.May 14 2021, 9:52 AM
pzheng updated this revision to Diff 345498.May 14 2021, 10:49 AM

Address new comments from @MaskRay.

pzheng marked 3 inline comments as done.May 14 2021, 10:53 AM

Thanks for all the feedbacks, @MaskRay! I will wait another day before committing the change.

This revision was automatically updated to reflect the committed changes.