[PATCH] D72900: [DebugInfo] Support 64-bit DWARF for .debug_names.

[Igor Kudrin via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:394-397
   // Check that we can read the fixed-size part.
   if (!AS.isValidOffsetForDataOfSize(*Offset, DWARF32HeaderFixedPartSize))
     return createStringError(errc::illegal_byte_sequence,
                              "Section too small: cannot read header.");
----------------
dblaikie wrote:
> Perhaps this could be refactored to common the length test?
> 
> ```
> uint64_t StartingOffset = *Offset);
> UnitLength = ...;
> Format = UnitLength == dwarf::DW_LENGTH_DWARF64 ? dwarf::DWARF64 : dwarf::DWARF32;
> if (!AS.isValidOffsetForDataOfSize(StartingOffset, dwarf::getUnitLengthFieldByteSize(Format) + CommonHeaderSize))
>   return createStringError(...);
> if (Format == dwarf::DWARF64) {
>   UnitLength = AS.getU64(Offset);
> } else if (...) {
>   return createStringError(...);
> }
> ```
> 
> I'm not super convinced it's so much better - but it does avoid the quirky "- 4" and such.
> 
> Alternatively we could skip all this and use the Error second parameter to the DataExtractor functions, and handle the error after the read attempts? More like the way DWARFUnitHeader::extract works, for example.
> 
I like the idea to check the remaining length before reading more than using the `Error` parameter. That makes the code more straightforward and allows producing more expressive error messages. So, for my taste, I would better rework `DWARFUnitHeader::extract()`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72900/new/

https://reviews.llvm.org/D72900