[PATCH] D69097: [AArch64][MachineOutliner] Return address signing for outlined functions

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

I think this needs some more tests. In particular, I'd like to see some test cases that show what happens when

- We tail call
- We don't save LR to the stack
- We get an outlined thunk

In the case of tail calls, I //think// that we probably don't want to sign, since we'll get a `b` instead of a `bl`, and we'll never modify the stack. This could save some size overhead too.

In the case of not saving LR to the stack, I think we probably have a similar case.

I haven't thought too hard about outlined thunks but I wouldn't be surprised if there's some similar weirdness.

Also another thing to consider here is whether or not we should ever outline, say, PAC/AUT instructions. It might be good to mark those as illegal to outline. That will also need a test. I think that writing a MIR test should make this fairly easy to check for.



================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5116-5117
+          [](const outliner::Candidate &a, const outliner::Candidate &b) {
+            if (outliningCandidatesSigningScopeConsensus(a, b) &&
+                outliningCandidatesSigningKeyConsensus(a, b)) {
+              return false;
----------------
I find "consensus" kind of confusing to use here. I'd expect it to return true when they agree, not when they disagree. Maybe something like "disagree" would be better to use here?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5635
 
+  bool isLeafFunction = true;
+
----------------
I'd use `IsLeafFunction` versus `isLeafFunction` for the sake of consistency with the rest of the code.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5707-5714
+    else {
+      // If "sign-return-address"="non-leaf" we only sign if the functions
+      // spills LR. Since the new machine function has not run through the
+      // normal frame lowering we check this by checking
+      if (Scope.equals("non-leaf")) {
+        if (!isLeafFunction) {
+          ShouldSignReturnAddr = true;
----------------
Simplify?

```
else if (Scope.equals("non-leaf") && !isLeafFunction)
  ShouldSignReturnAddr = true;
```


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5735-5737
+    if (MBBAUT != MBB.end()) {
+      DL = MBBAUT->getDebugLoc();
+    }
----------------
Prefer

```
if (MBBAUT != MBB.end())
  DL = MBBAUT->getDebugLoc();
```

I think clang-format should probably get this?


================
Comment at: llvm/test/CodeGen/AArch64/machine-outliner-retaddr-sign-diff-scope-same-key.ll:1
+; RUN: llc -verify-machineinstrs -enable-machine-outliner -mtriple \
+; RUN: aarch64-arm-none-eabi %s -o - | FileCheck %s
----------------
Can you use MIR tests here? That would make it easier to test each frame variant with signing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69097/new/

https://reviews.llvm.org/D69097