[PATCH] D49676: [DWARF] support for .debug_addr (consumer)
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 30 02:50:39 PDT 2018
jhenderson added inline comments.
================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:73
+ }
+ /// Return the size of the table header including the length
+ /// but not including the addresses.
----------------
Nit: Since this and the function before are multi-line definitions, I'd suggest putting blank lines either side of them.
================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1625
+ uint8_t Addr = 0;
+ for (const auto &CU : compile_units()) {
+ Addr = CU->getAddressByteSize();
----------------
Nit: I think this needs to be the explicit type, rather than auto, based on the coding style guide.
================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:54
+ uint32_t TmpLength = getLength();
+ HeaderData.Length = 0;
+ return createStringError(errc::invalid_argument,
----------------
jhenderson wrote:
> You might as well use the existing function for this.
>
> But actually, here and elsewhere below, I suggest you just assume that the Length is valid. I'd only invalidate it when the version is unsupported, DWARF64 is detected or the section is too short for a length field. That way the caller can just skip the rest of the table and attempt to read another one a bit later, which might work.
Do you have any comments on the second half of this inline comment? Because I don't think that's been done.
================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:16
+
+void DWARFDebugAddrTable::clear() {
+ HeaderData = {};
----------------
I'd call `invalidateLength` here (yes, the reinitializing of HeaderData renders this redundant, but that then means you are relying on knowing that the value 0 for Length is the method used for knowing if the Length is invalid).
================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:25
+ uint8_t AddrSize,
+ std::function<void(Error)> WarnCallback) {
+ HeaderOffset = *OffsetPtr;
----------------
I think the common thing to do in other parts of the DWARF library is to call `clear()` here, to ensure we don't try to extract into an already-filled in table.
================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:36
+ WarnCallback(createStringError(errc::invalid_argument,
+ "DWARF version is not defined, assuming version 5"));
+ SuggestedVersion = 5;
----------------
This should probably say "DWARF version is not defined in a CU" etc, since the Version is defined in the table usually.
================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:85
+ // We support DWARF version 5 for now as well as pre-DWARF5
+ // implementation of .debug_addr table, which doesn't contain a header
+ // and consists only of a series of addresses.
----------------
implementation -> implementations
================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:92
+ " which is different from the version suggested"
+ " by DWARF unit header: %" PRIu16,
+ HeaderOffset, HeaderData.Version, SuggestedVersion);
----------------
by DWARF -> by the DWARF.
But if I've understood the behaviour correctly this may be a bad error message to have, actually. Consider the case where we have both DWARF4 and DWARF5 style debug info tables in a single file (e.g. an executable). Won't this result in a version mismatch error? Or am I misunderstanding how this code works? If so, it probably needs comments explaining what is meant by "SuggestedVersion" and where it typically might come from. It might help to rename it "UnitVersion" as well.
Nit: by DWARF -> by the DWARF...
================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:154
+ OS << "Addrs: [";
+ for (const auto &Addr : Addrs) {
+ OS << format(AddrFmt.c_str(), Addr);
----------------
Don't use auto here, as per the style guide.
================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:72
+
+ // Perform basic validation of the remaining header fields.
+ if (HeaderData.Version != SuggestedVersion)
----------------
vleschuk wrote:
> jhenderson wrote:
> > jhenderson wrote:
> > > It may be worth saying that Version != 5 is not supported (except for the special case)?
> > It might have lost track. Which error message shows this exactly?
> Example:
>
>
> ```
> # ERR: section is not large enough to contain a .debug_addr table of length 0x10 at offset 0x0
> # ERR-NOT: {{.}}
>
> # too small section to contain section of given length
> .section .debug_addr,"", at progbits
> .Ldebug_addr0:
> .long 12 # unit_length
> .short 5 # version
> .byte 4 # address_size
> .byte 0 # segment_selector_size
> ```
>
> I added a test for this.
Sorry, I think I meant this point here:
> It may be worth saying that Version != 5 is not supported (except for the special case)?
I don't see this being done at all.
================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr.s:22
+
+# ERR: DWARF version is not defined, assuming version 5
+# ERR: DWARF version is not defined, assuming version 5
----------------
vleschuk wrote:
> jhenderson wrote:
> > It would be nice to have a test case that doesn't produce this warning.
> We already have on: see debug_addr_version_mismatch.s - the first table produces the warning, the second one doesn't.
Right, in that case, I don't think you should check the warning in this test. In fact, it might be worth crafting a small .debug_info unit to suppress the warning in this basic test case.
================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_size_mismatch.s:1
+# RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o - | \
+# RUN: llvm-dwarfdump -debug-addr - 2> %t.err | FileCheck %s
----------------
This test should be renamed to debug_addr_address_size_mismatch or similar, since the size could be referring to several different things.
https://reviews.llvm.org/D49676
More information about the llvm-commits
mailing list