[PATCH] D49676: [DWARF] support for .debug_addr (consumer)

[Victor Leschuk via Phabricator via llvm-commits]

vleschuk marked an inline comment as done.
vleschuk added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:54
+      uint32_t TmpLength = getLength();
+      HeaderData.Length = 0;
+      return createStringError(errc::invalid_argument,
----------------
jhenderson wrote:
> 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?
The most similar case is .debug_rnglists implementation where the same "mechanism" (break if length is invalid) is used, however the RnglistsTable implementation doesn't use it all: there is know explicit invalidation in case of error (I think we should work on it and see if there are any problems).

In our case I suggest to leave invalidation here: we assume that the header is correct and read the Length value from it, after that we see that our data contradicts our assumption and either Length is invalid or the section is malformed. In any case it is better to report an error than produce potentially invalid output.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:92
+                       " which is different from the version suggested"
+                       " by DWARF unit header: %" PRIu16,
+                       HeaderOffset, HeaderData.Version, SuggestedVersion);
----------------
jhenderson wrote:
> 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.
I think we can put a FIXME mark here for now and get back to it later if you don't mind.


https://reviews.llvm.org/D49676