Page MenuHomePhabricator

[lldb] Remove asserts in CommandReturnObject SetError and AppendError
ClosedPublic

Authored by DavidSpickett on Jun 23 2021, 4:38 AM.

Details

Summary

I added asserts to these in https://reviews.llvm.org/D104525.
They are available (directly or otherwise) via the API so we
should not assert.

Restore the previous behaviour. If the message
is empty, we return early before printing anything.
For SetError don't assert that the error is a failure.

The remaining assert is in AppendRawError which
is not part of the API.

Diff Detail

Event Timeline

DavidSpickett requested review of this revision.Jun 23 2021, 4:38 AM
DavidSpickett created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2021, 4:38 AM
teemperor accepted this revision.Jun 23 2021, 4:39 AM
teemperor added a subscriber: teemperor.

LGTM, I'll add them in my patch. Thanks for the quick turnaround!

This revision is now accepted and ready to land.Jun 23 2021, 4:39 AM

As noted, the assert in AppendRawError remains. It has two internal callers and no API definition.

LGTM, I'll add them in my patch. Thanks for the quick turnaround!

To clarify, is there still a regression with the API?

  • This removes the asserts so you won't crash
  • No functions are changed in lldb/include/lldb/API/SBCommandReturnObject.h

The one thing that is different is that calling the API SetError with an empty string will now set eReturnStatusFailed. So:

s = do_thing_and_return_err_string_if_err(...)
return_object.SetError(s)

Would now set it to failed even if s is empty.

Perhaps the SetError(...) functions should retain the empty message == nop behaviour and AppendError can set the status unconditionally?

LGTM, I'll add them in my patch. Thanks for the quick turnaround!

To clarify, is there still a regression with the API?

No regression anymore from what I can see. My point was that I'll fix the usage from the SB API and then I can just re-add the asserts in the same patch (the asserts themselves are a good idea, the SB API just should have been updated to prevent hitting them from user scripts)

  • This removes the asserts so you won't crash
  • No functions are changed in lldb/include/lldb/API/SBCommandReturnObject.h

The one thing that is different is that calling the API SetError with an empty string will now set eReturnStatusFailed. So:

s = do_thing_and_return_err_string_if_err(...)
return_object.SetError(s)

Would now set it to failed even if s is empty.

Perhaps the SetError(...) functions should retain the empty message == nop behaviour and AppendError can set the status unconditionally?

I think the assert behaviour is fine for the internal API, but the SB API behaviour is a bit more tricky. But let's discuss the expected behaviour in a dedicated review for that.

This revision was landed with ongoing or failed builds.Jun 23 2021, 6:11 AM
This revision was automatically updated to reflect the committed changes.