Asserts should not cause undefined behaviour


(Joseph Lord) #1

I propose that assert and assertionFailure should be no-ops (with branch hints) in unchecked builds as they are in normal release builds rather than resulting in undefined behaviour in the failure condition.

I would like to kick off a discussion of this. I found the proposal template useful for setting out my thoughts clearly to trigger the discussion but I'm not trying jump the discussion phase. I'm open to radical rewrites or to not proceed if there is opposition. Also if my understanding of the current code is wrong please let me know.

[Current Situation]

The documented behaviour of assert and assertionFailure in "disable safety checks" builds (still documented as -Ounchecked) is that the compiler "may assume that it would evaluate to true" or in the assertionFailure case never be called.

This documented behaviour would allow the compiler to completely eliminate tests and branches before or after the assertion and take the operation deep into undefined behaviour.

https://github.com/apple/swift/blob/cf8baedee2b09c9dd2d9c5519bf61629d1f6ebc8/stdlib/public/core/Assert.swift (latest commit to this file at time of writing)

[NOTE - Actual current behaviour]

It appears from the code as if the assumption is not currently applied on the assert method although it is on the assertionFailure case by means of the _conditionallyUnreachable() call. assert seems to be a no-op in both normal release and disable safety checks build modes.

It also appears that precondition does not apply the permitted assumption when in _isFastAssertConfiguration mode.

There also appears to be code in assert to hint the compiler that the assert will likely be true. This was something that I was planning to suggest and means that code containing asserts can be faster than the same code without the assert in release mode.

[Proposed Change]

Change the documentation for assert and assertionFailure so that behaviour in unchecked mode is the same as in normal release - no evaluation and no effect.

Change the behaviour of assertionFailure to match the updated documentation.

[Motivation]

1) Expected behaviour based on other languages is for assert to have no effect in release. (If current behaviour is highly desired another function name should be used). Having potential dangerous behaviour from a function that is familiar across languages and is regarded as a safety feature is undesirable.

2) Adding asserts to code should not make the code more dangerous whatever the build. Assuming the truth of the assert may lead to runtime safety checks being skipped and undefined behaviour when a no-op would be a safe behaviour.

3) "For highly robust code assert and then handle the error anyway" [Code Complete 2nd Edition section 8.2] While the designed expectation from the Swift team is to use them for checks against internal programming errors within a module in a number of cases I use assertions and assertionFailure calls while processing input data. The code without the assertion would simply fail to read the input gracefully but as the developer I want to know immediately if there are realistic cases that have not been handled so use assertions to flag it to myself that a case may need better handling. As such there are assertions in my code that are in error cases that I expect not to occur. I do not want the runtime safety checks being optimised out. [Being aware of this issue I use custom assertions but am therefore missing out on the branch hinting of the stdlib version and others may want similar behaviour and may not fully read the documentation].

[Impact on existing code]

Loss of performance in cases where assertionFailure is used. Loss of potential performance improvement option in assert usage. In most cases this performance loss will be small but there is potential where the assumption of the value allows large code blocks (to evaluate the condition) to be eliminated for significant effects but I suspect that these cases are rare and preconditionFailure could be used instead to get the performance in unchecked builds (at the cost of release performance) or an additional method could be added.

[Alternatives]

Renaming assert so that the behaviour is not assumed by users familiar with other languages. Function name suggestion:

assume/assumeUnreachable

This could either behaviour as the current assert/assertionFailure documentation or possibly allow the assumption to be made in normal release mode not just the unchecked. It should be a fatal error in debug builds if assumption is incorrect.

[Note about precondition]

precondition/preconditionFailure also have undefined behaviour in unchecked mode but this is not a problem for me for a couple of reasons:

1) I don't have the same prior understanding about what they do.

2) It is clear from release build behaviour that hitting the condition is always an error.


(Chris Lattner) #2

The documented behaviour of assert and assertionFailure in "disable safety checks" builds (still documented as -Ounchecked) is that the compiler "may assume that it would evaluate to true" or in the assertionFailure case never be called.

Right.

This documented behaviour would allow the compiler to completely eliminate tests and branches before or after the assertion and take the operation deep into undefined behaviour.

Only in cases where the assertion would have failed, right? The point of -Ounchecked is that - if your code was correct with the checks - that it will still be correct. Disabling overflow and array bounds checks is far more dangerous than the assertion behavior you cite here.

It appears from the code as if the assumption is not currently applied on the assert method although it is on the assertionFailure case by means of the _conditionallyUnreachable() call. assert seems to be a no-op in both normal release and disable safety checks build modes.

Right.

[Proposed Change]

Change the documentation for assert and assertionFailure so that behaviour in unchecked mode is the same as in normal release - no evaluation and no effect.

Why? :

1) Expected behaviour based on other languages is for assert to have no effect in release. (If current behaviour is highly desired another function name should be used). Having potential dangerous behaviour from a function that is familiar across languages and is regarded as a safety feature is undesirable.

This is the C model, but as you know, there is a whole field of custom assertions libraries that people have developed. I don’t think there is anything like consensus on this topic.

2) Adding asserts to code should not make the code more dangerous whatever the build. Assuming the truth of the assert may lead to runtime safety checks being skipped and undefined behaviour when a no-op would be a safe behaviour.

This only affects code built with -Ounchecked, which is definitely not a safe mode to build your code. The intention of this mode is that you can use it to get a performance boost, if you believe your code to be sufficiently tested. This mode, which isn’t the default in any way, intentionally takes the guard rails off to get better performance.

If you don’t like that model, don’t use this mode.

3) "For highly robust code assert and then handle the error anyway" [Code Complete 2nd Edition section 8.2]

Highly robust code shouldn’t build with -Ounchecked, so I don’t see how this point is pertinent.

-Chris

···

On Dec 28, 2015, at 5:48 AM, Joseph Lord via swift-evolution <swift-evolution@swift.org> wrote:


(Dave Abrahams) #3

Let’s just consider -O; I think I understand Joseph’s objection here, and it seems valid.

Normally in -O, we say that if you stay in the “safe subset” of Swift code, you never get undefined behavior, even if there’s a bug in your code. You might get *unpredictable* behavior of course, but presumably guaranteeing no undefined behavior rules out large classes of problems, including many security holes. Now suppose you decide to be responsible and add some asserts to help you catch bugs during development. Hopefully they help you catch all the bugs, but what if they don’t? All of a sudden, if you still have a bug when you ship, you now have undefined behavior. As much as I’m a fan of assertions having optimization benefits, It seems a little perverse that using them could make shipping code less secure.

-Dave

···

On Dec 31, 2015, at 12:27 PM, Chris Lattner via swift-evolution <swift-evolution@swift.org> wrote:

On Dec 28, 2015, at 5:48 AM, Joseph Lord via swift-evolution <swift-evolution@swift.org> wrote:

The documented behaviour of assert and assertionFailure in "disable safety checks" builds (still documented as -Ounchecked) is that the compiler "may assume that it would evaluate to true" or in the assertionFailure case never be called.

Right.

This documented behaviour would allow the compiler to completely eliminate tests and branches before or after the assertion and take the operation deep into undefined behaviour.

Only in cases where the assertion would have failed, right? The point of -Ounchecked is that - if your code was correct with the checks - that it will still be correct. Disabling overflow and array bounds checks is far more dangerous than the assertion behavior you cite here.

It appears from the code as if the assumption is not currently applied on the assert method although it is on the assertionFailure case by means of the _conditionallyUnreachable() call. assert seems to be a no-op in both normal release and disable safety checks build modes.

Right.

[Proposed Change]

Change the documentation for assert and assertionFailure so that behaviour in unchecked mode is the same as in normal release - no evaluation and no effect.

Why? :

1) Expected behaviour based on other languages is for assert to have no effect in release. (If current behaviour is highly desired another function name should be used). Having potential dangerous behaviour from a function that is familiar across languages and is regarded as a safety feature is undesirable.

This is the C model, but as you know, there is a whole field of custom assertions libraries that people have developed. I don’t think there is anything like consensus on this topic.

2) Adding asserts to code should not make the code more dangerous whatever the build. Assuming the truth of the assert may lead to runtime safety checks being skipped and undefined behaviour when a no-op would be a safe behaviour.

This only affects code built with -Ounchecked, which is definitely not a safe mode to build your code. The intention of this mode is that you can use it to get a performance boost, if you believe your code to be sufficiently tested. This mode, which isn’t the default in any way, intentionally takes the guard rails off to get better performance.

If you don’t like that model, don’t use this mode.


(Joseph Lord) #4

I gave a quick reply to an email later in the chain last night but I think these points are worth addressing. Apologies for the slow response, I wanted to ponder and consider the points rather than rush the response.

The documented behaviour of assert and assertionFailure in "disable safety checks" builds (still documented as -Ounchecked) is that the compiler "may assume that it would evaluate to true" or in the assertionFailure case never be called.

Right.

This documented behaviour would allow the compiler to completely eliminate tests and branches before or after the assertion and take the operation deep into undefined behaviour.

Only in cases where the assertion would have failed, right? The point of -Ounchecked is that - if your code was correct with the checks - that it will still be correct. Disabling overflow and array bounds checks is far more dangerous than the assertion behavior you cite here.

Yes only when it would have failed but that may still make correct code incorrect where whole branches of runtime belt and braces code could be a eliminated.

It appears from the code as if the assumption is not currently applied on the assert method although it is on the assertionFailure case by means of the _conditionallyUnreachable() call. assert seems to be a no-op in both normal release and disable safety checks build modes.

Right.

[Proposed Change]

Change the documentation for assert and assertionFailure so that behaviour in unchecked mode is the same as in normal release - no evaluation and no effect.

Why? :

1) Expected behaviour based on other languages is for assert to have no effect in release. (If current behaviour is highly desired another function name should be used). Having potential dangerous behaviour from a function that is familiar across languages and is regarded as a safety feature is undesirable.

This is the C model, but as you know, there is a whole field of custom assertions libraries that people have developed. I don’t think there is anything like consensus on this topic.

C, Python, Erlang, Ocaml, Java (although it can enabled at runtime) and probably more. I recognise that in house assertion policies and systems may have different behaviours and agreed modes but I think affecting surrounding code (before and after the assertion) is surprising based on people experience with these other languages. I have not seen elsewhere any instances of compilers removing code outside of the branch itself which could be possible if the compiler is allowed to assume the truth of the assertion condition.

if let names = jsonObject as? [String] where names.count > 0 {
  print(names.first)
} else {
  assertionFailure("Invalid JSON")
  return nil
}

Could be compiled to pretty much an unconditional print of an address in unchecked. The nil return could be unreachable, the if let reduced to an unsafe bitcast (maybe not given array bridging but possibly in other cases) and the .first converted to [0] given the assumed passing of the where clause.

2) Adding asserts to code should not make the code more dangerous whatever the build. Assuming the truth of the assert may lead to runtime safety checks being skipped and undefined behaviour when a no-op would be a safe behaviour.

This only affects code built with -Ounchecked, which is definitely not a safe mode to build your code. The intention of this mode is that you can use it to get a performance boost, if you believe your code to be sufficiently tested. This mode, which isn’t the default in any way, intentionally takes the guard rails off to get better performance.

If you don’t like that model, don’t use this mode

One fear is that libraries will be compiled in this way by people who aren't the original authors and that people won't realise the consequences.

3) "For highly robust code assert and then handle the error anyway" [Code Complete 2nd Edition section 8.2]

Highly robust code shouldn’t build with -Ounchecked, so I don’t see how this point is pertinent.

The quote was external validation of the concept of having asserts where the case is also handled. I would not limit the use to "highly robust code".

However as I said in my other email if no one else is concerned I will just go away and use my custom assertions and miss out on the compiler hinting.

Joseph

···

On Dec 31, 2015, at 8:27 PM, Chris Lattner <clattner@apple.com> wrote:
On Dec 28, 2015, at 5:48 AM, Joseph Lord via swift-evolution <swift-evolution@swift.org> wrote:


(Chris Lattner) #5

2) Adding asserts to code should not make the code more dangerous whatever the build. Assuming the truth of the assert may lead to runtime safety checks being skipped and undefined behaviour when a no-op would be a safe behaviour.

This only affects code built with -Ounchecked, which is definitely not a safe mode to build your code. The intention of this mode is that you can use it to get a performance boost, if you believe your code to be sufficiently tested. This mode, which isn’t the default in any way, intentionally takes the guard rails off to get better performance.

If you don’t like that model, don’t use this mode.

Let’s just consider -O; I think I understand Joseph’s objection here, and it seems valid.

Ah, good point.

Normally in -O, we say that if you stay in the “safe subset” of Swift code, you never get undefined behavior, even if there’s a bug in your code. You might get *unpredictable* behavior of course, but presumably guaranteeing no undefined behavior rules out large classes of problems, including many security holes. Now suppose you decide to be responsible and add some asserts to help you catch bugs during development. Hopefully they help you catch all the bugs, but what if they don’t? All of a sudden, if you still have a bug when you ship, you now have undefined behavior. As much as I’m a fan of assertions having optimization benefits, It seems a little perverse that using them could make shipping code less secure.

Yes, I agree. -O should not imply undefined behavior in the case of an assert() predicate being dynamically false.

It sounds like we just need a documentation update here?

-Chris

···

On Dec 31, 2015, at 1:56 PM, Dave Abrahams <dabrahams@apple.com> wrote:


(Dave Abrahams) #6

Wait a sec; I just read the doc comments for assert over again. They don’t say there’s undefined behavior in -O if the condition isn’t satisfied. So now I don’t understand what Joseph is complaining about. assert in -O is documented to act exactly as C’s assert would with NDEBUG #defined.

-Dave

···

On Dec 31, 2015, at 1:56 PM, Dave Abrahams via swift-evolution <swift-evolution@swift.org> wrote:

On Dec 31, 2015, at 12:27 PM, Chris Lattner via swift-evolution <swift-evolution@swift.org> wrote:

On Dec 28, 2015, at 5:48 AM, Joseph Lord via swift-evolution <swift-evolution@swift.org> wrote:

The documented behaviour of assert and assertionFailure in "disable safety checks" builds (still documented as -Ounchecked) is that the compiler "may assume that it would evaluate to true" or in the assertionFailure case never be called.

Right.

This documented behaviour would allow the compiler to completely eliminate tests and branches before or after the assertion and take the operation deep into undefined behaviour.

Only in cases where the assertion would have failed, right? The point of -Ounchecked is that - if your code was correct with the checks - that it will still be correct. Disabling overflow and array bounds checks is far more dangerous than the assertion behavior you cite here.

It appears from the code as if the assumption is not currently applied on the assert method although it is on the assertionFailure case by means of the _conditionallyUnreachable() call. assert seems to be a no-op in both normal release and disable safety checks build modes.

Right.

[Proposed Change]

Change the documentation for assert and assertionFailure so that behaviour in unchecked mode is the same as in normal release - no evaluation and no effect.

Why? :

1) Expected behaviour based on other languages is for assert to have no effect in release. (If current behaviour is highly desired another function name should be used). Having potential dangerous behaviour from a function that is familiar across languages and is regarded as a safety feature is undesirable.

This is the C model, but as you know, there is a whole field of custom assertions libraries that people have developed. I don’t think there is anything like consensus on this topic.

2) Adding asserts to code should not make the code more dangerous whatever the build. Assuming the truth of the assert may lead to runtime safety checks being skipped and undefined behaviour when a no-op would be a safe behaviour.

This only affects code built with -Ounchecked, which is definitely not a safe mode to build your code. The intention of this mode is that you can use it to get a performance boost, if you believe your code to be sufficiently tested. This mode, which isn’t the default in any way, intentionally takes the guard rails off to get better performance.

If you don’t like that model, don’t use this mode.

Let’s just consider -O; I think I understand Joseph’s objection here, and it seems valid.

Normally in -O, we say that if you stay in the “safe subset” of Swift code, you never get undefined behavior, even if there’s a bug in your code. You might get *unpredictable* behavior of course, but presumably guaranteeing no undefined behavior rules out large classes of problems, including many security holes. Now suppose you decide to be responsible and add some asserts to help you catch bugs during development. Hopefully they help you catch all the bugs, but what if they don’t? All of a sudden, if you still have a bug when you ship, you now have undefined behavior. As much as I’m a fan of assertions having optimization benefits, It seems a little perverse that using them could make shipping code less secure.


(Dave Abrahams) #7

I’m pretty sure the documentation reflects assumptions that the optimizer is already taking advantage of, but the performance team knows for sure.

-Dave

···

On Jan 1, 2016, at 11:25 PM, Chris Lattner <clattner@apple.com> wrote:

On Dec 31, 2015, at 1:56 PM, Dave Abrahams <dabrahams@apple.com> wrote:

2) Adding asserts to code should not make the code more dangerous whatever the build. Assuming the truth of the assert may lead to runtime safety checks being skipped and undefined behaviour when a no-op would be a safe behaviour.

This only affects code built with -Ounchecked, which is definitely not a safe mode to build your code. The intention of this mode is that you can use it to get a performance boost, if you believe your code to be sufficiently tested. This mode, which isn’t the default in any way, intentionally takes the guard rails off to get better performance.

If you don’t like that model, don’t use this mode.

Let’s just consider -O; I think I understand Joseph’s objection here, and it seems valid.

Ah, good point.

Normally in -O, we say that if you stay in the “safe subset” of Swift code, you never get undefined behavior, even if there’s a bug in your code. You might get *unpredictable* behavior of course, but presumably guaranteeing no undefined behavior rules out large classes of problems, including many security holes. Now suppose you decide to be responsible and add some asserts to help you catch bugs during development. Hopefully they help you catch all the bugs, but what if they don’t? All of a sudden, if you still have a bug when you ship, you now have undefined behavior. As much as I’m a fan of assertions having optimization benefits, It seems a little perverse that using them could make shipping code less secure.

Yes, I agree. -O should not imply undefined behavior in the case of an assert() predicate being dynamically false.

It sounds like we just need a documentation update here?


(Lily Ballard) #8

assert() and assertionFailure() are already explicitly documented as doing nothing in -O builds, and only making the assumption that the condition is always true/function is never called in -Ounchecked builds.

As far as I can tell, the only actual inaccuracy in the documentation is the fact that assert() claims that the optimizer may assume the condition is always true in -Ounchecked when it doesn't seem to actually do that (because it doesn't do anything at all when not using -Onone). Both assert() and assertionFailure() do nothing in -O, which matches the documentation.

-Kevin Ballard

···

On Fri, Jan 1, 2016, at 11:25 PM, Chris Lattner via swift-evolution wrote:

> On Dec 31, 2015, at 1:56 PM, Dave Abrahams <dabrahams@apple.com> wrote:
>>> 2) Adding asserts to code should not make the code more dangerous whatever the build. Assuming the truth of the assert may lead to runtime safety checks being skipped and undefined behaviour when a no-op would be a safe behaviour.
>>
>> This only affects code built with -Ounchecked, which is definitely not a safe mode to build your code. The intention of this mode is that you can use it to get a performance boost, if you believe your code to be sufficiently tested. This mode, which isn’t the default in any way, intentionally takes the guard rails off to get better performance.
>>
>> If you don’t like that model, don’t use this mode.
>
> Let’s just consider -O; I think I understand Joseph’s objection here, and it seems valid.

Ah, good point.

> Normally in -O, we say that if you stay in the “safe subset” of Swift code, you never get undefined behavior, even if there’s a bug in your code. You might get *unpredictable* behavior of course, but presumably guaranteeing no undefined behavior rules out large classes of problems, including many security holes. Now suppose you decide to be responsible and add some asserts to help you catch bugs during development. Hopefully they help you catch all the bugs, but what if they don’t? All of a sudden, if you still have a bug when you ship, you now have undefined behavior. As much as I’m a fan of assertions having optimization benefits, It seems a little perverse that using them could make shipping code less secure.

Yes, I agree. -O should not imply undefined behavior in the case of an assert() predicate being dynamically false.

It sounds like we just need a documentation update here?


(Joseph Lord) #9

Thanks for the responses.

The behaviour and documentation at -O is fine. My worry is only for unchecked builds.

My concern is only for the unchecked behaviour. I still think it is surprising that adding an assert can substantively change the behaviour (undefined if false condition occurs). If assert was a brand new idea that did not exist in other languages It would be absolutely appropriate behaviour for the word as it is now but with the history of assert in other languages I still have two problems based on my understanding of asserts across languages.

1) Adding asserts should never be able to add bugs (which they can do in unchecked code).
2) The stdlib assert cannot be used to handle any errors that may potentially occur and are handled but that you want to be informed of during development if there is any possibility that the code will ever be compiled in unchecked mode.

The quote I put in the original email about having asserts and handling the error was to show that it is not a practice unique to myself.

An example of where I use asserts is when parsing JSON I will always conditionally handle all potential results of invalid content (missing elements or wrong datatypes) and will usually just ignore the item and fail gracefully but will put asserts so that in development I know that either the data is malformed or I have a bug and am not handling all the reasonable inputs. If this code was compiled in unchecked mode with stdlib asserts it is likely to expose crashes and potentially be exploitable.

Having said all of that there hasn't been much interest on the list so maybe my feeling about assert isn't ideally shared.

Joseph

···

On Jan 2, 2016, at 5:39 PM, Dave Abrahams via swift-evolution <swift-evolution@swift.org> wrote:

On Dec 31, 2015, at 1:56 PM, Dave Abrahams via swift-evolution <swift-evolution@swift.org> wrote:

On Dec 31, 2015, at 12:27 PM, Chris Lattner via swift-evolution <swift-evolution@swift.org> wrote:

On Dec 28, 2015, at 5:48 AM, Joseph Lord via swift-evolution <swift-evolution@swift.org> wrote:

The documented behaviour of assert and assertionFailure in "disable safety checks" builds (still documented as -Ounchecked) is that the compiler "may assume that it would evaluate to true" or in the assertionFailure case never be called.

Right.

This documented behaviour would allow the compiler to completely eliminate tests and branches before or after the assertion and take the operation deep into undefined behaviour.

Only in cases where the assertion would have failed, right? The point of -Ounchecked is that - if your code was correct with the checks - that it will still be correct. Disabling overflow and array bounds checks is far more dangerous than the assertion behavior you cite here.

It appears from the code as if the assumption is not currently applied on the assert method although it is on the assertionFailure case by means of the _conditionallyUnreachable() call. assert seems to be a no-op in both normal release and disable safety checks build modes.

Right.

[Proposed Change]

Change the documentation for assert and assertionFailure so that behaviour in unchecked mode is the same as in normal release - no evaluation and no effect.

Why? :

1) Expected behaviour based on other languages is for assert to have no effect in release. (If current behaviour is highly desired another function name should be used). Having potential dangerous behaviour from a function that is familiar across languages and is regarded as a safety feature is undesirable.

This is the C model, but as you know, there is a whole field of custom assertions libraries that people have developed. I don’t think there is anything like consensus on this topic.

2) Adding asserts to code should not make the code more dangerous whatever the build. Assuming the truth of the assert may lead to runtime safety checks being skipped and undefined behaviour when a no-op would be a safe behaviour.

This only affects code built with -Ounchecked, which is definitely not a safe mode to build your code. The intention of this mode is that you can use it to get a performance boost, if you believe your code to be sufficiently tested. This mode, which isn’t the default in any way, intentionally takes the guard rails off to get better performance.

If you don’t like that model, don’t use this mode.

Let’s just consider -O; I think I understand Joseph’s objection here, and it seems valid.

Normally in -O, we say that if you stay in the “safe subset” of Swift code, you never get undefined behavior, even if there’s a bug in your code. You might get *unpredictable* behavior of course, but presumably guaranteeing no undefined behavior rules out large classes of problems, including many security holes. Now suppose you decide to be responsible and add some asserts to help you catch bugs during development. Hopefully they help you catch all the bugs, but what if they don’t? All of a sudden, if you still have a bug when you ship, you now have undefined behavior. As much as I’m a fan of assertions having optimization benefits, It seems a little perverse that using them could make shipping code less secure.

Wait a sec; I just read the doc comments for assert over again. They don’t say there’s undefined behavior in -O if the condition isn’t satisfied. So now I don’t understand what Joseph is complaining about. assert in -O is documented to act exactly as C’s assert would with NDEBUG #defined.

-Dave


(Lily Ballard) #10

Having said that, it occurs to me that maybe the optimizer is actually specially detecting the call to assert() under -Ounchecked, but from the source of assert() there's nothing there to indicate that it actually makes any assumptions under -Ounchecked.

-Kevin Ballard

···

On Fri, Jan 1, 2016, at 11:58 PM, Kevin Ballard wrote:

On Fri, Jan 1, 2016, at 11:25 PM, Chris Lattner via swift-evolution wrote:
> > On Dec 31, 2015, at 1:56 PM, Dave Abrahams <dabrahams@apple.com> wrote:
> >>> 2) Adding asserts to code should not make the code more dangerous whatever the build. Assuming the truth of the assert may lead to runtime safety checks being skipped and undefined behaviour when a no-op would be a safe behaviour.
> >>
> >> This only affects code built with -Ounchecked, which is definitely not a safe mode to build your code. The intention of this mode is that you can use it to get a performance boost, if you believe your code to be sufficiently tested. This mode, which isn’t the default in any way, intentionally takes the guard rails off to get better performance.
> >>
> >> If you don’t like that model, don’t use this mode.
> >
> > Let’s just consider -O; I think I understand Joseph’s objection here, and it seems valid.
>
> Ah, good point.
>
> > Normally in -O, we say that if you stay in the “safe subset” of Swift code, you never get undefined behavior, even if there’s a bug in your code. You might get *unpredictable* behavior of course, but presumably guaranteeing no undefined behavior rules out large classes of problems, including many security holes. Now suppose you decide to be responsible and add some asserts to help you catch bugs during development. Hopefully they help you catch all the bugs, but what if they don’t? All of a sudden, if you still have a bug when you ship, you now have undefined behavior. As much as I’m a fan of assertions having optimization benefits, It seems a little perverse that using them could make shipping code less secure.
>
> Yes, I agree. -O should not imply undefined behavior in the case of an assert() predicate being dynamically false.
>
> It sounds like we just need a documentation update here?

assert() and assertionFailure() are already explicitly documented as doing nothing in -O builds, and only making the assumption that the condition is always true/function is never called in -Ounchecked builds.

As far as I can tell, the only actual inaccuracy in the documentation is the fact that assert() claims that the optimizer may assume the condition is always true in -Ounchecked when it doesn't seem to actually do that (because it doesn't do anything at all when not using -Onone). Both assert() and assertionFailure() do nothing in -O, which matches the documentation.


(Arnold Schwaighofer) #11

'assert' evaluates the condition and aborts only in Odebug builds.

'precondition' evaluates the condition and aborts also in optimized -0 builds.

As far as I remember the decision was made to give it this semantics to mimic C's assert() function.

If an abort is desired in optimized builds one should use 'precondition'.

···

Sent from my iPhone

On Jan 2, 2016, at 8:27 AM, Dave Abrahams <dabrahams@apple.com> wrote:

On Jan 1, 2016, at 11:25 PM, Chris Lattner <clattner@apple.com> wrote:

On Dec 31, 2015, at 1:56 PM, Dave Abrahams <dabrahams@apple.com> wrote:

2) Adding asserts to code should not make the code more dangerous whatever the build. Assuming the truth of the assert may lead to runtime safety checks being skipped and undefined behaviour when a no-op would be a safe behaviour.

This only affects code built with -Ounchecked, which is definitely not a safe mode to build your code. The intention of this mode is that you can use it to get a performance boost, if you believe your code to be sufficiently tested. This mode, which isn’t the default in any way, intentionally takes the guard rails off to get better performance.

If you don’t like that model, don’t use this mode.

Let’s just consider -O; I think I understand Joseph’s objection here, and it seems valid.

Ah, good point.

Normally in -O, we say that if you stay in the “safe subset” of Swift code, you never get undefined behavior, even if there’s a bug in your code. You might get *unpredictable* behavior of course, but presumably guaranteeing no undefined behavior rules out large classes of problems, including many security holes. Now suppose you decide to be responsible and add some asserts to help you catch bugs during development. Hopefully they help you catch all the bugs, but what if they don’t? All of a sudden, if you still have a bug when you ship, you now have undefined behavior. As much as I’m a fan of assertions having optimization benefits, It seems a little perverse that using them could make shipping code less secure.

Yes, I agree. -O should not imply undefined behavior in the case of an assert() predicate being dynamically false.

It sounds like we just need a documentation update here?

I’m pretty sure the documentation reflects assumptions that the optimizer is already taking advantage of, but the performance team knows for sure.

-Dave


(Dave Abrahams) #12

'assert' evaluates the condition and aborts only in Odebug builds.

'precondition' evaluates the condition and aborts also in optimized -0 builds.

As far as I remember the decision was made to give it this semantics to mimic C's assert() function.

If an abort is desired in optimized builds one should use 'precondition’.

Thanks, Arnold, but this doesn’t address the key question: in -O builds, does the optimizer make optimizations based on the assumption that asserts would not fire if they were enabled?

Sent from my iPhone

2) Adding asserts to code should not make the code more dangerous whatever the build. Assuming the truth of the assert may lead to runtime safety checks being skipped and undefined behaviour when a no-op would be a safe behaviour.

This only affects code built with -Ounchecked, which is definitely not a safe mode to build your code. The intention of this mode is that you can use it to get a performance boost, if you believe your code to be sufficiently tested. This mode, which isn’t the default in any way, intentionally takes the guard rails off to get better performance.

If you don’t like that model, don’t use this mode.

Let’s just consider -O; I think I understand Joseph’s objection here, and it seems valid.

Ah, good point.

Normally in -O, we say that if you stay in the “safe subset” of Swift code, you never get undefined behavior, even if there’s a bug in your code. You might get *unpredictable* behavior of course, but presumably guaranteeing no undefined behavior rules out large classes of problems, including many security holes. Now suppose you decide to be responsible and add some asserts to help you catch bugs during development. Hopefully they help you catch all the bugs, but what if they don’t? All of a sudden, if you still have a bug when you ship, you now have undefined behavior. As much as I’m a fan of assertions having optimization benefits, It seems a little perverse that using them could make shipping code less secure.

Yes, I agree. -O should not imply undefined behavior in the case of an assert() predicate being dynamically false.

It sounds like we just need a documentation update here?

I’m pretty sure the documentation reflects assumptions that the optimizer is already taking advantage of, but the performance team knows for sure.

-Dave

-Dave

···

On Jan 2, 2016, at 2:57 AM, Arnold <aschwaighofer@apple.com> wrote:

On Jan 2, 2016, at 8:27 AM, Dave Abrahams <dabrahams@apple.com> wrote:

On Jan 1, 2016, at 11:25 PM, Chris Lattner <clattner@apple.com> wrote:
On Dec 31, 2015, at 1:56 PM, Dave Abrahams <dabrahams@apple.com> wrote:


(Arnold Schwaighofer) #13

'assert' evaluates the condition and aborts only in Odebug builds.

'precondition' evaluates the condition and aborts also in optimized -0 builds.

As far as I remember the decision was made to give it this semantics to mimic C's assert() function.

If an abort is desired in optimized builds one should use 'precondition’.

Thanks, Arnold, but this doesn’t address the key question: in -O builds, does the optimizer make optimizations based on the assumption that asserts would not fire if they were enabled?

I think you answered this question already in your follow up email: no the optimizer does not make optimization based on this assumption.

···

Sent from my iPhone

On Jan 2, 2016, at 6:35 PM, Dave Abrahams <dabrahams@apple.com> wrote:

On Jan 2, 2016, at 2:57 AM, Arnold <aschwaighofer@apple.com> wrote:

Sent from my iPhone

On Jan 2, 2016, at 8:27 AM, Dave Abrahams <dabrahams@apple.com> wrote:

On Jan 1, 2016, at 11:25 PM, Chris Lattner <clattner@apple.com> wrote:

On Dec 31, 2015, at 1:56 PM, Dave Abrahams <dabrahams@apple.com> wrote:

2) Adding asserts to code should not make the code more dangerous whatever the build. Assuming the truth of the assert may lead to runtime safety checks being skipped and undefined behaviour when a no-op would be a safe behaviour.

This only affects code built with -Ounchecked, which is definitely not a safe mode to build your code. The intention of this mode is that you can use it to get a performance boost, if you believe your code to be sufficiently tested. This mode, which isn’t the default in any way, intentionally takes the guard rails off to get better performance.

If you don’t like that model, don’t use this mode.

Let’s just consider -O; I think I understand Joseph’s objection here, and it seems valid.

Ah, good point.

Normally in -O, we say that if you stay in the “safe subset” of Swift code, you never get undefined behavior, even if there’s a bug in your code. You might get *unpredictable* behavior of course, but presumably guaranteeing no undefined behavior rules out large classes of problems, including many security holes. Now suppose you decide to be responsible and add some asserts to help you catch bugs during development. Hopefully they help you catch all the bugs, but what if they don’t? All of a sudden, if you still have a bug when you ship, you now have undefined behavior. As much as I’m a fan of assertions having optimization benefits, It seems a little perverse that using them could make shipping code less secure.

Yes, I agree. -O should not imply undefined behavior in the case of an assert() predicate being dynamically false.

It sounds like we just need a documentation update here?

I’m pretty sure the documentation reflects assumptions that the optimizer is already taking advantage of, but the performance team knows for sure.

-Dave

-Dave