This is an archive of the discontinued LLVM Phabricator instance.

[stack protector] Improved data layout rules, part 4
ClosedPublic

Authored by jmagee on Jan 13 2014, 6:31 PM.

Details

Summary

Hi,

This is the third part in a series of patches that will update the stack layout
rules for SSP, specifically to support the ssp-strong rules.

The breakdown of the series is:

Patch 1 [DONE]: Split the StackProtector pass from a single cpp file,
StackProtector.cpp, into StackProtector.h and StackProtector.cpp.

Patch 2 [DONE]: Update the StackProtector pass to do the datalayout analysis.

Patch 3 [DONE]: Use the new layout analysis to replicate the old layout behaviour.

Patch 4 [This patch]: Implements the stricter layout rules for sspstrong and sspreq.

  • This patch changes the PrologueEpilogInserter and LocalStackSlotAllocation passes to follow the extended stack layout rules for sspstrong and sspreq.
  • Included are the documentation updates and final tests.

Thanks,
Josh

Diff Detail

Event Timeline

Ping.

Note, in my original summary I said this was the "third part" of the series, but it is really the fourth and final part of my SSP stack layout patch series.

  • Josh

Ping. (On a Friday!)

void added a comment.Jan 31 2014, 2:08 PM

See comments inline.

docs/LangRef.rst
1064 ↗(On Diff #6440)

Please use a monospaced font for '>= ssp-buffer-size'. here and elsewhere.

1069 ↗(On Diff #6440)

If these are order of how it's determined where an array goes, please number the list.

lib/CodeGen/LocalStackSlotAllocation.cpp
198 ↗(On Diff #6440)

Please use one variable per declaration line:

StackObjSet LargeArrayObjs;
StackObjSet SmallArrayObjs;
StackObjSet AddrOfObjs;

lib/CodeGen/PrologEpilogInserter.cpp
547 ↗(On Diff #6440)

Is this code identical to the one in CodeGen/LocalStackSlotAllocation.cpp? If so, please refactor it.

557 ↗(On Diff #6440)

Same comment here as above. One variable per declaration please.

See my inline comment in response to the refactoring. All the rest of the comments look good - I'll make those changes.

lib/CodeGen/PrologEpilogInserter.cpp
547 ↗(On Diff #6440)

A large amount of the code in LocalStackSlotAllocation.cpp is identical to PrologEpilogInserter.cpp - there is significant refactoring that can be done there. It is not easy to draw the line...

I'm happy to refactor the stack-protector stuff out as a first step. I'm also happy to continue that effort to refactor out much of the duplication in these passes - the duplication always puzzled (and bothered) me a bit.

I'd prefer to do the refactoring as subsequent patches. Does that sound reasonable?

void accepted this revision.Jan 31 2014, 3:34 PM

LGTM with above discussed changes.

lib/CodeGen/PrologEpilogInserter.cpp
547 ↗(On Diff #6440)

A subsequent patch is fine, it's not like this is your fault or something. :-) I think that any significant refactoring you can do here would be an overall benefit. Thanks!

jmagee closed this revision.Jan 31 2014, 5:42 PM

Closed by commit rL200601 (authored by @jmagee).

Thanks for the review! I'll get cracking on those PEI/LocalStackSlot refactors.