Page MenuHomePhabricator

Change CompileUnit and ARanges interfaces to propagate errors

Authored by zturner on Mar 14 2019, 12:10 PM.

Diff Detail


Event Timeline

zturner created this revision.Mar 14 2019, 12:10 PM
clayborg requested changes to this revision.Mar 14 2019, 4:24 PM

Must fix logic error as mentioned in inlined comments.

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?

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;
This revision now requires changes to proceed.Mar 14 2019, 4:24 PM
zturner marked 2 inline comments as done.Mar 14 2019, 4:59 PM
zturner added inline comments.
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

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.

clayborg added inline comments.Mar 14 2019, 10:33 PM
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.

This revision was not accepted when it landed; it landed in state Needs Revision.Mar 15 2019, 10:32 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2019, 10:32 AM
ikudrin added inline comments.

@zturner, this probably should be

if (error)
  return std::move(error);