[PATCH] D74197: [DebugInfo] Simplify DWARFDebugAddr.
Igor Kudrin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 11 01:35:19 PST 2020
ikudrin marked 4 inline comments as done.
ikudrin added a subscriber: labath.
ikudrin added inline comments.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:21
+ assert(Data.isValidOffsetForDataOfSize(*OffsetPtr, DataSize));
+ if (AddrSize != 4 && AddrSize != 8)
+ return createStringError(errc::not_supported,
----------------
jhenderson wrote:
> Perhaps something for a later change, but the `DataExtractor` also supports sizes 1 and 2, and the latter at least is currently used by some architectures (possibly incorrectly - see D73961 and D73962).
As there is some uncertainty about that, I'd prefer not to rush with that change here.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:55
Format = dwarf::DwarfFormat::DWARF32;
- if (UnitVersion >= 5) {
- HeaderData.Length = Data.getU32(OffsetPtr);
- if (HeaderData.Length == dwarf::DW_LENGTH_DWARF64) {
- invalidateLength();
- return createStringError(errc::not_supported,
- "DWARF64 is not supported in .debug_addr at offset 0x%" PRIx64,
- HeaderOffset);
- }
- if (HeaderData.Length + sizeof(uint32_t) < sizeof(Header)) {
- uint32_t TmpLength = HeaderData.Length;
- invalidateLength();
- return createStringError(errc::invalid_argument,
- "address table at offset 0x%" PRIx64
- " has too small length (0x%" PRIx32
- ") to contain a complete header",
- HeaderOffset, TmpLength);
- }
- uint64_t End = HeaderOffset + getLength();
- if (!Data.isValidOffsetForDataOfSize(HeaderOffset, End - HeaderOffset)) {
- uint32_t TmpLength = HeaderData.Length;
- invalidateLength();
- return createStringError(
- errc::invalid_argument,
- "section is not large enough to contain an address table "
- "at offset 0x%" PRIx64 " with a length field of 0x%" PRIx32,
- HeaderOffset, TmpLength);
- }
-
- HeaderData.Version = Data.getU16(OffsetPtr);
- HeaderData.AddrSize = Data.getU8(OffsetPtr);
- HeaderData.SegSize = Data.getU8(OffsetPtr);
- DataSize = getDataSize();
- } else {
- HeaderData.Version = UnitVersion;
- HeaderData.AddrSize = AddrSize;
- // TODO: Support for non-zero SegSize.
- HeaderData.SegSize = 0;
- DataSize = Data.size();
+ Length = Data.getU32(OffsetPtr);
+ if (Length == dwarf::DW_LENGTH_DWARF64) {
----------------
jhenderson wrote:
> The debug line code uses `getRelocatedValue` here. I don't know if it's correct to do so (especially as it doesn't for the DWARF64 case...), but raising it just in case.
>
> Also, as mentioned elsewhere, you could try passing an `Error` as the second argument to avoid needing to do the `isValid...` check. (Same goes elsewhere around here).
> Also, as mentioned elsewhere, you could try passing an Error as the second argument to avoid needing to do the isValid... check. (Same goes elsewhere around here).
I've tried that in D71704, but I can't say I really like the result. For my taste, explicit checking for the remaining size looks much more clear and helps to generate more accurate error messages. Let's postpone the changes in that area until @labath presents his solution.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:55
Format = dwarf::DwarfFormat::DWARF32;
- if (UnitVersion >= 5) {
- HeaderData.Length = Data.getU32(OffsetPtr);
- if (HeaderData.Length == dwarf::DW_LENGTH_DWARF64) {
- invalidateLength();
- return createStringError(errc::not_supported,
- "DWARF64 is not supported in .debug_addr at offset 0x%" PRIx64,
- HeaderOffset);
- }
- if (HeaderData.Length + sizeof(uint32_t) < sizeof(Header)) {
- uint32_t TmpLength = HeaderData.Length;
- invalidateLength();
- return createStringError(errc::invalid_argument,
- "address table at offset 0x%" PRIx64
- " has too small length (0x%" PRIx32
- ") to contain a complete header",
- HeaderOffset, TmpLength);
- }
- uint64_t End = HeaderOffset + getLength();
- if (!Data.isValidOffsetForDataOfSize(HeaderOffset, End - HeaderOffset)) {
- uint32_t TmpLength = HeaderData.Length;
- invalidateLength();
- return createStringError(
- errc::invalid_argument,
- "section is not large enough to contain an address table "
- "at offset 0x%" PRIx64 " with a length field of 0x%" PRIx32,
- HeaderOffset, TmpLength);
- }
-
- HeaderData.Version = Data.getU16(OffsetPtr);
- HeaderData.AddrSize = Data.getU8(OffsetPtr);
- HeaderData.SegSize = Data.getU8(OffsetPtr);
- DataSize = getDataSize();
- } else {
- HeaderData.Version = UnitVersion;
- HeaderData.AddrSize = AddrSize;
- // TODO: Support for non-zero SegSize.
- HeaderData.SegSize = 0;
- DataSize = Data.size();
+ Length = Data.getU32(OffsetPtr);
+ if (Length == dwarf::DW_LENGTH_DWARF64) {
----------------
jhenderson wrote:
> dblaikie wrote:
> > ikudrin wrote:
> > > jhenderson wrote:
> > > > The debug line code uses `getRelocatedValue` here. I don't know if it's correct to do so (especially as it doesn't for the DWARF64 case...), but raising it just in case.
> > > >
> > > > Also, as mentioned elsewhere, you could try passing an `Error` as the second argument to avoid needing to do the `isValid...` check. (Same goes elsewhere around here).
> > > > Also, as mentioned elsewhere, you could try passing an Error as the second argument to avoid needing to do the isValid... check. (Same goes elsewhere around here).
> > >
> > > I've tried that in D71704, but I can't say I really like the result. For my taste, explicit checking for the remaining size looks much more clear and helps to generate more accurate error messages. Let's postpone the changes in that area until @labath presents his solution.
> > That sounds like a (harmless) bug in the debug line code - I don't think there's any reason the length would need to be relocatable. But I could be wrong (/maybe/ for platforms that do linker relaxation of some kind and use anything like a ULEB encoding for address ranges in debug_line (thus debug_line contribution could change size during linking & the length would then need a linker relocation to adjust for that) - but I don't think that's supported anywhere... ). Probably best to switch that to getU32 unless any tests fail/etc.
> Now that I think about it, I did a prototype piece of work where I split the line table header into a separate section in the object from its body, as part of investigations into removing dead line table blocks for GC-ed sections. I think I needed it relocatable to allow for a symbol at the table end to be used to calculate the length in the unit_length field. However, it certainly didn't support DWARF64 or anything like that, so definitely had some inefficiencies. Perhaps the "correct" thing to do would be to make the DWARF64 length part a relocatable read? But in practice, I don't think it really matters.
> Now that I think about it, I did a prototype piece of work where I split the line table header into a separate section in the object from its body, as part of investigations into removing dead line table blocks for GC-ed sections.
Sounds like your object file is not following the DWARF standard, no? Next, when you link your objects into a final binary, which should have normal DWARF sections, the linker should apply your relocation statically and remove it from the resulting binary, right? Thus, we still can use just `getU32`/`getU64` to read the length from any DWARF-compliant file.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:89
+ // Perform a basic validation of the header fields.
+ if (Version != 5)
return createStringError(errc::not_supported,
----------------
jhenderson wrote:
> Do we have an unsupported version test case for version 6 (or more specifically, one higher than the current max supported version)?
`llvm/test/tools/llvm-dwarfdump/X86/debug_addr_unsupported_version.s`
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74197/new/
https://reviews.llvm.org/D74197
More information about the llvm-commits
mailing list