[PATCH] D45549: [DWARF v5] improved support for .debug_rnglists/consumer

[Wolfgang Pieb via Phabricator via llvm-commits]

wolfgangp added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugRnglists.h:120
+    default:
+      llvm_unreachable("Invalid DWARF format (expected DWARF32 or DWARF64");
+    }
----------------
aprantl wrote:
> JDevlieghere wrote:
> > Is `Format` checked before this function is called? If not, this should probably also be an `Optional` as it can fail due to bad input. Otherwise I'd add a comment specifying this as a pre-condition. 
> Nit: By moving the unreachable outside of the switch statement, we could benefit from the incomplete-switch warning if a new enumerator is added.
> Is Format checked before this function is called? If not, this should probably also > be an Optional as it can fail due to bad input. Otherwise I'd add a comment 
> specifying this as a pre-condition.


Actually, format is (or rather, will be once DWARF64 support is added) derived from parsing the table, and it really can only be either DWARF32 or DWARF64 (or something else to be added in the future). If it isn't it's clearly an internal error. I'll add a comment to that effect.





================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:255
+    if (Error Err = Rnglists.extract(rnglistData, &Offset)) {
+      errs() << "error: " + toString(std::move(Err)) << '\n';
+      uint64_t Length = Rnglists.length();
----------------
aprantl wrote:
> Not your code, but since you touched it: IIRC there's supposed to be a helper function that prints "error:" in color somewhere in libDebugInfo.
Found it. So far it seems to be only used by the verifier.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp:200
+    default:
+      llvm_unreachable("Unsupported range list encoding");
+    }
----------------
JDevlieghere wrote:
> Same as my other comment, we shouldn't crash on bad input.
> Same as my other comment, we shouldn't crash on bad input.
Any unsupported encodings should have been found during the initial extraction of the range list, which would have prevented the list from being internalized. So at this point we expect a list with only supported encodings. I'll add a comment to that effect.


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:152
+// following it (DWARF v5 and later).
+static Optional<DWARFDebugRnglistTable>
+parseRngListTableHeader(DWARFDataExtractor &DA, uint32_t Offset) {
----------------
JDevlieghere wrote:
> Would it make sense to turn this into an Expected and propagate the Error from `extractHeaderAndOffsets`?
Good idea. This allows reporting of errors when we first parse the range list table.


https://reviews.llvm.org/D45549