Page MenuHomePhabricator

lldb_assert: abort when assertions are enabled.
ClosedPublic

Authored by JDevlieghere on Jul 5 2019, 1:51 PM.

Details

Summary

We had a long discussion in D59911 about lldb_assert and what it means. The result was the assert manifesto on https://lldb.llvm.org/resources/source.html:

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.

However, currently lldb_assert doesn't behave they way it's being described there: it doesn't abort in a debug/assert build.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Jul 5 2019, 1:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2019, 1:52 PM
Herald added a subscriber: abidh. · View Herald Transcript

Add comments with the exact wording from the docs.

JDevlieghere edited the summary of this revision. (Show Details)Jul 5 2019, 1:59 PM

Would it be possible to assert after the printing the stack trace? Having stack traces in bug reports is always nice to have and displaying the "Assertion failed: ..." message seems also useful for the user.

JDevlieghere added a comment.EditedJul 5 2019, 2:11 PM

Would it be possible to assert after the printing the stack trace? Having stack traces in bug reports is always nice to have and displaying the "Assertion failed: ..." message seems also useful for the user.

The stack trace will be printed when the assert is hit, because we have a call to LLVM's pretty stack trace printer in the signal handler. That's the reason I moved the assert *before* the PrintStackTrace call.

Regarding the Assertion failed message, I wasn't sure if we wanted to print that twice, once coming form lldb_assert (with the real function, file and line) and once from the assert (with the function, file and line pointing to the lldb_assert implementation). If you think it's useful I'm happy to move the assertion to just after that print statement.

teemperor accepted this revision.Jul 5 2019, 2:15 PM

Would it be possible to assert after the printing the stack trace? Having stack traces in bug reports is always nice to have and displaying the "Assertion failed: ..." message seems also useful for the user.

The stack trace will be printed when the assert is hit, because we have a call to LLVM's pretty stack trace printer in the signal handler. That's the reason I moved the assert *before* the PrintStackTrace call.

Regarding the Assertion failed message, I wasn't sure if we wanted to print that twice, once coming form lldb_assert (with the real function, file and line) and once from the assert (with the function, file and line pointing to the lldb_assert implementation). If you think it's useful I'm happy to move the assertion to just after that print statement.

Ah, thanks for the explanation! The message was more of a positive side-effect of moving it afterwards, but I don't think it's worth moving this just for that message. So I think the current version is good to go then.

This revision is now accepted and ready to land.Jul 5 2019, 2:15 PM
davide accepted this revision.Jul 5 2019, 2:20 PM
davide added inline comments.
lldb/source/Utility/LLDBAssert.cpp
25–26 ↗(On Diff #208235)

llvm_unreachable?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2019, 2:22 PM
labath added a comment.Jul 8 2019, 1:30 PM

lldbassert(x) already expands to assert(x) in a debug (LLDB_CONFIGURATION_DEBUG) build. It sounds like you want this to assert in a non-debug build which has assertions enabled (Release+Assertions) ? If that's the case, then I think we should just change the #ifdef LLDB_CONFIGURATION_DEBUG in LLDBAssert.h into #ifdef _DEBUG..

Did you see this recent commit?

commit 515d1306ffb1e159c65b19d4cbe6c2f0997dfbf6
Author: Adrian Prantl <aprantl@apple.com>
Date:   Fri Mar 29 16:12:27 2019 +0000

    Don't abort() in lldb_assert and document why.
    
    rdar://problem/49356014
    
    Differential Revision: https://reviews.llvm.org/D59911
    
    llvm-svn: 357268

Actually, it looks like this patch fixed an oversight in that commit. So all is good :-)