[PATCH] D50136: [AArch64] - Return address signing dwarf support

[Oliver Stannard via Phabricator via llvm-commits]

olista01 added inline comments.


================
Comment at: include/llvm/BinaryFormat/Dwarf.def:90
+#ifndef HANDLE_DW_CFA_DUP
+#define HANDLE_DW_CFA_DUP(ID, NAME, PRED)
+#endif
----------------
Maybe HANDLE_DW_CFA_TARGET or HANDLE_DW_CFA_PRED would be a better name for this? 


================
Comment at: include/llvm/BinaryFormat/Dwarf.def:839
 // Vendor extensions:
 HANDLE_DW_CFA(0x1d, MIPS_advance_loc8)
 HANDLE_DW_CFA(0x2d, GNU_window_save)
----------------
Now that we have a way to represent target-specific opcodes, should we limit these to their respective targets? I think that GNU_window_save is PowerPC-specific, and GNU_args_size is X86-specific.


================
Comment at: lib/BinaryFormat/Dwarf.cpp:461
+    Triple::ArchType Arch) {
+#define SELECT_AARCH64 Arch == llvm::Triple::aarch64
+#define HANDLE_DW_CFA(ID, NAME)
----------------
Should this be #undef'd after these includes? The other macros are cleared at the end of Dwarf.def, but this one isn't.


================
Comment at: lib/BinaryFormat/Dwarf.cpp:461
+    Triple::ArchType Arch) {
+#define SELECT_AARCH64 Arch == llvm::Triple::aarch64
+#define HANDLE_DW_CFA(ID, NAME)
----------------
olista01 wrote:
> Should this be #undef'd after these includes? The other macros are cleared at the end of Dwarf.def, but this one isn't.
I think this also needs to check Triple::aarch64_be.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:5290
 
+bool AArch64AsmParser::parseDirectiveCFINegateRAState() {
+  getStreamer().EmitCFINegateRAState();
----------------
The assembly parsing isn't covered by any of the tests.


================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:5292
+  getStreamer().EmitCFINegateRAState();
+  return false;
+}
----------------
Do we need to check that we are at AsmToken::EndOfStatement (as is done for other directives), or is that checked somewhere else?


================
Comment at: tools/llvm-readobj/DwarfCFIEHPrinter.h:190
       ELFT::Is64Bits ? 8 : 4);
-  DWARFDebugFrame EHFrame(/*IsEH=*/true, /*EHFrameAddress=*/Address);
+  DWARFDebugFrame EHFrame(Triple::ArchType(ObjF->getArch()), /*IsEH=*/true, /*EHFrameAddress=*/Address);
   EHFrame.parse(DE);
----------------
Long line, please clang-format this.


https://reviews.llvm.org/D50136