[PATCH] D72155: [DebugInfo] Make incorrect debug line extended opcode length non-fatal

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

> Making it easier for consumers to continue and try to do something with slightly bad output. One of the problems with using the unrecoverable errors is that it prevents people even trying to iterate over later tables.

I misremembered the rest of the code. An unrecoverable error doesn't prevent the SectionParser from continuing to the next line table. It just means that tools like llvm-dwarfdump will treat them as an error. The only difference between this and the recoverable error is that parsing of the CURRENT table will stop.

I took a step back and re-discussed the situation with a colleague offline. The errors are passed to callbacks so that clients can decide what to do when they see an error. For example, a consumer who needs to rely on the output being definitely correct can choose the callback to say "ignore this line table, and also ignore any further errors in that table". The only real difference between the unrecoverable and recoverable callbacks are that we stop parsing at the point the former is hit because we have no way of continuing, meaning our information is incomplete, whereas the latter will try to continue and give some information, but the information might be inaccurate. Given this, I don't think causing producing spurious information is wrong, as long as we have reported at least one error.

That being said, I do think "largest one wins" is the wrong approach, having considered some of the other cases. In particular, the SectionParser treats the unit length field as sacrosanct, and will always follow it (assuming it can possibly make sense - see `totalLengthIsValid`) when moving to the next table, evn if it means that the offset moves backwards from the point it got to after reading the table. I therefore think for consistency, we should treat all length fields as sacrosanct, i.e. jump to the position the offset should be according to the length (whether it is bigger or not) and continue from there. Yes, this might cause the parsing to produce bogus information, but that's okay (see my above point).

I'll update this and the other similar diffs accordingly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72155