[PATCH] D72900: [DebugInfo] Support 64-bit DWARF for .debug_names.
Igor Kudrin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 27 05:26:33 PST 2020
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
More information about the llvm-commits
mailing list