[PATCH] D74197: [DebugInfo] Simplify DWARFDebugAddr.

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin marked an inline comment as done.
ikudrin added a subscriber: HsiangKai.
ikudrin added inline comments.


================
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) {
----------------
dblaikie wrote:
> jhenderson wrote:
> > ikudrin wrote:
> > > 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.
> > Right, it's not DWARF compliant in the object (but would be in the final linked output was the idea), so would require some extra work in llvm-dwarfdump to simulate concatenation of the sections for it to work. I guess more generally, there's technically nothing stopping an assembler emitting a relocation instead of writing the literal value itself, but overall, I don't think it really matters.
> Yeah, for now the error checking needs to be against the DWARF-encoded length, which means the DWARFDataExtractor error checking would be insufficient (the DWARFDataExtractor would read correctly/without error beyond the DWARF-encoded length & so would miss errors). I think?
> 
> But after that, the error handling would look more or less like the D71704 case - and I do think that's better than explicit length checking. It will make the code easier to update in the future (not having to touch two places - one to parse a new field, another to change the length expectations, etc).
Well, there was a revision where `getU*` was changed to `getRelocatedValue` for a `Length` field, D58335. Maybe @HsiangKai can tell more about using relocations against such fields.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74197