This is an archive of the discontinued LLVM Phabricator instance.

Reland [AArch64][MachineOutliner] Return address signing for outlined functions
ClosedPublic

Authored by tellenbach on Nov 23 2019, 9:47 AM.

Details

Summary

Reland after fixing a bug that allowed outlining of SP modifying instructions
that invalidated return address signing.

During AArch64 frame lowering instructions to enable return address
signing are inserted into functions if needed. Functions generated during
machine outlining don't run through target frame lowering and hence are
missing such instructions.

This patch introduces the following changes:

  1. If not all functions that potentially participate in function outlining agree on their return address signing scope and their return address signing key, outlining is disabled for these functions.
  2. If not all functions that potentially participate in function outlining agree on their support for v8.3A features, outlining is disabled for these functions.
  3. If an outlining candidate would outline instructions that modify sp in a way that invalidates return address signing, outlining is disabled for that particular candidate.
  4. If all candidate functions agree on the signing scope, signing key and their support for v8.3 features, the outlined function behaves as if it had the same scope and key attributes and as if it would provide the same v8.3A support as the original functions.

Event Timeline

tellenbach created this revision.Nov 23 2019, 9:47 AM

Add newline at eof

tellenbach edited the summary of this revision. (Show Details)Nov 23 2019, 10:02 AM
tellenbach added reviewers: ostannard, paquette.
tellenbach marked an inline comment as done.Nov 23 2019, 10:05 AM
tellenbach added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
5209

This fixes the bug in the original patch (https://reviews.llvm.org/D69097).

ostannard added inline comments.Nov 29 2019, 3:23 AM
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
5209

I think moving this to getOutliningType would be better, as that applies to individual instructions, not the whole sequence, so we might still be able to find a shorter sequence which can be outlined.

tellenbach marked an inline comment as done.Dec 2 2019, 6:00 AM
tellenbach added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
5209

As you've mentioned, getOutliningType considers single instructions, therefore matching sp modifying add and sub instructions would be unnecessarily removed.

Furthermore, getOutliningType runs before we know if an outlined function would need return address signing.

ostannard accepted this revision.Dec 2 2019, 9:34 AM

LGTM

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
5209

Ah, of course.

This revision is now accepted and ready to land.Dec 2 2019, 9:34 AM
This revision was automatically updated to reflect the committed changes.
tellenbach marked 2 inline comments as done.

Thanks for the review!

scott.linder added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
5297

You may already be aware, but there is a container-overflow here in the ASan build (see http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/37147/steps/check-llvm%20asan/logs/stdio for the failure). Seems like RepeatedSequenceLocs can be empty here.

tellenbach marked an inline comment as done.Dec 4 2019, 2:52 PM
tellenbach added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
5297

Thanks for your comment. I'll have a look!

I have reverted this due to the asan failures. f65267e is the commit.

tellenbach updated this revision to Diff 232237.Dec 4 2019, 4:56 PM

@saugustine Thanks, I was just about to fix it with this patch but running the asan tests locally took longer than expected.

Do I need new review for this?

tellenbach updated this revision to Diff 232239.Dec 4 2019, 5:02 PM
tellenbach marked 2 inline comments as done.
tellenbach added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
5248

This fixes the Asan failure.

@saugustine Thanks, I was just about to fix it with this patch but running the asan tests locally took longer than expected.

Do I need new review for this?

I wouldn't think so. It's been reviewed twice now and will be a useful change once it sticks. So conisder me accepting it.

@saugustine Thanks, I was just about to fix it with this patch but running the asan tests locally took longer than expected.

Do I need new review for this?

I wouldn't think so. It's been reviewed twice now and will be a useful change once it sticks. So conisder me accepting it.

Thanks! Committed here: cec2d5c17457

tellenbach reopened this revision.EditedDec 11 2019, 8:52 AM

Reopening this since it's still not correct.

The current implementation has a bug in calculating legal sp modifying instructions: It currently accepts e.g. $sp = ADDXri $fp, 0, 0 as legal but only instructions such as $sp = ADDXri $sp, 0, 0 are okay (since the overall change to $sp is 0).

Thanks again, @ostannard for catching this.

This revision is now accepted and ready to land.Dec 11 2019, 8:52 AM
tellenbach planned changes to this revision.Dec 11 2019, 8:52 AM

hasIllegalSPModification now only accepts matching sp modifying add and sub instructions of if they just increment/decrement sp.

PACIASP
...
$sp = ADDXri $sp, 16, 0    // Incrementing sp modification
...
$sp = SUBXri $sp, 16, 0    // Matching decrementing sp modification
...
AUTIASP
RET

or

PACIASP
...
$sp = ADDXri $sp, 0, 0   // Incrementing sp modification (sp is unchanged)
...
AUTIASP
RET

are okay but e.g. $sp = ADDXri $x8, 0, 0 is illegal.

This revision is now accepted and ready to land.Dec 11 2019, 9:33 AM
tellenbach requested review of this revision.Dec 11 2019, 9:35 AM
ostannard accepted this revision.Dec 12 2019, 1:00 AM

LGTM, thanks.

This revision is now accepted and ready to land.Dec 12 2019, 1:00 AM
This revision was automatically updated to reflect the committed changes.