Page MenuHomePhabricator

[lldb] Replace debug-only assert in AppleObjCTypeEncodingParser::BuildObjCObjectPointerType with lldbassert
Needs ReviewPublic

Authored by teemperor on Mar 24 2020, 5:57 AM.

Details

Reviewers
JDevlieghere
davide
Group Reviewers
Restricted Project
Summary

This assert is only checked in Debug builds but ignored in all other builds. This replaces this code with lldbassert
which should print a warning to the user in release builds and actually asserts in debug builds.

Diff Detail

Event Timeline

teemperor created this revision.Mar 24 2020, 5:57 AM
teemperor updated this revision to Diff 252289.Mar 24 2020, 5:59 AM
  • Upload correct diff.
This revision is now accepted and ready to land.Mar 24 2020, 9:03 AM
davide added a subscriber: davide.Mar 24 2020, 10:50 AM

Please don't do this. I've seen that assertion triggering. If anything, you might want to make a lldbassert.

davide requested changes to this revision.Mar 24 2020, 10:50 AM
This revision now requires changes to proceed.Mar 24 2020, 10:50 AM

Please don't do this. I've seen that assertion triggering. If anything, you might want to make a lldbassert.

Did you investigate why the assertion triggered? Was it because of the forward declaration or was it one of the "other weird cases". To me this change seems perfectly in line with https://lldb.llvm.org/resources/contributing.html

I don't recall the details.

Soft assertions. LLDB provides lldb_assert() as a soft alternative to cover the middle ground of situations that indicate a recoverable bug in LLDB. In a Debug configuration lldb_assert() behaves like assert(). In a Release configuration it will print a warning and encourage the user to file a bug report, similar to LLVM’s crash handler, and then return execution. Use these sparingly and only if error handling is not otherwise feasible. Specifically, new code should not be using lldb_assert() and existing uses should be replaced by other means of error handling.

This is a recoverable bug, in my books.

Also, this was asserting already -- so it's not introducing any new assert().

According to the comment there is at least one valid reason for this code path to trigger. An lldb_assert will encourage users to file bugs and I think we have enough real issues that we can do without the additional noise.

davide added a comment.EditedMar 24 2020, 11:54 AM

According to the comment there is at least one valid reason for this code path to trigger. An lldb_assert will encourage users to file bugs and I think we have enough real issues that we can do without the additional noise.

I do believe users should file bugs if this triggers, yes. In the recent past, I looked at ObjC more than most, so these bugs can land on me.

teemperor updated this revision to Diff 252517.Mar 25 2020, 2:02 AM
teemperor retitled this revision from [lldb] Remove Debug-only assert in AppleObjCTypeEncodingParser::BuildObjCObjectPointerType to [lldb] Replace debug-only assert in AppleObjCTypeEncodingParser::BuildObjCObjectPointerType with lldbassert.
teemperor edited the summary of this revision. (Show Details)
  • moved to lldbassert
kwk added a subscriber: kwk.Mar 26 2020, 4:23 AM

Let's summarize this.

  1. In the old and the new version, the program is aborted with an assert() if the configuration is LLDB_CONFIGURATION_DEBUG.
  2. In the old version, nothing happens, when the configuration is non-Debug.
  3. In the new version, the assert in non-Debug configurations will be turned into a warning and *print a warning and encourage the user to file a bug report* (@JDevlieghere that is the noise you're referring to, right?)

@kkleine yes, that's correct.

So what should happen with this?

vsk added a subscriber: vsk.Apr 13 2020, 9:08 AM

The change looks reasonable to me. @davide?