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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp:25
+                          lldb::offset_t *offset_ptr) {
+  assert(debug_info.ValidOffset(*offset_ptr));
+
----------------
zturner wrote:
> 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
As long as everyone builds with assertions off for release mode we are good. Apple builds used to not do this, but now I believe they do. So all good, since if this falls through, then the length won't be ok and we will exit just fine with an error.


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

https://reviews.llvm.org/D59381