[PATCH] D73962: [DebugInfo] Error if unsupported address size detected in line table

Wed Feb 5 07:39:09 PST 2020

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

================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:647
+          // the address and continue to the next opcode.
+          if (OpcodeAddressSize != 4 && OpcodeAddressSize != 8) {
+            RecoverableErrorCallback(createStringError(
----------------
probinson wrote:
> I just happened to notice D73961 which says AVR usually has 2-byte addresses.  Maybe that's a broader issue for a separate patch but there are a lot of 16-bit machines still out there in the world.
> I just happened to notice D73961 which says AVR usually has 2-byte addresses. Maybe that's a broader issue for a separate patch but there are a lot of 16-bit machines still out there in the world.

Thanks for flagging that up. I saw the 4/8 byte support check in several other places, so copied those. I'll expand this to 2 and 1 too, since that's what the DataExtractor can support. The other places can be fixed as needed.

> I'd generally like to assume that the /first/ time a value/field/length/etc is seen, that is correct and anything later that conflicts with it is incorrect.
I've been following the approach of assuming the length field is correct (in this case, the length of the extended opcode). Indeed, in this case, if we were to follow a different approach, we could read past the end of the instruction.

> In what cases would we reach here anyway?
We hit here any time the length field of a DW_LNE_set_address opcode is not either 5 or 9 (i.e. 1 for the opcode, plus 4/8 for the address).

> Could we check earlier and ensure the extractor has a meaningful (non-zero) address size?
I'm deliberately not checking that any earlier stated address makes sense, because it's not actually needed until here. If a V5 line table had an address of, say, 7, it wouldn't matter if the table had no DW_LNE_set_address opcodes, so an earlier validation would lead to an error that has no impact. @probinson (I think) previously encouraged me to be lazy in reporting this sort of thing.

Additionally, we don't always have any address size information until this point - in the case where e.g. llvm-dwarfdump is dumping just the whole of a V4 .debug_line section and there is no corresponding debug info unit, the parser knows nothing about the address until it hits this point.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73962