[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:323-325
+// Treat this error as unrecoverable - we cannot be sure what any of
+// the data represents including the length field, so cannot skip it or 
make
+// any reasonable assumptions.

jhenderson wrote:
> labath wrote:
> > BTW, I think this error should be recoverable too. I believe the reason why 
> > the length field comes *before* the version number is specifically so that 
> > one can skip over contributions with unsupported (future) version numbers.
> > 
> > While it's hard to say what the future versions of dwarf will look like, I 
> > would expect that the committee will try very hard to avoid  making changes 
> > in the length field. I think they'd use one of the 
> > DW_LENGTH_lo_reserved..DW_LENGTH_hi_reserved-1 constants for severely 
> > incompatible changes.
> "Unrecoverable" here means don't try to parse this table, but do allow 
> parsing the next. I think the comment might be slightly misleading in this 
> regard. FWIW, a version of 0 or 1 probably doesn't have a leading length, so 
> it is definitely unrecoverable. For versions > 5, which are now checked, we 
> don't know what the structure of the header is, so although we could take a 
> guess, we'd almost certainly get it wrong and produce invalid (possibly very 
> invalid) output. I don't have a strong opinion as to whether that should be 
> an unrecoverable error or not (currently it is).
Ah, right, I see now what you mean. I agree it makes no sense to parse the 
contents of a contribution with an unrecognized version. The part about not 
being able to trust the length field threw me off, as the length of the 
contribution is the one thing we can expect to remain unchanged between dwarf 
versions.

Sorry about the false alarm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72158



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

[James Henderson via Phabricator via lldb-commits]

jhenderson marked an inline comment as done.
jhenderson added inline comments.



Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:323-325
+// Treat this error as unrecoverable - we cannot be sure what any of
+// the data represents including the length field, so cannot skip it or 
make
+// any reasonable assumptions.

labath wrote:
> BTW, I think this error should be recoverable too. I believe the reason why 
> the length field comes *before* the version number is specifically so that 
> one can skip over contributions with unsupported (future) version numbers.
> 
> While it's hard to say what the future versions of dwarf will look like, I 
> would expect that the committee will try very hard to avoid  making changes 
> in the length field. I think they'd use one of the 
> DW_LENGTH_lo_reserved..DW_LENGTH_hi_reserved-1 constants for severely 
> incompatible changes.
"Unrecoverable" here means don't try to parse this table, but do allow parsing 
the next. I think the comment might be slightly misleading in this regard. 
FWIW, a version of 0 or 1 probably doesn't have a leading length, so it is 
definitely unrecoverable. For versions > 5, which are now checked, we don't 
know what the structure of the header is, so although we could take a guess, 
we'd almost certainly get it wrong and produce invalid (possibly very invalid) 
output. I don't have a strong opinion as to whether that should be an 
unrecoverable error or not (currently it is).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72158



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:323-325
+// Treat this error as unrecoverable - we cannot be sure what any of
+// the data represents including the length field, so cannot skip it or 
make
+// any reasonable assumptions.

BTW, I think this error should be recoverable too. I believe the reason why the 
length field comes *before* the version number is specifically so that one can 
skip over contributions with unsupported (future) version numbers.

While it's hard to say what the future versions of dwarf will look like, I 
would expect that the committee will try very hard to avoid  making changes in 
the length field. I think they'd use one of the 
DW_LENGTH_lo_reserved..DW_LENGTH_hi_reserved-1 constants for severely 
incompatible changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72158



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

[James Henderson via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7116e431c0ab: [DebugInfo] Make most debug line prologue 
errors non-fatal to parsing (authored by jhenderson).

Changed prior to commit:
  https://reviews.llvm.org/D72158?vs=240852&id=241076#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72158

Files:
  lld/test/ELF/Inputs/undef-bad-debug.s
  lld/test/ELF/undef.s
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
  llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
  llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
  llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s
  llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test
  llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

Index: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
===
--- llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
+++ llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
@@ -359,10 +359,15 @@
 
   generate();
 
-  checkGetOrParseLineTableEmitsFatalError(
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+nullptr, RecordRecoverable);
+  EXPECT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+
+  checkError(
   {"parsing line table prologue at 0x found an invalid directory "
"or file table description at 0x0014",
-   "failed to parse entry content descriptions because no path was found"});
+   "failed to parse entry content descriptions because no path was found"},
+  std::move(Recoverable));
 }
 
 TEST_P(DebugLineParameterisedFixture, ErrorForTooLargePrologueLength) {
@@ -379,14 +384,24 @@
 
   generate();
 
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+nullptr, RecordRecoverable);
+  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+  DWARFDebugLine::LineTable Result(**ExpectedLineTable);
+  // Undo the earlier modification so that it can be compared against a
+  // "default" prologue.
+  --Result.Prologue.PrologueLength;
+  checkDefaultPrologue(Version, Format, Result.Prologue, 0);
+
   uint64_t ExpectedEnd =
   Prologue.TotalLength + 1 + Prologue.sizeofTotalLength();
-  checkGetOrParseLineTableEmitsFatalError(
+  checkError(
   (Twine("parsing line table prologue at 0x should have ended at "
  "0x00") +
Twine::utohexstr(ExpectedEnd) + " but it ended at 0x00" +
Twine::utohexstr(ExpectedEnd - 1))
-  .str());
+  .str(),
+  std::move(Recoverable));
 }
 
 TEST_P(DebugLineParameterisedFixture, ErrorForTooShortPrologueLength) {
@@ -408,16 +423,29 @@
 
   generate();
 
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+nullptr, RecordRecoverable);
+  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+  DWARFDebugLine::LineTable Result(**ExpectedLineTable);
+  // Undo the earlier modification so that it can be compared against a
+  // "default" prologue.
+  if (Version < 5)
+Result.Prologue.PrologueLength += 2;
+  else
+Result.Prologue.PrologueLength += 1;
+  checkDefaultPrologue(Version, Format, Result.Prologue, 0);
+
   uint64_t ExpectedEnd =
   Prologue.TotalLength - 1 + Prologue.sizeofTotalLength();
   if (Version < 5)
 --ExpectedEnd;
-  checkGetOrParseLineTableEmitsFatalError(
+  checkError(
   (Twine("parsing line table prologue at 0x should have ended at "
  "0x00") +
Twine::utohexstr(ExpectedEnd) + " but it ended at 0x00" +
Twine::utohexstr(ExpectedEnd + 1))
-  .str());
+  .str(),
+  std::move(Recoverable));
 }
 
 INSTANTIATE_TEST_CASE_P(
@@ -636,14 +664,15 @@
   EXPECT_EQ(Parser.getOffset(), 0u);
   ASSERT_FALSE(Parser.done());
 
-  Parser.skip(RecordUnrecoverable);
+  Parser.skip(RecordRecoverable, RecordUnrecoverable);
   EXPECT_EQ(Parser.getOffset(), 62u);
   ASSERT_FALSE(Parser.done());
 
-  Parser.skip(RecordUnrecoverable);
+  Parser.skip(RecordRecoverable, RecordUnrecoverable);
   EXPECT_EQ(Parser.getOffset(), 136u);
   EXPECT_TRUE(Parser.done());
 
+  EXPECT_FALSE(Recoverable);
   EXPECT_FALSE(Unrecoverable);
 }
 
@@ -688,10 +717,11 @@
   generate();
 
   DWARFDebugLine::SectionParser Parser(LineData, *Context, CUs, TUs);
-  Parser.skip(RecordUnrecoverable);
+  Parser.skip(RecordRecoverable, RecordUnrecoverable);
 
   EXPECT_EQ(Parser.getOffset(), 4u);
   EXPECT_TRUE(Parser.done());
+  EXPECT_FALSE(Recoverable);
 
   checkError("parsing line table prologue at offset 0x unsupported "
  "reserved unit length found of value 0xfff0",
@@ -767,11 +797,12 @@
   generate();
 
   DWARFDebugLine::SectionParser Pa

[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

[James Henderson via Phabricator via lldb-commits]

jhenderson added a comment.

Thanks for the review comments! I'll go ahead and land it like this, assuming 
my local test run passes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72158



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Actually I think this is fine. We would want to squeeze as much information as 
possible from these kinds of line tables.

I don't think fully preserving the existing behavior would be that easy, 
actually. We have another call to the llvm line table parser in 
`ParseLLVMLineTable` (line 154), but this one calls 
`DWARFDebugLine::LineTable::getOrParseLineTable` There, we already ignore (log) 
recoverable errors, and it'd be hard to tell the "new" kinds of errors from the 
"old" ones...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72158



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

[James Henderson via Phabricator via lldb-commits]

jhenderson requested review of this revision.
jhenderson added a comment.

In D72158#1844534 , @labath wrote:

> If I understand this correctly, this will cause lldb to continue to read the 
> parsed line table contribution after encountering some errors in the 
> prologue, whereas previously we would stop straight away. That sounds 
> reasonable if now or in the future we will be able to get some useful 
> information (at least some subset of file names, or line numbers without file 
> names, etc.) out of these kinds of line tables.


Indeed, that's what this change allows: the parser will continue parsing after 
reporting the Errors via the new callback. In cases where it can't (i.e. 
unsupported versions or reserved unit length values), it will stop and return 
the Error (as it previously did, and also did for the now-recoverable Errors). 
For now I've changed LLDB to record both kinds of Errors in the same way as 
they were before, but the recoverable errors do not prevent it subsequently 
calling `ParseSupportFilesFromPrologue`. I could just as easily change it to 
doing what it always did for all cases, namely log the errors and not call 
`ParseSupportFilesFromPrologue`. That's probably the safer approach on further 
reflection, so I'll update the patch tomorrow (I'm just about to leave the 
office for the day), unless someone thinks changing the behaviour is good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72158



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

If I understand this correctly, this will cause lldb to continue to read the 
parsed line table contribution after encountering some errors in the prologue, 
whereas previously we would stop straight away. That sounds reasonable if now 
or in the future we will be able to get some useful information (at least some 
subset of file names, or line numbers without file names, etc.) out of these 
kinds of line tables. If that is not the case, and these errors are recoverable 
only in the sense that they allow you to jump to the next contribution, then it 
might be better to treat these errors as unrecoverable for lldb purposes 
(jumping to the next contribution is not interesting for us since we always use 
DW_AT_stmt_list to locate the line table).

However, I don't think that resolving that needs to hold this patch up, as this 
behavior can be easily adjusted from within lldb.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72158



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

[James Henderson via Phabricator via lldb-commits]

jhenderson reopened this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

Could somebody please look at the LLDB change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72158



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D72158: [DebugInfo] Make most debug line prologue errors non-fatal to parsing

[James Henderson via Phabricator via lldb-commits]

jhenderson updated this revision to Diff 240852.
jhenderson added a reviewer: labath.
jhenderson added a comment.
Herald added subscribers: lldb-commits, emaste.
Herald added a reviewer: espindola.
Herald added a project: LLDB.

Fix LLD test + fix LLDB build.

I'm uncertain if the LLDB fix is the right fix to make or not, but it does at 
least stop this change breaking the build.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72158

Files:
  lld/test/ELF/Inputs/undef-bad-debug.s
  lld/test/ELF/undef.s
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
  llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
  llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
  llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s
  llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test
  llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

Index: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
===
--- llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
+++ llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
@@ -359,10 +359,15 @@
 
   generate();
 
-  checkGetOrParseLineTableEmitsFatalError(
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+nullptr, RecordRecoverable);
+  EXPECT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+
+  checkError(
   {"parsing line table prologue at 0x found an invalid directory "
"or file table description at 0x0014",
-   "failed to parse entry content descriptions because no path was found"});
+   "failed to parse entry content descriptions because no path was found"},
+  std::move(Recoverable));
 }
 
 TEST_P(DebugLineParameterisedFixture, ErrorForTooLargePrologueLength) {
@@ -379,14 +384,24 @@
 
   generate();
 
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+nullptr, RecordRecoverable);
+  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+  DWARFDebugLine::LineTable Result(**ExpectedLineTable);
+  // Undo the earlier modification so that it can be compared against a
+  // "default" prologue.
+  --Result.Prologue.PrologueLength;
+  checkDefaultPrologue(Version, Format, Result.Prologue, 0);
+
   uint64_t ExpectedEnd =
   Prologue.TotalLength + 1 + Prologue.sizeofTotalLength();
-  checkGetOrParseLineTableEmitsFatalError(
+  checkError(
   (Twine("parsing line table prologue at 0x should have ended at "
  "0x00") +
Twine::utohexstr(ExpectedEnd) + " but it ended at 0x00" +
Twine::utohexstr(ExpectedEnd - 1))
-  .str());
+  .str(),
+  std::move(Recoverable));
 }
 
 TEST_P(DebugLineParameterisedFixture, ErrorForTooShortPrologueLength) {
@@ -408,16 +423,29 @@
 
   generate();
 
+  auto ExpectedLineTable = Line.getOrParseLineTable(LineData, 0, *Context,
+nullptr, RecordRecoverable);
+  ASSERT_THAT_EXPECTED(ExpectedLineTable, Succeeded());
+  DWARFDebugLine::LineTable Result(**ExpectedLineTable);
+  // Undo the earlier modification so that it can be compared against a
+  // "default" prologue.
+  if (Version < 5)
+Result.Prologue.PrologueLength += 2;
+  else
+Result.Prologue.PrologueLength += 1;
+  checkDefaultPrologue(Version, Format, Result.Prologue, 0);
+
   uint64_t ExpectedEnd =
   Prologue.TotalLength - 1 + Prologue.sizeofTotalLength();
   if (Version < 5)
 --ExpectedEnd;
-  checkGetOrParseLineTableEmitsFatalError(
+  checkError(
   (Twine("parsing line table prologue at 0x should have ended at "
  "0x00") +
Twine::utohexstr(ExpectedEnd) + " but it ended at 0x00" +
Twine::utohexstr(ExpectedEnd + 1))
-  .str());
+  .str(),
+  std::move(Recoverable));
 }
 
 INSTANTIATE_TEST_CASE_P(
@@ -636,14 +664,15 @@
   EXPECT_EQ(Parser.getOffset(), 0u);
   ASSERT_FALSE(Parser.done());
 
-  Parser.skip(RecordUnrecoverable);
+  Parser.skip(RecordRecoverable, RecordUnrecoverable);
   EXPECT_EQ(Parser.getOffset(), 62u);
   ASSERT_FALSE(Parser.done());
 
-  Parser.skip(RecordUnrecoverable);
+  Parser.skip(RecordRecoverable, RecordUnrecoverable);
   EXPECT_EQ(Parser.getOffset(), 136u);
   EXPECT_TRUE(Parser.done());
 
+  EXPECT_FALSE(Recoverable);
   EXPECT_FALSE(Unrecoverable);
 }
 
@@ -688,10 +717,11 @@
   generate();
 
   DWARFDebugLine::SectionParser Parser(LineData, *Context, CUs, TUs);
-  Parser.skip(RecordUnrecoverable);
+  Parser.skip(RecordRecoverable, RecordUnrecoverable);
 
   EXPECT_EQ(Parser.getOffset(), 4u);
   EXPECT_TRUE(Parser.done());
+  EXPECT_FALSE(Recoverable);
 
   checkError("parsing line table prologue at offset 0x unsupported "
  "reserved unit length found