[PATCH] D73086: [DWARF5] Added support for debug_macro section parsing and dumping in llvm-dwarfdump.
Igor Kudrin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 5 07:40:47 PST 2020
ikudrin added a comment.
The test should be extended to use all supported kinds of entries. There should also be another macro unit without the `debug_line_offset` field. There should be tests for other combinations of flags in the header to show that the correct diagnostics for unsupported flags are generated.
================
Comment at: llvm/include/llvm/BinaryFormat/Dwarf.def:20
defined HANDLE_DW_LNCT || defined HANDLE_DW_MACRO || \
+ defined HANDLE_MACRO_FLAGS || \
defined HANDLE_DW_RLE || defined HANDLE_DW_LLE || \
----------------
It should be `HANDLE_MACRO_FLAG`, without `S`, to correspond to other definitions here.
================
Comment at: llvm/include/llvm/BinaryFormat/Dwarf.def:829
HANDLE_DW_MACRO(0x0c, undef_strx)
+// DWARF v5 Macro header flags
+HANDLE_MACRO_FLAGS(0x01, OFFSET_SIZE)
----------------
Add an empty line before the definitions.
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:277
/// Get a pointer to the parsed DebugMacro object.
+ const DWARFDebugMacro *getDebugMacro();
const DWARFDebugMacro *getDebugMacinfo();
----------------
Every getter here has its own comment. Please, declare the new method in the same way.
Please, describe the differences between `getDebugMacro()` and `getDebugMacinfo()` in the corresponding comments.
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:47
+
+ // FIXME: add support for 64-bit DWARF;
+ uint8_t Flags;
----------------
This `FIXME` makes no sense because there is nothing to fix here. Please, remove it.
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:55
+ uint64_t DebugLineOffset;
+ // FIXME: add support for opcode_operands_table_flag.
+ uint16_t getVersion() const { return Version; }
----------------
How does this `FIXME` correspond to `getVersion()`? Please, move it to an appropriate place.
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h:62
+ /// Parse the debug_macro header.
+ Expected<bool> parseMacroHeader(DWARFDataExtractor data, uint64_t *Offset);
+ };
----------------
The return value, `bool`, is never used; moreover, this method never returns `false`. Use `Error` as the result type and return `Error::success()` to indicate success.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:56
break;
case DW_MACINFO_define:
case DW_MACINFO_undef:
----------------
The mix of `DW_MACINFO_` and `DW_MACRO_` constants in one switch looks untidy. How about using only `DW_MACRO_` set, and adding comments and/or `static_assert`s to show that the numeric values are the same?
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:80
+void DWARFDebugMacro::parse(DWARFContext &Context, DWARFDataExtractor data) {
+ uint64_t strOffset = 0;
uint64_t Offset = 0;
----------------
Please, move the declaration to the place where the variable is used.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:88
M->Offset = Offset;
+ if (Context.getMaxVersion() >= 5) {
+ Expected<bool> Status = M->Header.parseMacroHeader(data, &Offset);
----------------
This prevents parsing .debug_macinfo sections if an input file has a mix of debugging information with different versions. The parsing method should explicitly distinct two kinds of macro section. Maybe it will be more clear if there are two distinct parsing methods.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:89
+ if (Context.getMaxVersion() >= 5) {
+ Expected<bool> Status = M->Header.parseMacroHeader(data, &Offset);
+ if (!Status) {
----------------
It looks like you do not initialize the header for `.debug_macinfo` sections, but use `Header.getVersion()` in `DWARFDebugMacro::dump()`. Please, fix.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:91
+ if (!Status) {
+ WithColor::error() << toString(std::move(Status.takeError())) << '\n';
+ return;
----------------
This line causes a warning with clang++-9:
```
.../DWARFDebugMacro.cpp:91:42: warning: moving a temporary object prevents copy elision [-Wpessimizing-move]
WithColor::error() << toString(std::move(Status.takeError())) << '\n';
^
.../DWARFDebugMacro.cpp:91:42: note: remove std::move call here
WithColor::error() << toString(std::move(Status.takeError())) << '\n';
^~~~~~~~~~ ~
1 warning generated.
```
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:91
+ if (!Status) {
+ WithColor::error() << toString(std::move(Status.takeError())) << '\n';
+ return;
----------------
ikudrin wrote:
> This line causes a warning with clang++-9:
> ```
> .../DWARFDebugMacro.cpp:91:42: warning: moving a temporary object prevents copy elision [-Wpessimizing-move]
> WithColor::error() << toString(std::move(Status.takeError())) << '\n';
> ^
> .../DWARFDebugMacro.cpp:91:42: note: remove std::move call here
> WithColor::error() << toString(std::move(Status.takeError())) << '\n';
> ^~~~~~~~~~ ~
> 1 warning generated.
> ```
Parsing methods should not print diagnostics, just pass errors to the callers.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:109
switch (E.Type) {
default:
----------------
This switch also mixes constants from different sets. Please, tide it up.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:160
+ }
+ Flags = Flagdata;
+ if (Flags & MACRO_DEBUG_LINE_OFFSET)
----------------
You should also check for the `MACRO_OPCODE_OPERANDS_TABLE` flag and issue an error.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:162
+ if (Flags & MACRO_DEBUG_LINE_OFFSET)
+ DebugLineOffset = data.getU32(Offset);
+ return true;
----------------
Add `// FIXME: Add support for DWARF64`.
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:1
+# RUN: llvm-mc --filetype=obj -dwarf-version=5 %s -o %t.o
+# RUN: llvm-dwarfdump -v %t.o | FileCheck %s
----------------
You probably should add `-triple x86_64-unknown-linux` or alike, otherwise, the test will probably fail on Windows.
There is no need for a temporary file, you may just redirect the output to the input of `llvm-dwarfdump`:
```
# RUN: llvm-mc -triple x86_64-unknown-linux -filetype=obj -dwarf-version=5 %s -o - | \
# RUN: llvm-dwarfdump -debug-macro - | FileCheck %s
```
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:2
+# RUN: llvm-mc --filetype=obj -dwarf-version=5 %s -o %t.o
+# RUN: llvm-dwarfdump -v %t.o | FileCheck %s
+
----------------
Do not dump everything, just `-debug-macro`.
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:19
+# CHECK-NEXT: DW_MACRO_define_strp - lineno: 0 macro: __llvm__ 1
+# CHECK-NEXT: DW_MACRO_define_strp - lineno: 0 macro: clang version 11.0.0
+ .text
----------------
Do you know why it prints this content for this macro? It looks suspicious.
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:20
+# CHECK-NEXT: DW_MACRO_define_strp - lineno: 0 macro: clang version 11.0.0
+ .text
+ .file "test.c"
----------------
This line is not needed.
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:21
+ .text
+ .file "test.c"
+ .file 0 "/home" "test.c" md5 0x55645bd344ef72cf48bab13d10ec2c80
----------------
This too.
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:23
+ .file 0 "/home" "test.c" md5 0x55645bd344ef72cf48bab13d10ec2c80
+ .section .debug_abbrev,"", at progbits
+ .byte 1 # Abbreviation Code
----------------
The section is not required for the test.
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:44
+ .byte 0 # EOM(3)
+ .section .debug_info,"", at progbits
+.Lcu_begin0:
----------------
As well as this one.
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:71
+ .byte 1 # Line Number
+ .file 1 "." "foo.h" md5 0x0f0cd0e22b44f49d3944992c8dc28661 # File Number
+ .byte 1
----------------
Move it next to `.file 0` line.
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:81
+ .byte 4 # DW_MACRO_end_file
+ .byte 5 # DW_MACRO_define_strp
+ .byte 0 # Line Number
----------------
Why do you need more than one `DW_MACRO_define_strp` in the test?
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:87
+ .byte 0 # End Of Macro List Mark
+ .section .debug_str_offsets,"", at progbits
+ .long 16
----------------
The section is not used in this test, so, should be removed.
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:92
+.Lstr_offsets_base0:
+ .section .debug_str,"MS", at progbits,1
+.Linfo_string0:
----------------
Remove all lines which are not used.
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:105
+ .asciz "__llvm__ 1" # string offset=193
+ .ident "clang version 11.0.0 (https://github.com/llvm/llvm-project.git e4c50ac4207915968d76be259c042c0311fa2d97)"
+ .section ".note.GNU-stack","", at progbits
----------------
This is not needed.
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:106
+ .ident "clang version 11.0.0 (https://github.com/llvm/llvm-project.git e4c50ac4207915968d76be259c042c0311fa2d97)"
+ .section ".note.GNU-stack","", at progbits
+ .addrsig
----------------
And this.
================
Comment at: llvm/test/DebugInfo/X86/debug-macro-v5.s:107
+ .section ".note.GNU-stack","", at progbits
+ .addrsig
+ .section .debug_line,"", at progbits
----------------
Same here.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73086/new/
https://reviews.llvm.org/D73086
More information about the llvm-commits
mailing list