Page MenuHomePhabricator

Support GCC's -fstack-usage flag
Needs RevisionPublic

Authored by pzheng on Wed, Apr 14, 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.Wed, Apr 14, 2:03 PM
pzheng requested review of this revision.Wed, Apr 14, 2:03 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptWed, Apr 14, 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.EditedWed, Apr 14, 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
2664

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

clang/lib/Frontend/CompilerInvocation.cpp
1925

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

1929

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

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

Address @jansvoboda11's comments.

pzheng marked 3 inline comments as done.Thu, Apr 15, 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.Thu, Apr 15, 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.EditedSat, Apr 17, 4:44 AM

Only nit. Added two more potential reviewers.

clang/include/clang/Basic/CodeGenOptions.h
391–396

Nit: comment? Like one in TargetOptions.h

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

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

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

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

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

CC1 needs two options?

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

5480

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

MakeArgString works with a Twine.

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

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.

3

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

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

Not sure we need three. One suffices.

9

Include quotes

llvm/include/llvm/CodeGen/AsmPrinter.h
179

Delete = nullptr

llvm/include/llvm/Target/TargetOptions.h
350

Delete = ""

MaskRay requested changes to this revision.Mon, Apr 26, 8:28 PM
This revision now requires changes to proceed.Mon, Apr 26, 8:28 PM