This is a continuation of D59370 for compile units and aranges.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Must fix logic error as mentioned in inlined comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp | ||
---|---|---|
25 ↗ | (On Diff #190700) | 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? |
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp | ||
46 ↗ | (On Diff #190700) | 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; |
lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp | ||
---|---|---|
25 ↗ | (On Diff #190700) | 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 |
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp | ||
46 ↗ | (On Diff #190700) | 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. |
lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp | ||
---|---|---|
25 ↗ | (On Diff #190700) | 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. |
@zturner, this probably should be