[PATCH] D73086: [WIP][DWARF5] Added support for debug_macro section parsing and dumping in llvm-dwarfdump.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 11 03:03:55 PDT 2020
jhenderson added a comment.
Something you may wish to consider is writing the tests as unit tests rather than using dwarfdump to test them, since the code is in a library. It may require a little extra legwork to set up initially (writing dwarf generator code, like there already is for debug info and debug line), but I think there is a decent payoff in return. It'll allow you to avoid the noise that some of these tests have.
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:279
+
+ /// Get a pointer to the parsed Macinfo inforamtion object.
+ Expected<const DWARFDebugMacro *> getDebugMacinfo();
----------------
Typo: information
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:23
+class DWARFContext;
class DWARFDebugMacro {
+ /// DWARFv5 section 6.3.1 Macro Information Header
----------------
Have a blank line between the forward declarations and the class definition, like there was before.
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:24
class DWARFDebugMacro {
+ /// DWARFv5 section 6.3.1 Macro Information Header
+ enum HeaderFlagMask {
----------------
Missing trailing full stop.
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:30
+ struct MacroHeader {
+ /// Macro information number.
+ uint16_t Version = 0;
----------------
version information?
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:32
+ uint16_t Version = 0;
+ /// flags
+ /// The bits of the flags field are interpreted as a set of flags, some of
----------------
I'm not sure this line of the comment is useful at all.
Also, include a blank line after the previous member, and not after this comment:
```
uint16_t Version = 0;
/// Big comment about flags
/// ...
uint8_t Flags;
```
Similar comment goes elsewhere in this file.
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:35
+ /// which may indicate that additional fields follow. The following flags,
+ /// beginning with the least significant bit, are defined: offset_size_flag:
+ /// If the offset_size_flag is zero, the header is for a 32-bit DWARF
----------------
Put "offset_size_flag:" on a new line, like the other flags.
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:50
+ /// An offset in the .debug_line section of the beginning of the line
+ /// number information in the containing compilation, encoded as a 4-byte
+ /// offset for a 32-bit DWARF format macro section and an 8-byte offset
----------------
compilation -> compilation unit?
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:52
+ /// offset for a 32-bit DWARF format macro section and an 8-byte offset
+ /// for a 64-bit DWARF format macro section
+ uint64_t DebugLineOffset;
----------------
Nit: missing trailing full stop.
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:55-57
+ uint16_t getVersion() const { return Version; }
+ uint8_t getFlags() const { return Flags; }
+ uint64_t getDebugLineOffset() const { return DebugLineOffset; }
----------------
The getters seem completely superfluous, if you've made the members all public. I'd recommend getting rid of them, or making the members private.
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:64
+ Error parseMacroHeader(DWARFDataExtractor data, uint64_t *Offset);
+ };
/// A single macro entry within a macro list.
----------------
Add a blank line after the end of the struct definition.
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:103
+ /// parameter.
+ Error parse(DataExtractor StringExtractor, DWARFDataExtractor data,
+ bool IsMacro);
----------------
data -> Data
(and update the comment accordingly).
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:449
+ DObj->getMacroSection().Data)) {
+ Expected<const DWARFDebugMacro *> ExpectedMacro = getDebugMacro();
+ if (!ExpectedMacro)
----------------
It might make sense for this code to follow a similar pattern to the .debug_line code below, with an extra layer that allows parsing and dumping of individual blocks. That way you can say "dump the block starting at offset 0x1000" or whatever.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:841
MacinfoDWO.reset(new DWARFDebugMacro());
- MacinfoDWO->parse(MacinfoDWOData);
+ if (auto Err = MacinfoDWO->parse(this->getStringExtractor(), MacinfoDWOData,
+ /*IsMacro=*/false))
----------------
Don't use `auto` unless the type is obvious from the right-hand-side of the assignment. Be explicit with this being `Error`.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:853
+ Macro.reset(new DWARFDebugMacro());
+ if (auto Err = Macro->parse(this->getStringExtractor(), MacroData,
+ /*IsMacro=*/true))
----------------
`auto` -> `Error`
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:859
+
+Expected<const DWARFDebugMacro *> DWARFContext::getDebugMacinfo() {
if (Macinfo)
----------------
These three functions all look very similar. I wonder if it makes sense to create a small helper that does the common stuff. It could take a reference to the unique_ptr, the `IsMacro` boolean etc as needed.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:56
break;
- case DW_MACINFO_define:
- case DW_MACINFO_undef:
+ // debug_macro/debug_macinfo shares couple of common encodings.
+ // DW_MACRO_define == DW_MACINFO_define
----------------
"debug_macro and debug_macinfo share some common encodings."
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:61
+ // DW_MACRO_end_file == DW_MACINFO_end_file
+ // For readibility/uniformity we are using DW_MACRO_*.
+ case DW_MACRO_define:
----------------
readability
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:112
+ uint64_t strOffset = 0;
switch (E.Type) {
----------------
`StrOffset` to match LLVM coding style.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:138
+ // FIXME:Add support for DWARF64
+ strOffset = data.getRelocatedValue(4 /*Offset Size*/, &Offset);
+ E.MacroStr = StringExtractor.getCStr(&strOffset);
----------------
`/*OffsetSize=*/4`
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:160
+
+Error DWARFDebugMacro::MacroHeader::parseMacroHeader(DWARFDataExtractor data,
+ uint64_t *Offset) {
----------------
`Data`.
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:1
+# RUN: llvm-mc -triple x86_64-unknown-linux --filetype=obj -dwarf-version=5 %s -o -|\
+# RUN: llvm-dwarfdump -v - | FileCheck %s
----------------
Add a comment at the top of this test explaining what this test is about.
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:2
+# RUN: llvm-mc -triple x86_64-unknown-linux --filetype=obj -dwarf-version=5 %s -o -|\
+# RUN: llvm-dwarfdump -v - | FileCheck %s
+
----------------
You should have a verbose and non-verbose version of this test, as the printing is usually different.
Also, consider adding --strict-whitespace and --match-full-lines to the FileCheck call to ensure the whole content is checked and spacing is correct.
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:4
+
+# CHECK: debug_macro contents:
+# CHECK-NEXT: 0x00000000:
----------------
With my suggestion above, I'd make this line
`# CHECK:debug_macro contents:`
so that the colon lines up with those below it.
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:8
+# CHECK-NEXT: DW_MACRO_start_file - lineno: 0 filenum: 0
+# CHECK-NEXT: DW_MACRO_define_strp - lineno: 1 macro: DWARF_VERSION 5
+# CHECK-NEXT: DW_MACRO_end_file
----------------
Why is this indented?
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:30-39
+ .section .debug_str,"MS", at progbits,1
+.Linfo_string0:
+ .asciz "clang version 11.0.0" # string offset=0
+.Linfo_string1:
+ .asciz "a-v5.c" # string offset=105
+.Linfo_string2:
+ .asciz "/macro" # string offset=112
----------------
Is any of this block needed?
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:45
+ .byte 0
+ .file 1 "/macro" "a-v4.c"
+ .byte 1
----------------
Is this line relevant?
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:49
+ .byte 1
+ .ascii "DWARF_VERSION"
+ .byte 32
----------------
Nit: too many spaces here after .ascii
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:55-63
+ .section .debug_str,"MS", at progbits,1
+.Linfo_string4:
+ .asciz "clang version 11.0.0" # string offset=0
+.Linfo_string5:
+ .asciz "a-v4.c" # string offset=105
+.Linfo_string6:
+ .asciz "/macro" # string offset=112
----------------
Can you get rid of this block?
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:1
+# RUN: llvm-mc -triple x86_64-unknown-linux --filetype=obj -dwarf-version=5 %s -o -|\
+# RUN: llvm-dwarfdump --debug-macro - | FileCheck %s
----------------
Same comments apply in this test as in the previous one.
Also, use single or double dashes consistently for the entire llvm-mc call.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73086/new/
https://reviews.llvm.org/D73086
More information about the llvm-commits
mailing list