Page MenuHomePhabricator

[DebugInfo] Make most debug line prologue errors non-fatal to parsing
ClosedPublic

Authored by jhenderson on Jan 3 2020, 7:41 AM.

Details

Summary

Many of the debug line prologue errors are not inherently fatal. In most cases, we can make reasonable assumptions and carry on. This patch does exactly that. In the case of length problems, the approach of "assume stated length is correct" is taken which means the offset might need adjusting.

Depends on D72157.

Diff Detail

Event Timeline

jhenderson created this revision.Jan 3 2020, 7:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2020, 7:41 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

Early ping - I'd like to get this and the other related reviews in before the release branch is created if possible.

As mentioned in another review - I'm not sure "assume bigger is correct" is a great strategy & not sure there's a lot of value in continuing in the face of a conflict like that. What sort of situations really benefit from such parsing optimism?

As mentioned in another review - I'm not sure "assume bigger is correct" is a great strategy & not sure there's a lot of value in continuing in the face of a conflict like that. What sort of situations really benefit from such parsing optimism?

See my comments in D72155. Having discussed it with a colleague, I think going with "the length field is always right" is a better approach, where possible. The extra parsing could be useful to some users, I guess.

jhenderson edited the summary of this revision. (Show Details)

Rebased + changed to assuming the stated length is always correct. This required some additional test changes to better demonstrate the behaviour difference.

dblaikie accepted this revision.Jan 17 2020, 5:10 PM
dblaikie added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
468–482

Note for future work: This seems fairly convoluted in terms of how to iterate over the section, parsing the contributions & handling the errors.

Having the explicit "skip" operation might be nice to avoid - and there's always recurring discussions about how to make this sort of parsing lazy so that users can get as much or as little data as they need without parsing things they don't need (so if the API were made more lazy generally - then "skip" would just be "parseLazily" then request the contribution length - if that doesn't error out, jump that many bytes ahead and parse the next one) - this would also move to fewer callback based errors, and more errors propagated on specific API interactions (eg: if you ask for the length, you return an ErrorOr<Length>, etc). Not sure if it's better or worse, but it's an option to consider.

Where does this code capture hard failures? Ah, in "Parser.done()"? So any hard failure (eg: failure to parse the length at all) results in the Parser moving to state "done" (& producing an error through one of the callbacks?)?

& should one or both of the "dumpWarning"s be errors? Recoverable failures (ultimately in the worst case "we could parse the length, but nothing else makes any sense" is a recoverable error) aren't necessarily warnings. Take Clang's behavior - lots of hard errors are recoverable (you missed a semicolon, we're going to keep going assuming you meant to have a semicolon there - but we aren't going to compile this code, it's still an error)

llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s
337–338

For this and other comments - probably not important/might be distracting to say what these would be parsed as rather than just to say that they're garbage data within the encoded length of the extended opcode, but beyond the parsed representation of that opcode? (or somtehing to that effect)

376–378

Not sure I understand what's happening here - again, perhaps you're describing two different ways this could be parsed? I'd focus on the way it is being parsed & not the ambiguity of other ways it could be parsed/error-recovered. (you can mention what assumptions about previous invalid data are made to form that particular parse)

This revision is now accepted and ready to land.Jan 17 2020, 5:10 PM
jhenderson marked 3 inline comments as done.Mon, Jan 27, 2:50 AM
jhenderson added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
468–482

Thanks for the comments!

Where does this code capture hard failures? Ah, in "Parser.done()"? So any hard failure (eg: failure to parse the length at all) results in the Parser moving to state "done" (& producing an error through one of the callbacks?)?

Right. The hard failures are primarily there to handle the cases where we really don't know where to continue from because you gave it data that doesn't really look like a line table. Since the parser would then be in a bad state, trying to get the next unit doesn't really make sense, so we just say we're done.

& should one or both of the "dumpWarning"s be errors? Recoverable failures (ultimately in the worst case "we could parse the length, but nothing else makes any sense" is a recoverable error) aren't necessarily warnings.

It's difficult to say, because it depends entirely on the client. Ideally, the tool using this library would configure the situation for its own needs. Always warning may well not be appropriate for some clients, but should it really be an error if e.g. lldb can't parse some of the debug data? I think the main client here is actually llvm-dwarfdump, where it could be argued that lots of things are really errors, but we shouldn't stop. But should the dumper exit with code 1 if the format is malformed? I don't know!

llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s
337–338

Fair enough. In some cases, the extra context is there to explain why we check what we do, but I'm okay changing it.

376–378

Actually, in this context, the header is being read twice, once actually as the header, and once again as part of the body, so indeed I am describing two different ways this is parsed. The change in behaviour is to continue reading from the end of the header, as claimed in the header length field, which means these parts are both part of the file table, and also the start of a line table sequence.

The comments are intended to help people (including myself) trying to match up what the data in the body is doing versus the checks in the file. I could format them differently as a table perhaps, to make it clearer what they represent on each parse?

jhenderson marked 2 inline comments as done.

Improve test comments.

@dblaikie, I'm going to hold off pushing this for a day or so, in case you've got any more comments about my latest comment update.

This revision was automatically updated to reflect the committed changes.

This broke LLDB's build, due to the interface change of Prologue::parse, as well as breaking an LLD test (I don't know why the LLD test failure didn't show up locally until after I committed...). I've reverted for the time being.

I'm not an LLDB developer (and don't have LLDB builds enabled), so I'm not really sure what would be the right way to fix this. Can anybody (@JDevlieghere?) help out here? My thinking is to take the code that currently handles parse errors and to pass that in as the callback too, as this best preserves the current behaviour. On the other hand, a prologue was parsed, at least partly successfully, so maybe it shouldn't be treated as an error?

jhenderson added a reviewer: labath.

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.

Herald added a project: Restricted Project. · View Herald Transcript
jhenderson reopened this revision.Tue, Jan 28, 6:07 AM

Could somebody please look at the LLDB change?

This revision is now accepted and ready to land.Tue, Jan 28, 6:07 AM

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.

jhenderson requested review of this revision.Tue, Jan 28, 8:35 AM

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.

labath accepted this revision.Tue, Jan 28, 11:59 PM

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...

This revision is now accepted and ready to land.Tue, Jan 28, 11:59 PM

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

This revision was automatically updated to reflect the committed changes.
labath added inline comments.Fri, Feb 14, 6:13 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
323–325

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.

jhenderson marked an inline comment as done.Fri, Feb 14, 8:00 AM
jhenderson added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
323–325

"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).

labath added inline comments.Mon, Feb 17, 4:03 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
323–325

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.