[PATCH] D72828: [DWARF5] Added support for emission of debug_macro section.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Please implement the dwarfdump support first (this patch can be reviewed during that time - but shouldn't be committed without tests & ideally those tests should be written using dwarfdump (sometimes there are exceptions for especially complicated dumping situations, where raw assembly testing is used instead, but I don't think it's necessary to make that tradeoff here))

& I'm surprised this patch causes updates to the tests - what changes in the patch caused changes in these tests? (stringpool and dbg-stack-value-range.mir) - ah, because debug_str now needs to be emitted after debug_macro because the macro section might introduce new strings? OK. Could you separate that ordering change/test update out into a standalone patch. It'll make the rest of this more targeted when reviewing.



================
Comment at: llvm/lib/BinaryFormat/Dwarf.cpp:464-475
+      .Case("DW_MACRO_define", DW_MACRO_define)
+      .Case("DW_MACRO_undef", DW_MACRO_undef)
+      .Case("DW_MACRO_start_file", DW_MACRO_start_file)
+      .Case("DW_MACRO_end_file", DW_MACRO_end_file)
+      .Case("DW_MACRO_define_strp", DW_MACRO_define_strp)
+      .Case("DW_MACRO_undef_strp", DW_MACRO_undef_strp)
+      .Case("DW_MACRO_import", DW_MACRO_import)
----------------
Build this with Dwarf.def rather than writing it out explicitly. Something like this:

```
return StringSwitch<unsigned>(MacroString)
#define HANDLE_DW_MACRO(ID, NAME)
  .Case("DW_MACRO_" #NAME, ID)
#include "llvm/BinaryFormat/Dwarf.def"
  .Default(DW_MACINFO_invalid);
```


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2779
+    DEBUG_LINE_OFFSET = 2,
+    OPCODE_OPERANDS_TABLE = 3
+  };
----------------
This should probably be 4, not 3, since it's a bit field column. (if you tried to | in OPCODE_OPERANDS_TABLE of 3 into the table, you'd overwrite bits 1 and 2 (changing the value of offset_size and debug_line_offset, rather than specifying the value of opcode_operands))


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2785-2786
+  Asm->emitInt16(5);
+  if (DwarfParams.getDwarfOffsetByteSize() == 4)
+    Flags |= OFFSET_SIZE_32;
+  if (Asm->getObjFileLowering().getDwarfLineSection())
----------------
This looks to be backwards - since OFFSET_SIZE_32 is zero, these two lines don't have any effect (& 32 bit is the default - if the bit is zero).

So it should /probably/ be:

```
if (offset byte size == 8)
  Flags |= OFFSET_SIZE_32;
```


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2787-2788
+    Flags |= OFFSET_SIZE_32;
+  if (Asm->getObjFileLowering().getDwarfLineSection())
+    Flags |= DEBUG_LINE_OFFSET;
+  // FIXME: We are not setting opcode_operands_table_flag, since we're not
----------------
Seems this line offset is optional and the DWARF spec doesn't mention what it might be useful for. Do you have any specific use in mind? Otherwise I'd leave it out. (@aprantl @probinson )


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2789-2790
+    Flags |= DEBUG_LINE_OFFSET;
+  // FIXME: We are not setting opcode_operands_table_flag, since we're not
+  // emitting opcode_operands_table
+  Asm->OutStreamer->AddComment("Flags: 32 bit, lineptr present");
----------------
I don't think this is a FIXME situation - LLVM has no extensions here, so there's no need for the opcode_operands_table.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2819-2827
+    if (M.getMacinfoType() == llvm::dwarf::DW_MACINFO_define) {
+      Asm->OutStreamer->AddComment(
+          dwarf::MacroString(dwarf::DW_MACRO_define_strp));
+      Asm->EmitULEB128(dwarf::getMacro("DW_MACRO_define_strp"));
+    } else {
+      Asm->OutStreamer->AddComment(
+          dwarf::MacroString(dwarf::DW_MACRO_undef_strp));
----------------
Perhaps refactor this to only have "AddComment / EMitULEB128" written once, but parameterized on the enum, eg:

```
unsigned Type = M.getMacinfoType() == dwarf::DW_MACINFO_define ? dwarf::DW_MACRO_define_strp : dwarf::DW_MACRO_undef_strp;
Asm->OutStreamer->AddComment(dwarf::MacroString(Type));
Asm->EmitULEB128(Type);
```


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2868
+    Asm->OutStreamer->AddComment(dwarf::MacroString(dwarf::DW_MACRO_end_file));
+    Asm->EmitULEB128(dwarf::getMacro("DW_MACRO_end_file"));
+  } else {
----------------
Parsing constant strings back into constants is a bit suboptimal (both in terms of compile time performance, chance of mistakes (the string is checked at runtime whereas a reference to an enum would be checked at compile time)) & is curiously inverted compared to the line before - which uses an enum, then converts that to a string for the comment. There are a few other cases of this - in general, don't write string constants like this due to the lack of error/type checking, prefer using the enums & calling stringifying functions if needed.

It'd be nice to simplify this code a bit in general - the fact that this adds comments to the new debug_macro paths that are then missing from the debug_macinfo paths, etc, isn't ideal.

I'm wondering if it'd make sense to instead phrase this whole function parameterized on the type of enums, so it'd be implemented a bit like this:

```
if (UseMacro) {
  emitMacroFileImpl(F, U, dwarf::DW_MACRO_start_file, dwarf::DW_MACRO_end_file, dwarf::MacroString);
} else {
  emitMacroFileImpl(F, U, dwarf::DW_MACINFO_start_file, dwarf::DW_MACINFO_end_file, dwarf::MacinfoString);
}
```

So the implementation doesn't have conditionals in several places, keeps it consistent, etc.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2895-2899
+  if (getDwarfVersion() >= 5) {
+    emitDebugMacinfoImpl(Asm->getObjFileLowering().getDwarfMacroSection());
+    return;
+  }
   emitDebugMacinfoImpl(Asm->getObjFileLowering().getDwarfMacinfoSection());
----------------
This might be more legible using the conditional (?:) operator to pass the specific debug section - so it's obvious to the reader that that's the only difference (that they're not calling subtly differently named functions, etc - just calling the same function with a different argument), eg:

```
auto &ObjLower = Asm->getObjFileLowering();
emitDebugMacinfoImpl(getDwarfVersion() >= 5 ? ObjLower.getDwarfMacroSection() : ObjLower.getDwarfMacInfoSection());
```


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

https://reviews.llvm.org/D72828