Page MenuHomePhabricator

[Driver] Add flag enabling the function stack size section that was added in r319430
ClosedPublic

Authored by seaneveson on Dec 1 2017, 4:07 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

seaneveson created this revision.Dec 1 2017, 4:07 AM
MatzeB edited edge metadata.Dec 8 2017, 1:14 PM

Looks good to me, but I'm not too familiar with the clang driver code. So it would be good if you could at least add some more experienced clang developers into the reviewer list and wait some more days before committing.

I also wonder whether it would be possible to move the fact that it defaults to on for PS4 into the PS4CPU.cpp file somehow (not that things are distributed that cleanly right now anyway).

The title says "Add cc1 option..." which to me implies a "cc1 only" option. What you're actually doing is adding a driver option. Please update the title.

test/Driver/stack-size-section.c
2 ↗(On Diff #125100)

Driver tests use -### to report the command line they'd use to invoke cc1, and verify the presence or absence of the option that you expect to see in the cc1 command line. The actual functionality test (presence or absence of something in the IR or whatever) should go in some other more appropriate place.
Also, we use FileCheck not grep.

seaneveson retitled this revision from Add cc1 flag enabling the function stack size section that was added in r319430 to [Driver] Add flag enabling the function stack size section that was added in r319430.Dec 11 2017, 2:15 AM

Improve tests.

seaneveson marked an inline comment as done.Dec 13 2017, 2:07 AM

I also wonder whether it would be possible to move the fact that it defaults to on for PS4 into the PS4CPU.cpp file somehow (not that things are distributed that cleanly right now anyway).

I see what you're saying, but I can't see an easy way to do that right now, and there are already a number of target dependent defaults done in the same way.

bruno added a subscriber: bruno.Jan 4 2018, 10:17 AM
bruno added inline comments.
lib/Driver/ToolChains/Clang.cpp
3783 ↗(On Diff #126350)

What happens when you invoke cc1 directly for ps4 and don't specify any of the two options? Is it going to default to not using stack size section? It also seems that all non ps4 targets will get -fno-stack-size-section flag by default in cc1, is it really needed?

seaneveson updated this revision to Diff 128745.Jan 5 2018, 6:35 AM

Thanks Bruno,

Changed the default for cc1 to be false regardless of the target. The default in the driver is still true for PS4 (only).

bruno accepted this revision.Jan 5 2018, 1:58 PM

Thanks! LGTM

This revision is now accepted and ready to land.Jan 5 2018, 1:58 PM
This revision was automatically updated to reflect the committed changes.
seaneveson marked an inline comment as done.