[PATCH] D114545: [CodeGen] Async unwind - add a pass to fix CFI information

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

Looks great! Thanks again for the hard work! And sorry again for being so slow to get to this important pass :-(



================
Comment at: llvm/include/llvm/CodeGen/CFIFixup.h:1
+//===------ CFIFixup.h - Insert CFI remember/restore instructions ---------===//
+//
----------------
"C++" is missing. Hyphens on the left are too long. See https://llvm.org/docs/CodingStandards.html#file-headers


================
Comment at: llvm/lib/CodeGen/CFIFixup.cpp:166
+  // No point starting before the prologue block.
+  // TODO: the unwind tables will still be incorrect if an epilogue physically
+  // preceeds the prologue.
----------------
chill wrote:
> MaskRay wrote:
> > Delete the TODO: I believe this is not a TODO, but an assumption. The stack based remember/restore approach just cannot handle epilogue before prologue in theory.
> > 
> > This is what I wanted to express but probably not in a clear way:
> > "If we consider (a) a prologue, (b) an epilogue, or (c) a region delimitered by a pair of prologue/epilogue, as a vertex in a graph,
> > the algorithm can handle any linear basic block layout which is a pre-order traversal.
> > (This condition is sufficient but not necessary: if we require that all prologues are the same, more traversal orders can be handled)"
> I've moved it to the start of the file. If an actual need appears, I think it's implementable
> and the pass need not restrict itself to remember/restore state.
If an epilogue proceeds the associated prologue, the pass needs to copy the prologue state and remember/restore cannot be used. This is a case I'd call an assumption instead of a TODO since implementing it seems has nearly no advantage, at the cost of large complexity. OK, you make the call whether the TODO should remain here:)


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:301
 static StackOffset getSVEStackSize(const MachineFunction &MF);
+static bool needsShadowCallStackPrologueEpilogue(MachineFunction &MF);
 
----------------
Optional: wonder whether this can be avoided by moving  AArch64FrameLowering::resetCFIToInitialState after the body of needsShadowCallStackPrologueEpilogue


================
Comment at: llvm/test/CodeGen/AArch64/arm64-shrink-wrapping.ll:29
 ; ENABLE-NEXT:  LBB0_2: ; %false
+; ENABLE-NEXT:    .cfi_def_cfa wsp, 0
+; ENABLE-NEXT:    .cfi_same_value w18
----------------
chill wrote:
> MaskRay wrote:
> > Thanks for the update to `foo`. It makes me understand the purpose of `StrongNoFrameOnEntry`. Right now there is no epilogue CFI, so BB1's `HasFrameOnExit` is (incorrectly) false. We rely on  `StrongNoFrameOnEntry` being true to generate `.cfi_def_cfa wsp, 0` and `.cfi_same_value *`.
> > 
> > However, using `MF.getFrameInfo().getCalleeSavedInfo();` has the problem that we may add redundant `.cfi_same_value`. As this test shows, none of w18/w30/w29 is updated in previous blocks but we add them: (a) it unnecessarily increases unwind table size (b) it may confuse a reader.
> > 
> > We may need to check whether the registers have actually been modified by previous CFI instructions, before emitting `.cfi_same_value`.
> These (except for `x18`) are not redundant. At `LBB0_2` the unwind info says the CSR are on the stack, but they aren't there.
> 
> `x18` is because of it's potential use as a shadow call stack pointer, I've fixed the code to emit `.cfi_same_value x18` only if the function indeed uses shadow stack.
Thanks. I did not notice .cfi_offset w30 yesterday. My bad. Seems that there is some mechanism to not emit .cfi_same_value for unreferenced registers. Sounds good.


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

https://reviews.llvm.org/D114545