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

[Victor Leschuk via Phabricator via llvm-commits]

vleschuk added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:72
+
+  // Perform basic validation of the remaining header fields.
+  if (HeaderData.Version != SuggestedVersion)
----------------
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 ... 




================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:54
+      uint32_t TmpLength = getLength();
+      HeaderData.Length = 0;
+      return createStringError(errc::invalid_argument,
----------------
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;                                            
```


================
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:
> 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.


https://reviews.llvm.org/D49676