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

[Pavel Labath via Phabricator via llvm-commits]

labath marked 5 inline comments as done.
labath added inline comments.


================
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());
----------------
labath wrote:
> 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.
Test case added. Instead of adding the error prefix, I've created D75265.


================
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;
 }
----------------
labath wrote:
> 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.
The most interesting thing to test here would be to check that the parser correctly advances to the next unit for a contribution with length=DW_LENGTH_lo_reserved. However, that is not possible, since the very next thing it does after checking `totalLengthIsValid()` (which would have returned false before, but returns true now) is to check that the next unit offset fits into the data extractor.

However, I have created a dwarfdump test that ensures that we can dump a prologue of an incomplete contribution with this length (my other patch has introduced a bug where we would skip dumping of prologues with these  lengths. That seems to be the next best thing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75116