[SR-905] precondition failure should show message in crash report

[swift-ci]

Previous ID	SR-905
Radar	rdar://problem/40723738
Original Reporter	swillits (JIRA User)
Type	Bug

Environment

OS X

Additional Detail from JIRA

Votes	5
Component/s	Standard Library
Labels	Bug
Assignee	None
Priority	Medium

md5: e8379b37c0510613305dc3534aa2b4d1

Issue Description:

On OS X, if a precondition fails, the crash report contains no mention of the precondition. By contrast a fatalError("The message.") call, in the crash report does show:

fatal error: The message.: file /path/to/file.swift, line 55

precondition failures should do the same thing, just as fatalError() does and just as assert() does in C/Obj-C.

It is far more useful to have this information in the crash report than not, and there's no reason it shouldn't appear. I consider this a bug as the message is simply lost in a release configuration even though the precondition is still checked.

[belkadan]

I'd be careful here—the contents of such messages may be a secrecy leak (not to mention code size bloat). I think it's probably still the right thing to do even in Release builds, but it's not obvious. (I guess we already have this behavior for fatalError, as you say.)

[groue]

Let me try to foster this issue.

Users complain that libraries that use precondition() can not communicate the reason of the error:

groue/GRDB.swift#37

Some libraries have already jump in the wagon of avoiding precondition, and use fatalError instead:

https://github.com/SnapKit/SnapKit

This issue is a serious threat to the usefulness of precondition.

[belkadan]

cc @gribozavr

[gribozavr]

IIRC we intentionally don't save precondition messages in release builds for code size reasons. The person fixing this should show that the code size impact is minimal.

[groue]

@gribozavr Please consider revising your opinion, for the following practical reason. The many Swift libraries that ship today still target Apple operating systems, and are widely distributed by package managers like CocoaPods and Carthage, which you may have heard about. Carthage, in particular, is getting traction. And Carthage rebuilds release frameworks, not debug frameworks. It is hence very difficult for the host application to get the error of the precondition, and just for sake of binary size.

Don't you think we developer want to use precondition instead of fatalError precisely because we read the documentation that says that precondition remains in release builds???

Myself, as a library author, aiming at a wide audience, have to take this seriously. I must switch to fatalError in order to be a friendly library, and have to see this issue as a bug in the precondition function.

[gribozavr]

Thank you for your feedback. I consider this a defect in the mentioned package management systems.

Imagine that we made the change. Now your application can still trap in a binary framework in a force-unwrap of an optional, or in checked arithmetic. There is no hope to debug those without a full debug build of the binary framework.

> It is hence very difficult for the host application to get the error of the precondition, and just for sake of binary size.

Binary size is very important. You might be shipping the app to millions of users. Bigger applications don't just mean longer download size. They also mean less space for data that is important for the users.

It is not the host application that needs to get the error description, it is the developer. And developers should be able to use better tools. If we design the language around limitations of the current tools, we won't arrive at a better language.

[swift-ci]

Comment by Seth Willits (JIRA)

Two thoughts:

1) How is the message less important in a release build than it is in a debug build? Presumably in a debug build, one is most often running with the debugger attached, and any dynamic values shown in the message can easily be accessed through the debugger itself. The message in that case can be viewed simply as automated debugger commands. In the case of a release build however, without the message being logged somewhere, that information is completely lost, and yet that information is potentially critical to finding the source of the error.

2) The concern about binary size seems to imply either that the mechanism for storing these messages is so inefficient that it adds significant size even when having just a few messages, or that precondition() is expected to be used liberally in all functions with preconditions, and thus the inclusion of messages in builds adds sizable space requirements.

In the former case, it seems the inefficiency itself should be fixed, and in the latter case, why is it the language's job to assume it knows what I want to do? Is there a reason this can't be a compiler option? I do want these messages, even at the expense of increased file size, because I can't imagine the space being used in any of my applications to ever add up to any size of consequence (it certainly isn't an issue with my liberal use of assert() in Obj-C for example), and I really do want to see the message.

I humbly admit I don't know every factor in making this decision, but at the very very least, it seems completely silly to have to use a wrapper function around fatalError just so that the message appears in the crash log, when precondition() by name is intended for exactly what I want, and by behavior does exactly what I want – at least in debug builds.

[gribozavr]

> How is the message less important in a release build than it is in a debug build?

A release build runs in the field. In most cases, the message even if logged, would be logged to a place where nobody would actually read it.

If you get a crash log though, you would be able to symbolicate it and get complete information about source files and line numbers throughout the stack trace.

> The concern about binary size seems to imply either that the mechanism for storing these messages is so inefficient that it adds significant size even when having just a few messages, or that precondition() is expected to be used liberally in all functions with preconditions, and thus the inclusion of messages in builds adds sizable space requirements.

Neither. It implies that every byte counts.

Please also read my comment above, this part:

> Imagine that we made the change. Now your application can still trap in a binary framework in a force-unwrap of an optional, or in checked arithmetic. There is no hope to debug those without a full debug build of the binary framework.

[swift-ci]

Comment by Seth Willits (JIRA)

> If you get a crash log though, you would be able to symbolicate it and get complete information about source files and line numbers throughout the stack trace.

You make it sound so easy. :-)

1) Symbolication is a major pain in the neck. Getting the right dsyms from the right build, having to run it through atos on the command line to simply get a line number, then checkout the old version of the code for that particular build, find the right line number, just to get the assertion message is a lot of manual tedious work. And to do it for multiple crash reports is the most painfully annoying thing there is in the entire development process.

Yes, this is something tools could make better, but until Xcode comes along with a new feature (that isn't limited to App Store apps only) and saves us from this tedium, there aren't going to be tools for this that aren't custom written, which is far from easy. (I'm already down that road, and have spent a lot of time doing exactly this.)

2) On top of that, a major thing that is different in Swift is that the logged message is actually dynamic — unlike assert() in C where the message is simply the static line of code that threw the assertion. In Swift, if a precondition fails, the message can include other related properties' runtime values which might explain the conditions that lead to the failure. Symbolicating and getting a line number back can't compare to that. (Yes, with some assembly nonsense you can inject that extra info into the crash log, and again wrap precondition() in another function that makes it palettable...)

So yes, it's all possible that we can make make better tools and save space, etc, but as the practical matter exists in my situation, (and I can't help but think many others'), having the message in the log is immensely more convenient, and lets me find and correct problems quicker. I have made the conscious choice that the small space saving is less important than that.

Is there a reason that this cannot at least be a compiler option?

> Please also read my comment above, this part:
>> Imagine that we made the change. Now your application can still trap in a binary framework in a force-unwrap of an optional, or in checked arithmetic. There is no hope to debug those without a full debug build of the binary framework.

I'm sorry, but I don't know what point this is trying to make.

[gribozavr]

>> Please also read my comment above, this part:
>>> Imagine that we made the change. Now your application can still trap in a binary framework in a force-unwrap of an optional, or in checked arithmetic. There is no hope to debug those without a full debug build of the binary framework.
> I'm sorry, but I don't know what point this is trying to make.

My point is that even if we make the requested change, there are many other ways your application can crash without a useful message (or with a message that is useless without a precise stack trace), and you still would need to debug it.

[swift-ci]

Comment by Seth Willits (JIRA)

Ok. I'm not sure if anyone else suggested otherwise, but at least I am certainly aware of that.

---

In summation, I strongly feel that the assumption that space savings is always more important than seeing the message is simply flawed and that this should at least be a compiler option.

[gribozavr]

> Ok. I'm not sure if anyone else suggested otherwise, but at least I am certainly aware of that.

So what is the point of including the message then, if you would still need to debug the app as usual probably in the majority of cases, especially given that including the message is not free? What problem does it solve?

[swift-ci]

Comment by Seth Willits (JIRA)

Hmm... I can't help but think that with that logic, it seems like you should be asking "what's the point of the message ever, even in debug builds, since you're just going to debug it anyway?"

There are two major reasons:

1) I can clearly see what condition caused the crash, and dynamic related info, in the release build's crash log without having to jump through umpteen different hoops:

1. Copy the symbol address in the crash log

2. Copy the stride offset for the correct library for that address

3. Go into Xcode

4. Open the organizer

5. Find the right build archive

6. Go through the hassle of revealing the archive in Finder and navigating through 4 layers of folders to find the correct dsym file

7. Open Terminal

8. Lookup what atos's command options are again

9. Stick in the path to the dsym and the addresses

10. Get the file and line number from atos

11. Go to my SCM, Checkout the right build (because how else are line numbers going to line up?)

12. Find the right file

13. Jump to the given line number

... and do that for each crash report. That's an insanely inefficient workflow.

(Tools can be made and do exist to make at least some of that simpler, but not everyone has the ability or the desire to make their own, or even use someone else's.)

I want to see the message and file etc in the crash log itself, without having to do aaaaaall of that work, much like an assert does in C/Obj-C already. The precondition message combined with the stack frame will uniquely point to exactly which precondition/assert failed, which means the exact line number is irrelevant. Unlike having to use atos to convert the address into a line number (which I swear is very often wrong by one line number), and being required to use scm to checkout the old build just so I can be (almost) sure which precondition failed, if the message is in the crash log, it's simply plain as day and I can look at it in the current version of the code, and be absolutely 100% certain I'm looking at the precondition that failed. It's vastly more convenient.

2) The second major reason is that the message parameter allows the message in the crash log to contain other related runtime values, which can help explain how to reproduce the bug. It's like focused site-of-the-crash logging. In Obj-C I've made very effective use of:

if (!precondition) {
    StickAStringInTheCrashLog([NSString stringWithFormat:@"....", a, b, c]);
    assert(precondition);
}

where the string gets inserted into "Application Specific Info" via use of:

static char *__crashreporter_info__ = nil;
asm(".desc ___crashreporter_info__, 0x10");

This has let me capture related values for hard-to-track assertion failures which give insight into how to recreate the conditions that led to the failure. In Obj-C it's necessary to use that entire if-statement because when an assert() fails the expression is printed literally in the log, meaning it's impossible to include actual values. In Swift, however, we are blessed with the behavior that precondition(), upon failure, will actually print the message parameter's string value, so that it can include dynamic values. eg:

func foo(x: Int) {
    precondition(x < 3, "x is \(x) -- expected < 3")
    ...
}

Without including the message in the crash log in release builds, I can't know what the value of x was, which could be really really important. Maybe I can only possibly imagine that the unexpected value is 4, but for a particular user I can see directly in the crash log that the value is 5, which turns out to be mind blowingly useful information. Generally speaking, without introducing a dedicated log file or asking the user to go dig around in Console.app ——— both of which requires actual contact info for and participation from my user ——— I may never get this info, and related to a specific file. But if the message is in the crash log, which even anonymously is submitted to me, now I have it.

Extremely useful.

[swift-ci]

Comment by Robert Payne (JIRA)

As the author of SnapKit which has quite a few users…

I typically use preconditions to avoid developer programmer errors. Things like you're calling this on an invalid/wrong thread, the supported arguments are invalid even though it wasn't user input etc…

The issue as a framework developer is we can't use these to ensure developers know they are making programmer errors because the information is lost in a release build of the framework.

The argument that you would 'have to debug it anyways' is sort of a scapegoat. Think about how many exceptions Apple's frameworks in Objective-C fire when you make a programmer error and then imagine the developer experience if those exceptions contained no information and because you don't have the dsyms you don't even see where it's coming from. That is essentially what preconditions are like for developers using frameworks that were written in Swift with preconditions, granted in most cases you do get the dsyms and have access to at least some information.

For now what we have to do in SnapKit is use fatalError because the message is preserved, either way we want the app to terminate and stop executing immediately. We'd prefer to use a precondition because logically it makes more sense.

[groue]

We'd prefer to use a precondition because logically it makes more sense.

More: precondition is documented to help the compiler assume some truth in unchecked builds. With all preconditions replaced with explicit checks and fatalErrors, those optimizations will not apply: unchecked builds are less efficient. Library developers are punished for helping out their users with explicit error messages...