[PATCH] D49676: [DWARF] support for .debug_addr (consumer)
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 31 02:47:53 PDT 2018
jhenderson added inline comments.
================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:72
+
+ // Perform basic validation of the remaining header fields.
+ if (HeaderData.Version != SuggestedVersion)
----------------
vleschuk wrote:
> jhenderson wrote:
> > 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.
> I am sorry, I think now that's me who lost track here? Are talking about the comment below?
>
> > // We support DWARF version 5 for now as well as pre-DWARF5 ...
>
>
I'll reset this by commenting on the line in question...
================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:54
+ uint32_t TmpLength = getLength();
+ HeaderData.Length = 0;
+ return createStringError(errc::invalid_argument,
----------------
vleschuk wrote:
> jhenderson wrote:
> > 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.
> Actually I don't see how it might work. The caller relies on the fact that length value from the header is valid and parses the rest of the section based on this assumption. In this case the length field is definitely invalid and telling caller that everything is ok will result in incorrect parsing. Here is an example:
>
>
> ```
> .Ldebug_addr0:
> .long 1 # unit_length (should be 12 here)
> .short 5 # version
> .byte 4 # address_size
> .byte 0 # segment_selector_size
> .long 0x00000000
> .long 0x00000001
> ```
>
> ```
> if (!AddrTable.hasValidLength())
> break; // If we do not break here we will increase Offset by 5 and not by 16 -> incorrect result.
> uint64_t Length = AddrTable.getLength();
> Offset = TableOffset + Length;
> ```
I think this comes down to "who should we trust more?". What do we do in other cases in the DWARF library?
================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:92
+ " which is different from the version suggested"
+ " by DWARF unit header: %" PRIu16,
+ HeaderOffset, HeaderData.Version, SuggestedVersion);
----------------
vleschuk wrote:
> jhenderson wrote:
> > 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...
> You understood it correctly. But there is no ideal way to handle this. I used the same approach here as used in extraction of DebugStrOffsets, CU "suggests" MaxVersion found and version mismatch is treated as error: in case of version mismatch one can't tell exactly how to parse the header.
Okay, I think I get you now. I think it shouldn't block this going in, in its first form, but I think I'd consider it a bug. Consider the following case:
- test.o has been compiled with DWARF version 5
- test2.o has been compiled with DWARF version 4 (and has a .debug_addr table)
- test.elf is test.o and test2.o linked together.
- llvm-dwarfdump will fail to dump both .debug_addr tables, because there will always be a mismatch.
Such a scenario is far from unlikely. It's quite common for static libraries built with older compiler versions (and therefore potentially different DWARF versions) to be linked together with objects or libraries from newer compilers.
By my understanding of the DWARF5 standard, the correct way to associate a .debug_addr table with a .debug_info table is to look at the DW_AT_addr_base attribute in the info table. In this case, each of the two .debug_info tables would point at a different .debug_addr table.
================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:90
+ // and consists only of a series of addresses.
+ if (HeaderData.Version != UnitVersion)
+ return createStringError(errc::invalid_argument,
----------------
Before this check, we should check that the Version is a supported Version. The only ones supported are <= 5 (I would argue 4 and 5 explicitly, but I'm not sure at what point pre-standardisated .debug_addr tables started getting emitted, so the lower bound might be wrong). So if we encounter a Version >= 6, we should emit an unsupported error, because in that case, we can't guarantee that our parser can handle it. I'd then assume the Length is valid and jump to the next table when iterating over it.
https://reviews.llvm.org/D49676
More information about the llvm-commits
mailing list