[Lldb-commits] [PATCH] D59381: Change CompileUnit and ARanges interfaces to propagate errors

[Zachary Turner via Phabricator via lldb-commits]

zturner marked 2 inline comments as done.
zturner added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp:25
+                          lldb::offset_t *offset_ptr) {
+  assert(debug_info.ValidOffset(*offset_ptr));
+
----------------
clayborg wrote:
> We have error checking now, use it instead of asserting? We are checking it with the assert anyways, so might as well check and return an error?
The reason I changed this is because the logic is simpler if we assert, and I examined all call-sites and ensured that the value is already sanitized before we call this function.

In this case, the only actual call to this function is here:

```
while (debug_info_data.ValidOffset(offset)) {
    llvm::Expected<DWARFUnitSP> cu_sp =
        DWARFCompileUnit::extract(m_dwarf2Data, debug_info_data, &offset);
   ...
}
```

so we have already verified that the offset is valid before we even call the function.  In fact, that is the most natural way to parse compile units anyway, by running a loop until you're at the end of the data, so it would be hard to imagine someone could actually intend to call the function

Because of this, I'm considering the assert here to be an enforcement of internal consistency and not one of user input


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp:46
+llvm::Expected<DWARFDebugAranges &> DWARFDebugInfo::GetCompileUnitAranges() {
+  assert(m_cu_aranges_up && m_dwarf2Data);
+
----------------
clayborg wrote:
> This logic is wrong. We cache the m_cu_arange_up. If it is NULL then we build. If not, we return what we have. Maybe change this to:
> 
> ```
> if (m_cu_aranges_up)
>   return *m_cu_aranges_up;
> ```
Yes, you are right.  I'm at home today on my Windows box so I couldn't really run the test suite, but that certainly would have caught this.  Thanks for pointing it out!  I'll make sure to fix when I'm back at work before submitting.


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

https://reviews.llvm.org/D59381