[PATCH] D44560: [DWARF] Rework debug line parsing to use llvm::Error and callbacks

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Sorry for not responding, I started drafting some follow-up comments regarding iterator-like implementations, but got caught up with other work, and never finished them off.

@dblaikie wrote:

> I thought at some point the review reached concensus that a separate error type was not needed - how'd it come back around again?


I got the impression that there wasn't consensus, so I was going to explore both approaches, but time has meant that I've only managed the second error type version, as presented by @espindola. In the current version, the majority of problems are now reported using `StringError`. If the user cares about the ability to continue to iterate, they should specifically handle `DebugLineLengthError`.

> I think I'd expect that API users wouldn't need to know the difference - they'd get told errors and get given as many line tables as could be parsed correctly, regardless of what kind of errors they were. (in the implementation, errors that result in inability to parse more things, would stop producing more things)

It's worth noting that at the moment, only one client requires the ability to iterate over all tables in sequence (DWARFContext, in the context of dumping them all). All others just ask for the line table at a given offset, so we don't necessarily need to optimise the iterate-over-all tables API, though I agree that it would be nice to improve it to be more iterator-like.

> Also, the mention in the patch description that this would result in inability to parse a line table contribution at a known offset seems problematic - if you have a section with some junk, then a line table, and there's a debug_info section that refers to the line table by the correct offset, the presence of junk coming before that line table doesn't seem like it should break dumping, right? But I suppose that's just "nice to have" maybe & not worth contorting the code too much to support.

I think you're right, although I don't think my change necessarily makes this worse - prior to my change, llvm-dwarfdump tries to read each table prologue in turn, and uses the length from the prologue to skip to the next one until it finds the correct offset. The length field could be broken in all sorts of ways, including being a reserved value. If a reserved value is hit, then the length is still used to jump to the next location, even though that is likely bogus (it's also likely outside of the section, though not necessarily).

In my implementation, if the length field is a reserved value, we stop. The rest of the time it's the same behaviour as the old behaviour. I'll update the summary to make this clearer.

A separate change could be made to simply jump directly to the desired offset rather than iterating over each table in turn, when a specific offset is specified. This would sort out this problem, and also allow further testing of the unit length field (e.g. to see if it falls within the section boundaries).


Repository:
  rL LLVM

https://reviews.llvm.org/D44560