Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
swift-ci opened this issue Mar 9, 2016 · 25 comments
Open

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

swift-ci opened this issue Mar 9, 2016 · 25 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. standard library Area: Standard library umbrella

Comments

@swift-ci
Copy link
Collaborator

swift-ci commented Mar 9, 2016

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
Copy link
Contributor

belkadan commented Mar 9, 2016

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
Copy link

groue commented Mar 31, 2016

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
Copy link
Contributor

cc @gribozavr

@gribozavr
Copy link
Collaborator

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
Copy link

groue commented Apr 10, 2016

@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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

> 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
Copy link
Collaborator Author

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
Copy link
Collaborator

>> 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
Copy link
Collaborator Author

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
Copy link
Collaborator

> 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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link

groue commented Apr 11, 2016

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...

@gribozavr
Copy link
Collaborator

> 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.

This is actually a completely different argument and it is much more convincing than anything that was said on this thread. This case is completely different since we are talking about the case when no source information is available.

@belkadan
Copy link
Contributor

belkadan commented Jun 1, 2018

I've definitely changed my opinion on this since the original comment; a recent example is this Twitter thread where the developer didn't know what precondition they were violating (or even that that rule existed, since it wasn't documented). It might still be a good idea to omit the source location info.

@belkadan
Copy link
Contributor

belkadan commented Jun 1, 2018

@swift-ci create

@swift-ci
Copy link
Collaborator Author

swift-ci commented Jun 7, 2018

Comment by Marc Palmer (JIRA)

I would like to lend my support here.

I am building an open source framework that people install with Carthage. Carthage builds dependencies in release mode. This means developers using my framework cannot tell what has happened even in their own apps' debug builds locally when building using my dependencies if I use precondition.

I now have to go through and remove all precondition uses and replace with:

guard ... else { fatalError( ... ) }

...which is pretty brutal.

@beccadax
Copy link
Contributor

beccadax commented Jul 5, 2018

Just gonna drop a couple “third ways” here which might get us code size or secrecy gains without losing descriptive assertions:

1. Compress assertion strings.
2. Replace assertion strings with small, opaque integer IDs, put a table of them in the debug info, and look up the assertion text when you symbolicate.

@jckarter
Copy link
Member

jckarter commented Jul 5, 2018

Our code generation already preserves the identity of traps by generating a distinct trap instruction for every precondition failure, and the DWARF information should preserve the mapping from trap instruction to source location, so if you have a dSYM you can symbolicate and recover source location info that way.

@jckarter
Copy link
Member

jckarter commented Jul 5, 2018

It looks like we don't drop the source location information even from `fatalError`s in release builds, which we really should, since it's a secrecy leak and someone with a dSYM can recover the source information from the debug info. Once we do that, @airspeedswift and I agree that it makes sense for `preconditionFailure` to behave effectively like `fatalError`, logging the reason string alone before ending the process.

@swift-ci
Copy link
Collaborator Author

swift-ci commented Jul 5, 2018

Comment by Marc Palmer (JIRA)

@jckarter that would appear to be correct about the current situation, if I understand you right. I have seen that fatalError(s) from my framework that are experienced inside an app will let me click through to the source of framework where the fatalError was, even for a release build of my framework.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@JensAyton
Copy link

In an ideal world, where symbolication of arbitrary released builds was easy and accessible to all, the best thing might be for the (static parts of) precondition messages and source locations to be encoded in the debug symbols as well.

@BarAgent
Copy link

Hey. Why hasn't this been fixed yet? It's been a problem since 2016.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. standard library Area: Standard library umbrella
Projects
None yet
Development

No branches or pull requests

8 participants