- When signing return addresses with -msign-return-address=<scope>{+<key>}, either the A key instructions or the B key instructions can be used. To correctly authenticate the return address, the unwinder/debugger must know which key was used to sign the return address.
- When and exception is thrown or a break point reached, it may be necessary to unwind the stack. To accomplish this, the unwinder/debugger must be able to first authenticate an the return address if it has been signed.
- To enable this, the augmentation string of CIEs has been extended to allow inclusion of a 'B' character. Functions that are signed using the B key variant of the instructions should have and FDE whose associated CIE has a 'B' in the augmentation string.
- One must also be able to preserve these semantics when first stepping from a high level language into assembly and then, as a second step, into an object file. To achieve this, I have introduced a new assembly directive '.cfi_b_key_frame ', that tells the assembler the current frame uses return address signing with the B key.
- This ensures that the FDE is associated with a CIE that has 'B' in the augmentation string.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Before I review the code itself, have you agreed the .append_augmentation directive with the GCC/GAS developers to make sure we implement it the same way?
If the design isn't yet set in stone, I have a few comments on it:
- Why doesn't it begin with ".cfi_", while every other related directive does? There's a complete list of them at https://sourceware.org/binutils/docs/as/CFI-directives.html
- Why are we adding a generic directive to append to the augmentation string, rather than a directive to select the B key? This looks very similar to .cfi_signal_frame, which also adds one character to the augmentation string.
lib/MC/MCAsmStreamer.cpp | ||
---|---|---|
1585 ↗ | (On Diff #165754) | This emits a pair of double quotes, which that parser doesn't expect. Also, there is no test for this. |
lib/MC/MCDwarf.cpp | ||
1729 ↗ | (On Diff #165754) | 80 columns (I'd suggest using git-clang-format on the patch). |
lib/MC/MCParser/AsmParser.cpp | ||
4185 ↗ | (On Diff #165754) | Outdated comment. |
4186 ↗ | (On Diff #165754) | This should probably be restricted to AArch64, ideally by putting it in the AArch64 asm parser. |
lib/MC/MCAsmStreamer.cpp | ||
---|---|---|
1582 ↗ | (On Diff #167740) | This will cause an assertion if we are not emitting unwind tables, so we need to skip this this as is currently done in AsmPrinter::emitCFIInstruction. |
- Addressed @olista01 comment on triggering an assertion when we don't emit unwind info
- The emit b key assembly directive isn't emitted when we don't emit unwind info
FYI – The following test fails if the ARM 32-bit target is NOT built alongside the 64-bit ARM target: MC/ELF/cfi-b-key-frame.s
My mistake. Something else is going on. I assumed based on past precedent that this was what was going on. (I'm not sure how many people at ARM are verifying that their AArch64 commits work when the 32-bit target is disabled).
Turns out my setup was busted and I thought I was building/testing AArch64 when I wasn't. I think you need to add // REQUIRES: aarch64-registered-target to the file.