[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
Fri Mar 13 04:24:33 PDT 2020
jhenderson added a comment.
In D73086#1916681 <https://reviews.llvm.org/D73086#1916681>, @SouraVX wrote:
> > 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.
>
> Nice suggestion thanks!, but I think that should be taken as a separate exercise. what's your take on this?
As it could simplify the tests for this, it might be better to do that upfront. You could then keep a basic test to show that llvm-dwarfdump can dump both kinds of sections, but the detail could be tested in the unit tests. I don't feel strongly about it however.
================
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
----------------
jhenderson wrote:
> 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.
Still missing a blank line between `Version` and the comment for flags.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:859
+
+Expected<const DWARFDebugMacro *> DWARFContext::getDebugMacinfo() {
if (Macinfo)
----------------
SouraVX wrote:
> jhenderson wrote:
> > 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.
> Since these are mostly 5 liner functions handling their cases cleanly as needed, introducing one more function just to reduce 2-3 LOC doesn't sound good idea(personal opinion).
> Plus problem in this entire patch(due to code shared by macro/macinfo) is that, you flip one thing and you'll endup modifying everything (handling macro/macinfo, parsing/dumping) just to make sure that existing functionality is not broken.
>
The flip side argument is that you'd have to modify 3 functions any time you wanted to change the functionality somehow. It's not really about the number of lines of code. I'm not sure sharing the code is that hard. See semi-pseudo code below.
```
template <typename DWARFDebugT, typename SectionT>
static Expected<const DWARFDebugT *> getDWARFDebugMacro(
std::unique_ptr<DWARFDebugT> &MacroPtr,
SectionT *MacroSec,
bool LittleEndian,
bool IsMacro,
DataExtractor StringExtractor) {
if (MacroPtr)
return MacroPtr.get();
DWARFDataExtractor Data(MacroSec, LittleEndian, 0);
MacroPtr.reset(new DWARFDebugT());
if (Error Err = MacroPtr->parse(StringExtractor, Data, IsMacro))
return std::move(Err);
return MacroPtr.get();
}
Expected<const DWARFDebugMacro *> DWARFContext::getDebugMacinfoDWO() {
return getDWARFDebugMacro(MacinfoDWO,
DObj->getMacinfoDWOSection(),
isLittleEndian(),
/*IsMacro=*/false,
getStringExtractor());
}
...
```
================
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
----------------
jhenderson wrote:
> "debug_macro and debug_macinfo share some common encodings."
I wasn't interested in the quotes. I wanted you to change the comment to match what I posted, so that it is good English!
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:136
// 3. Macro string
- E.MacroStr = data.getCStr(&Offset);
+ // FIXME:Add support for DWARF64
+ StrOffset = Data.getRelocatedValue(/*OffsetSize=*/4, &Offset);
----------------
Nit: "FIXME: Add..." (with space after the ':')
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:4
+
+# CHECK: debug_macro contents:
+# CHECK-NEXT: 0x00000000:
----------------
jhenderson wrote:
> With my suggestion above, I'd make this line
>
> `# CHECK:debug_macro contents:`
>
> so that the colon lines up with those below it.
This hasn't been done.
================
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
----------------
SouraVX wrote:
> jhenderson wrote:
> > Why is this indented?
> emission of `DW_FORM_define_strp` form is getting checked here. Is this what you're asking ?
I was just confused by the `DW_MACRO_define_strp` causing an extra two spaces of indentation, that's all. It looks slightly broken when I read it with little prior knowledge of debug_macro sections.
================
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
----------------
SouraVX wrote:
> jhenderson wrote:
> > Is any of this block needed?
> Yes, If `Flags` in Macro header indicate `debug_line_offset` is present then it is needed. That's why here.
Okay. I'd guess that you could delete most of the strings, right?
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:49
+ .byte 1
+ .ascii "DWARF_VERSION"
+ .byte 32
----------------
SouraVX wrote:
> jhenderson wrote:
> > Nit: too many spaces here after .ascii
> In editor it was looking normal *No extra space/character*. I'll check once more.
Is it using a tab instead of a space in the text. To me, I see:
```
.byte 1
.ascii "DWARF_VERSION"
```
with the first '"' in the second line not lining up with the 1. I guess you've not written this by hand though, so if this is what the assembler produces, it's fine.
================
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
----------------
SouraVX wrote:
> jhenderson wrote:
> > Can you get rid of this block?
> Sorry this one is not needed, removed thanks!
I was also actually referring to the extra strings. It looks like you've got several duplicates that could be deleted?
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:1-2
+# This test checks that llvm-dwarfdump can dump debug_macro and debug_macinfo
+# section present in a single object.
+
----------------
In new tests, I try to use '##' to distinguish comments from lit and FileCheck directives.
can dump debug_macro -> can dump both debug_macro
section -> sections
in a single object -> in the same object
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:4
+
+# RUN: llvm-mc -triple x86_64-unknown-linux -filetype=obj -dwarf-version=5 %s -o -|\
+# RUN: llvm-dwarfdump -debug-macro - | FileCheck -strict-whitespace -match-full-lines %s
----------------
`|\` -> `| \` for readability
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-macinfo.s:5
+# RUN: llvm-mc -triple x86_64-unknown-linux -filetype=obj -dwarf-version=5 %s -o -|\
+# RUN: llvm-dwarfdump -debug-macro - | FileCheck -strict-whitespace -match-full-lines %s
+
----------------
I'd add two extra spaces before `llvm-dwarfdump` to clearly show that it is a continuation of the previous line.
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:7
+
+# CHECK:.debug_macro contents:
+# CHECK-NEXT:0x00000000:
----------------
`# CHECK:.debug_macro contents:`
same below for the other `CHECK:` line.
================
Comment at: llvm/test/DebugInfo/X86/unsupported-dwarf64-debug-macro-v5.s:1
+# This test checks llvm-dwarfdump emits correct error diagnostics for the
+# unsupported case DWARF64 flag is present in debug_macro section header.
----------------
Same comments in this test as elsewhere re '##', and line continuations.
================
Comment at: llvm/test/DebugInfo/X86/unsupported-dwarf64-debug-macro-v5.s:2
+# This test checks llvm-dwarfdump emits correct error diagnostics for the
+# unsupported case DWARF64 flag is present in debug_macro section header.
+
----------------
unsupported case where the DWARF64 flag is present in the debug_macro section header.
================
Comment at: llvm/test/DebugInfo/X86/unsupported-dwarf64-debug-macro-v5.s:6
+# RUN: not llvm-dwarfdump -debug-macro - /dev/null 2>&1 | \
+# RUN: FileCheck -strict-whitespace -match-full-lines %s
+
----------------
No real need for strict-whitespace and match-full-lines in this test case. The formatting isn't really important.
================
Comment at: llvm/test/DebugInfo/X86/unsupported-opcode_operands_table-debug-macro-v5.s:1
+# This test checks llvm-dwarfdump emits correct error diagnostics for the
+# unsupported case when opcode_operands_table flag is present in debug_macro section header.
----------------
Same comments as in other test cases.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73086/new/
https://reviews.llvm.org/D73086
More information about the llvm-commits
mailing list