[PATCH] D75116: [DWARFDebugLine] Use new DWARFDataExtractor::getInitialLength

[Pavel Labath via Phabricator via llvm-commits]

labath planned changes to this revision.
labath marked 3 inline comments as done.
labath added a comment.

I haven't managed to get around to updating this patch today. I'll try to do it tomorrow.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:347-350
+    return createStringError(
+        errc::invalid_argument,
+        "parsing line table prologue at offset 0x%8.8" PRIx64 ": %s",
+        PrologueOffset, toString(std::move(Err)).c_str());
----------------
jhenderson wrote:
> If I understand this correctly, this will now report cases where the unit length couldn't be read due to data running out. Should this change include some testing for that too?
> 
> Also, in that case, if I've followed the code correctly, the error message will be "parsing line table prologue at offset 0x00000000: unexpected end of data", which doesn't give particularly great precision as to where the unexpected end of data was. How about adding something like "parsing line table prologue at offset 0x00000000: unable to read unit_length: unexpected end of data"?
Yes, I think I can add a test for the "data running out" case. 

I also believe you're right about the error message. I can add the extra string since it is convenient here, but in general I wouldn't want to subscribe to this level of detail in error messages. My goal is to make error handling to only take up a fraction of the function doing the parsing. Ideally I'd just carry the Error (or Cursor) object around, and only check it once or twice per function -- kind of like what the debug_names patch (D75119) does now. If I wanted report the exact point at which the data "ran out", I'd have to check for errors after every get() operation.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:1218
 bool DWARFDebugLine::Prologue::totalLengthIsValid() const {
-  return TotalLength == dwarf::DW_LENGTH_DWARF64 ||
-         TotalLength < dwarf::DW_LENGTH_lo_reserved;
+  return TotalLength != 0u;
 }
----------------
jhenderson wrote:
> Idle thought: If wonder if this should be unit tested (probably at a slightly higher level)? I could easily see a bug where a length equal to DW_LENGTH_lo_reserved in a DWARF64 file was not treated as valid.
Yes, I've noticed this too, and I am pretty sure there was a bug here. I'll see if I can construct some meanigful test here without creating a 4GB buffer.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp:810
 
 TEST_F(DebugLineBasicFixture, ParserMovesToEndForBadLengthWhenParsing) {
   if (!setupGenerator())
----------------
jhenderson wrote:
> This and the following case might want renaming slightly, since the offset is no longer at the end. Perhaps "ParserMarkedAsDoneFor..."?
will do


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75116