dblaikie added inline comments.
================ Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:27 +template <typename... Ts> +static llvm::Error createError(const char *Fmt, Ts &&... Vals) { + return createStringError(inconvertibleErrorCode(), Fmt, Vals...); ---------------- I guess "Ts &&... Vals" should be "const Ts &... Vals" since they're taken by const ref by createStringError anyway - no need for the fancy &&. ================ Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:31 + +static llvm::Error createOverflowError(const char *Section) { + return createError("location list overflows the %s section", Section); ---------------- Should this be StringRef rather than const char*? ================ Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:171 + case dwarf::DW_LLE_offset_pair: E.Value0 = Data.getULEB128(Offset); + break; ---------------- Looks to me like getULEB128 doesn't quite have the right error handling, if I'm reading it correctly: unsigned shift = 0; uint32_t offset = *offset_ptr; uint8_t byte = 0; while (isValidOffset(offset)) { byte = Data[offset++]; result |= uint64_t(byte & 0x7f) << shift; shift += 7; if ((byte & 0x80) == 0) break; } *offset_ptr = offset; return result; I /imagine/ it shouldn't update offset_ptr if it breaks out of the loop via !isValidOffset, rather than via the break? More broadly, I wonder if we should consider a more convenient way to do error handling here - since it's a bit unfortunate that you've had to split the logic for parsing these things across two switch statements - makes it a bit hard to follow what shape each LLE entry has, since it's spread out like this. ================ Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:220 } - return LL; + llvm_unreachable("Exit from function inside while loop!"); } ---------------- Given the loop condition is "while (true)" this unreachable seems a bit unnecessary (& the function has non-void return, so if there was a path that got through the loop I imagine the compiler would warn us about that?) Or is this working around a compiler that warns here despite the lack of any path out of the loop? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63591/new/ https://reviews.llvm.org/D63591 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits