www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - assert(0) behavior

reply Steven Schveighoffer <schveiguy yahoo.com> writes:
If you compile and run the following code, what happens?

void main()
{
    assert(0, "error message");
}

answer: it depends. On OSX, if you compile this like so:

dmd testassert.d
./testassert

You get this message + stack trace:

core.exception.AssertError testassert.d(3): error message

Not bad. But assert(0) is special in that it is always enabled, even in 
release mode. So let's try that:

dmd -release testassert.d
./testassert
Segmentation fault: 11

WAT. The explanation is, assert(0) is translated in release mode to a 
HLT instruction. on X86, this results in a segfault. But a seg fault is 
tremendously less useful. Let's say you are running a 10k line program, 
and you see this. Compared with seeing the assert message and stack 
trace, this is going to cause hours of extra debugging.

Why do we do this? I'm really not sure. Note that "error message" is 
essentially not used if we have a seg fault. Throwing an assert error 
shouldn't cause any issues with stack unwinding performance, since this 
can be done inside a nothrow function. And when you throw an 
AssertError, it shouldn't be caught anyway.

Why can't assert(0) throw an assert error in release mode instead of 
segfaulting? What would it cost to do this instead?

-Steve
Aug 03 2015
next sibling parent reply "Dicebot" <public dicebot.lv> writes:
On Monday, 3 August 2015 at 14:34:52 UTC, Steven Schveighoffer 
wrote:

 Why do we do this?


Because all asserts must be completely removed in -release

Yet assert(0) effectively mean "unreachable code" (it is actually 
defined that way in spec) and thus it is possible to ensure extra 
"free" bit of safety by crashing the app.
Aug 03 2015
next sibling parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 8/3/15 11:18 AM, Dicebot wrote:

 On Monday, 3 August 2015 at 14:34:52 UTC, Steven Schveighoffer wrote:

 Why do we do this?


 Because all asserts must be completely removed in -release


1. They aren't removed, they are replaced with a nearly useless segfault.
2. If we are going to put something in there instead of "assert", why 
not just throw an error?

Effectively:

assert(0, msg)

becomes a fancy way of writing (in any mode, release or otherwise):

throw new AssertError(msg);

This is actually the way I thought it was done.

-Steve
Aug 03 2015
next sibling parent reply "Dicebot" <public dicebot.lv> writes:
On Monday, 3 August 2015 at 15:50:56 UTC, Steven Schveighoffer 
wrote:

 On 8/3/15 11:18 AM, Dicebot wrote:

 On Monday, 3 August 2015 at 14:34:52 UTC, Steven Schveighoffer 
 wrote:

 Why do we do this?


 Because all asserts must be completely removed in -release


 1. They aren't removed, they are replaced with a nearly useless 
 segfault.
 2. If we are going to put something in there instead of 
 "assert", why not just throw an error?

 Effectively:

 assert(0, msg)

 becomes a fancy way of writing (in any mode, release or 
 otherwise):

 throw new AssertError(msg);

 This is actually the way I thought it was done.

 -Steve


Now, they are completely removed. There is effectively no 
AssertError present in -release (it is defined but compiler is 
free to assume it never happens). I'd expect any reasonable 
compiler to not even emit stack unwinding code for functions with 
assert(0) (and no other throwables are present).

assert(0) is effectively same as gcc __builtin_unreachable with 
all consequences for optimization - with only difference that 
latter won't even insert HLT but just continue executing 
corrupted program.
Aug 03 2015
parent Steven Schveighoffer <schveiguy yahoo.com> writes:
On 8/3/15 11:57 AM, Dicebot wrote:

 On Monday, 3 August 2015 at 15:50:56 UTC, Steven Schveighoffer wrote:

 On 8/3/15 11:18 AM, Dicebot wrote:

 On Monday, 3 August 2015 at 14:34:52 UTC, Steven Schveighoffer wrote:

 Why do we do this?


 Because all asserts must be completely removed in -release


 1. They aren't removed, they are replaced with a nearly useless segfault.
 2. If we are going to put something in there instead of "assert", why
 not just throw an error?

 Effectively:

 assert(0, msg)

 becomes a fancy way of writing (in any mode, release or otherwise):

 throw new AssertError(msg);

 This is actually the way I thought it was done.


 Now, they are completely removed. There is effectively no AssertError
 present in -release (it is defined but compiler is free to assume it
 never happens). I'd expect any reasonable compiler to not even emit
 stack unwinding code for functions with assert(0) (and no other
 throwables are present).


I actually don't care if stack is unwound (it's not guaranteed by 
language anyway). Just segfault is not useful at all to anyone who 
doesn't have core dump enabled, and even if they do enable it, it's not 
trivial to get at the real cause of the error. I'd settle for calling:

__onAssertZero(__FILE__, __LINE__, message);

Which can do whatever we want, print a message, do HLT, throw 
AssertError, etc.


 assert(0) is effectively same as gcc __builtin_unreachable with all
 consequences for optimization - with only difference that latter won't
 even insert HLT but just continue executing corrupted program.


No reason this would change. Compiler can still consider this 
unreachable code for optimization purposes.

-Steve
Aug 03 2015
prev sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 8/3/2015 8:50 AM, Steven Schveighoffer wrote:

 1. They aren't removed, they are replaced with a nearly useless segfault.


Not useless at all:

1. The program does not continue running after it has failed. (Please, let's
not 
restart that debate.
2. Running it under a debugger, the location of the fault will be identified.



 2. If we are going to put something in there instead of "assert", why not just
 throw an error?


Because they are expensive.

To keep asserts in all their glory in released code, do not use the -release
switch.
Aug 03 2015
next sibling parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 8/3/15 6:54 PM, Walter Bright wrote:

 On 8/3/2015 8:50 AM, Steven Schveighoffer wrote:

 1. They aren't removed, they are replaced with a nearly useless segfault.


 Not useless at all:

 1. The program does not continue running after it has failed. (Please,
 let's not restart that debate.


You can run some code that is fresh, i.e. don't *continue* running 
failing code, but it's ok, for example, to run a signal handler, or call 
a pure function. My thought is that you should print the file location 
and exit (perhaps with segfault).


 2. Running it under a debugger, the location of the fault will be
 identified.


Yeah, the real problem is when you are not running under a debugger. If 
you have an intermittent bug, or one that is in the field, a stack trace 
of the problem is worth 100x more than a segfault and trying to talk a 
customer through setting up a debugger, or even setting up their system 
to do a core dump for you to debug. They aren't always interested in 
that enough to care.

But also, a segfault has a specific meaning. It means, you tried to 
access memory you don't have access to. This clearly is not that, and it 
sends the wrong message -- memory corruption. This is not memory 
corruption, it's a program error.


 2. If we are going to put something in there instead of "assert", why
 not just
 throw an error?


 Because they are expensive.


They are expensive when? 2 microseconds before the program ends? At that 
point, I'd rather you spend as much resources telling me what went wrong 
as possible. More info the better.

Now, if it's expensive to include the throw vs. not including it (I 
don't know), then that's a fair point. If it means an extra few 
instructions to set up the throw (which isn't run at all when the 
assert(0) line isn't run), that's not convincing.


 To keep asserts in all their glory in released code, do not use the
 -release switch.


OK, this brings up another debate. The thing that triggered all this is 
an issue with core.time, see issue 
https://issues.dlang.org/show_bug.cgi?id=14863

Essentially, we wrote code to get all the clock information at startup 
on a posix system that supports clock_gettime, which is immutable and 
can be read easily at startup. However, how we handled the case where a 
clock could not be fetched is to assert(0, "don't support clock " ~ 
clockname).

The clear expectation was that the message will be printed (along with 
file/line number), and we can go fix the issue.

On Linux 2.6.32 - a supported LTS release I guess - one of these clocks 
is not supported. This means running a simple "hello world" program 
crashes at startup in a segfault, not a nice informative message.

So what is the right answer here? We want an assert to trigger for this, 
but the only assert that stays in is assert(0). However, that's useless 
if all someone sees is "segfuault". "some message" never gets printed, 
because druntime is compiled in release mode. I'm actually quite 
thrilled that someone figured this all out -- one person analyzed his 
core dump to narrow down the function, and happened to include his linux 
kernel version, and another person had the arcane knowledge that some 
clock wasn't supported in that version.

Is there a better mechanism we should be using in druntime that is 
clearly going to be compiled in release mode? Should we just throw an 
assert error directly? Clearly the code is not "corrupted" at this 
point, it's just an environmental issue. But we don't want to continue 
execution. What is the right call?

At the very least, assert(0, "message") should be a compiler error, the 
message is unused information.

-Steve
Aug 03 2015
next sibling parent reply =?UTF-8?B?QWxpIMOHZWhyZWxp?= <acehreli yahoo.com> writes:
On 08/03/2015 04:57 PM, Steven Schveighoffer wrote:


 At the very least, assert(0, "message") should be a compiler error, the
 message is unused information.


Agreed.

How about dumping the message to stderr as a best effort if the message 
is a literal? Hm... On the other hand, undefined behavior means that 
even trying that can cause harm like radiating a human with too much 
medical radiation. :(

Perhaps, if it is a complicated expression like calling format(), then 
it should be an error?

Ali
Aug 03 2015
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 8/3/2015 5:25 PM, Ali Çehreli wrote:

 On 08/03/2015 04:57 PM, Steven Schveighoffer wrote:

  > At the very least, assert(0, "message") should be a compiler error, the
  > message is unused information.

 Agreed.


No.

1. If you want the message, do not use -release.
2. Do not use asserts to issue error messages for input/environment errors.
Aug 03 2015
parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 8/3/15 9:13 PM, Walter Bright wrote:

 On 8/3/2015 5:25 PM, Ali Çehreli wrote:

 On 08/03/2015 04:57 PM, Steven Schveighoffer wrote:

  > At the very least, assert(0, "message") should be a compiler error,
 the
  > message is unused information.

 Agreed.


 No.

 1. If you want the message, do not use -release.
 2. Do not use asserts to issue error messages for input/environment errors.


Any assert(0) in druntime or phobos with a message printed is an error. 
It's compiled in release mode.

I'll look through and find them, and rework them.

-Steve
Aug 04 2015
next sibling parent Steven Schveighoffer <schveiguy yahoo.com> writes:
On 8/4/15 7:53 AM, Steven Schveighoffer wrote:


 Any assert(0) in druntime or phobos with a message printed is an error.
 It's compiled in release mode.

 I'll look through and find them, and rework them.


First stab: https://github.com/D-Programming-Language/druntime/pull/1337

-Steve
Aug 04 2015
prev sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 8/4/2015 4:53 AM, Steven Schveighoffer wrote:

 Any assert(0) in druntime or phobos with a message printed is an error. It's
 compiled in release mode.


When you're debugging phobos, you aren't compiling in release mode.

But I've also been of the opinion, not widely shared, that any programmer 
supplied messages in asserts are pointless.
Aug 04 2015
next sibling parent reply "deadalnix" <deadalnix gmail.com> writes:
On Tuesday, 4 August 2015 at 20:23:53 UTC, Walter Bright wrote:

 On 8/4/2015 4:53 AM, Steven Schveighoffer wrote:

 Any assert(0) in druntime or phobos with a message printed is 
 an error. It's
 compiled in release mode.


 When you're debugging phobos, you aren't compiling in release 
 mode.

 But I've also been of the opinion, not widely shared, that any 
 programmer supplied messages in asserts are pointless.


Would you be of the opinion that assert should be a statement 
rather than an expression ? unreadability in the middle of 
expressions, especially when evaluation order is not well 
defined, is close to impossible to get right.
Aug 04 2015
parent reply "deadalnix" <deadalnix gmail.com> writes:
On Tuesday, 4 August 2015 at 20:40:47 UTC, deadalnix wrote:

 Would you be of the opinion that assert should be a statement 
 rather than an expression ? unreadability in the middle of 
 expressions, especially when evaluation order is not well 
 defined, is close to impossible to get right.


Ping ? Walter ?
Aug 04 2015
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 8/4/2015 8:26 PM, deadalnix wrote:

 On Tuesday, 4 August 2015 at 20:40:47 UTC, deadalnix wrote:

 Would you be of the opinion that assert should be a statement rather than an
 expression ? unreadability in the middle of expressions, especially when
 evaluation order is not well defined, is close to impossible to get right.


 Ping ? Walter ?


If you want it as its own statement, write it that way. I don't see a need to 
change the language, nor would the breakage it would cause be justified.
Aug 04 2015
parent "Marc =?UTF-8?B?U2Now7x0eiI=?= <schuetzm gmx.net> writes:
On Wednesday, 5 August 2015 at 06:37:21 UTC, Walter Bright wrote:

 On 8/4/2015 8:26 PM, deadalnix wrote:

 On Tuesday, 4 August 2015 at 20:40:47 UTC, deadalnix wrote:

 Would you be of the opinion that assert should be a statement 
 rather than an
 expression ? unreadability in the middle of expressions, 
 especially when
 evaluation order is not well defined, is close to impossible 
 to get right.


 Ping ? Walter ?


 If you want it as its own statement, write it that way. I don't 
 see a need to change the language, nor would the breakage it 
 would cause be justified.


I suppose he's asking as maintainer of SDC.
Aug 05 2015
prev sibling parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 8/4/15 4:23 PM, Walter Bright wrote:

 On 8/4/2015 4:53 AM, Steven Schveighoffer wrote:

 Any assert(0) in druntime or phobos with a message printed is an
 error. It's
 compiled in release mode.


 When you're debugging phobos, you aren't compiling in release mode.


Unless you aren't debugging it, but an assert is still triggering. I 
want to stress, the bug that was found in core.time was environmental, 
and without the end user debugging it for us, we would never have known 
what this was, because the message wasn't printed.

I should say, any assert(0) with a message printed that is possible to 
trigger in release mode is an error. If you can ensure it's only 
possible to trigger when compiled in debug mode (or for unit tests), 
then assert(0, msg) is fine.


 But I've also been of the opinion, not widely shared, that any
 programmer supplied messages in asserts are pointless.


They can be helpful for context if it's not obvious what should have 
triggered the assert.

for example:

assert(x > y, "x: " ~ x.toString() ~ ", y: " ~ y.toString());

-Steve
Aug 04 2015
parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 8/4/15 4:43 PM, Steven Schveighoffer wrote:

 I should say, any assert(0) with a message printed that is possible to
 trigger in release mode is an error. If you can ensure it's only
 possible to trigger when compiled in debug mode (or for unit tests),
 then assert(0, msg) is fine.


In that vein, would it be possible to make this a warning? i.e. if you 
compile a file in -release mode, and the compiler encounters an 
assert(0, msg), it emits a warning?

-Steve
Aug 04 2015
parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Tuesday, 4 August 2015 at 20:48:52 UTC, Steven Schveighoffer 
wrote:

 On 8/4/15 4:43 PM, Steven Schveighoffer wrote:

 I should say, any assert(0) with a message printed that is 
 possible to
 trigger in release mode is an error. If you can ensure it's 
 only
 possible to trigger when compiled in debug mode (or for unit 
 tests),
 then assert(0, msg) is fine.


 In that vein, would it be possible to make this a warning? i.e. 
 if you compile a file in -release mode, and the compiler 
 encounters an assert(0, msg), it emits a warning?


I fail to see why it's a problem if assert(0) has a message. It's 
exactly the same as any other assertion except that it stays even 
with -release, and you don't get the message then (whereas with a 
normal assertion, you wouldn't get anything). Not being able to 
have a message for assert(0) would be just as bad as not having a 
message for any other assertion. By arguing that assert(0) should 
only have a message in debug mode, you'd be arguing for something 
like

static if(assert)
     assert(0, msg);
else
     assert(0);

which is pointless IMHO. I can definitely see an argument that we 
should be printing out something before the HLT instruction in 
-release mode, but as it stands, it's part of the spec that 
assert(0) just becomes a HLT instruction and so there's no 
message, and I don't think that should be a surprise given that 
the fact that assert(0) becomes a HLT instruction with -release 
is the key difference between it and other assertions. You don't 
get a warning about assertions having messages either, and their 
messages don't show up with -release. Instead, you get undefined 
behavior if they would have failed.

- Jonathan M Davis
Aug 04 2015
next sibling parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 8/4/15 4:56 PM, Jonathan M Davis wrote:

 On Tuesday, 4 August 2015 at 20:48:52 UTC, Steven Schveighoffer wrote:

 On 8/4/15 4:43 PM, Steven Schveighoffer wrote:

 I should say, any assert(0) with a message printed that is possible to
 trigger in release mode is an error. If you can ensure it's only
 possible to trigger when compiled in debug mode (or for unit tests),
 then assert(0, msg) is fine.


 In that vein, would it be possible to make this a warning? i.e. if you
 compile a file in -release mode, and the compiler encounters an
 assert(0, msg), it emits a warning?


 I fail to see why it's a problem if assert(0) has a message. It's
 exactly the same as any other assertion except that it stays even with
 -release, and you don't get the message then (whereas with a normal
 assertion, you wouldn't get anything).


Seeing an assert with a message gives the impression that you will see 
the message if you get there. I get that normal asserts may not even 
trigger. But the concept of "oh, if I see that message I know what 
happened" really goes out the window if you are never going to see it.

As I said earlier, any line in druntime that has an assert(0, msg) is a 
bug, because the message is never seen.

Doing the static if thing isn't the right answer either. The right 
answer is to choose one or the other (and by choosing to print a 
message, you could either throw an assert error directly, or print the 
message specifically and then assert(0) ).

So basically:

assert(0, msg);

becomes

printSomehow(msg);
assert(0);

With druntime it's difficult to actually print the message (and my PR is 
trying to fix that).

-Steve
Aug 04 2015
next sibling parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Tuesday, 4 August 2015 at 21:07:20 UTC, Steven Schveighoffer 
wrote:

 On 8/4/15 4:56 PM, Jonathan M Davis wrote:

 On Tuesday, 4 August 2015 at 20:48:52 UTC, Steven 
 Schveighoffer wrote:

 On 8/4/15 4:43 PM, Steven Schveighoffer wrote:

 I should say, any assert(0) with a message printed that is 
 possible to
 trigger in release mode is an error. If you can ensure it's 
 only
 possible to trigger when compiled in debug mode (or for unit 
 tests),
 then assert(0, msg) is fine.


 In that vein, would it be possible to make this a warning? 
 i.e. if you
 compile a file in -release mode, and the compiler encounters 
 an
 assert(0, msg), it emits a warning?


 I fail to see why it's a problem if assert(0) has a message. 
 It's
 exactly the same as any other assertion except that it stays 
 even with
 -release, and you don't get the message then (whereas with a 
 normal
 assertion, you wouldn't get anything).


 Seeing an assert with a message gives the impression that you 
 will see the message if you get there. I get that normal 
 asserts may not even trigger. But the concept of "oh, if I see 
 that message I know what happened" really goes out the window 
 if you are never going to see it.

 As I said earlier, any line in druntime that has an assert(0, 
 msg) is a bug, because the message is never seen.


It stills indicate why the assertion is there, which can be 
helpful to maintainers, and it _is_ possible to build druntime in 
debug mode. It's just not released that way (though it's been 
argued before that druntime/Phobos should be released with both 
debug and release builds).

And arguing against assert(0) with a message in druntime because 
it's not going to show the message normally would basically be 
the same as arguing against having any assertions in there at 
all, because they wouldn't show up in a typical build either. But 
that doesn't mean that they shouldn't be there, just that they're 
not as useful as they might otherwise be, because druntime is 
almost always built in release mode.


 Doing the static if thing isn't the right answer either. The 
 right answer is to choose one or the other (and by choosing to 
 print a message, you could either throw an assert error 
 directly, or print the message specifically and then assert(0) 
 ).

 So basically:

 assert(0, msg);

 becomes

 printSomehow(msg);
 assert(0);

 With druntime it's difficult to actually print the message (and 
 my PR is trying to fix that).


I really so no difference between having a message with an 
assert(0) as with any other assertion. You're not going to see 
either with -release. It's just that assert(0) kills your program 
instead of giving you undefined behavior.

I'm certainly not opposed to have a message be printed before the 
HLT instruction with assert(0), but I don't at all agree that the 
fact that the message is not seen in -release is a reason not to 
have a message.

- Jonathan M Davis
Aug 04 2015
parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 8/4/15 5:39 PM, Jonathan M Davis wrote:


 I'm certainly not opposed to have a message be printed before the HLT
 instruction with assert(0), but I don't at all agree that the fact that
 the message is not seen in -release is a reason not to have a message.


For instance:

https://github.com/D-Programming-Language/druntime/blob/master/src/core/time.d#L2283

This makes it seem like a message will be printed in the case where 
ticksPerSecond was 0. but in reality it simply throws a segfault.

Whether this happens or not in debug mode is pretty much irrelevant -- 
druntime is used in release mode by the vast majority of all developers, 
and this passes unit tests for us. It's the whole impetus for this 
thread, because someone actually did find a case where it gets there.

So why have a message with the clock name that failed? Why not just 
assert(0)? The only purpose I see for such a message is to trick the 
reviewer into accepting it (not that this was the intention of course) 
as being sufficiently explanatory when an error occurs.

We should always review such code with the view that when it *doesn't* 
print the message, is the error sufficient to a user such that they know 
where to look. I find it hard to believe it's *ever* sufficient, if you 
needed to have a message in the first place.

We can look at it this way -- if you need to add a message to an 
assert(0) for it to make sense, you should find a different way to 
communicate that.

-Steve
Aug 04 2015
next sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 8/4/2015 3:13 PM, Steven Schveighoffer wrote:

 For instance:

 https://github.com/D-Programming-Language/druntime/blob/master/src/core/time.d#L2283


 This makes it seem like a message will be printed in the case where
 ticksPerSecond was 0. but in reality it simply throws a segfault.


That's a bug in core/time.d, NOT a bug with asserts.
Aug 04 2015
parent Walter Bright <newshound2 digitalmars.com> writes:
On 8/4/2015 4:37 PM, Walter Bright wrote:

 On 8/4/2015 3:13 PM, Steven Schveighoffer wrote:

 For instance:

 https://github.com/D-Programming-Language/druntime/blob/master/src/core/time.d#L2283



 This makes it seem like a message will be printed in the case where
 ticksPerSecond was 0. but in reality it simply throws a segfault.


 That's a bug in core/time.d, NOT a bug with asserts.


https://issues.dlang.org/show_bug.cgi?id=14870
Aug 04 2015
prev sibling parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Tuesday, 4 August 2015 at 22:13:40 UTC, Steven Schveighoffer 
wrote:

 On 8/4/15 5:39 PM, Jonathan M Davis wrote:


 I'm certainly not opposed to have a message be printed before 
 the HLT
 instruction with assert(0), but I don't at all agree that the 
 fact that
 the message is not seen in -release is a reason not to have a 
 message.


 For instance:

 https://github.com/D-Programming-Language/druntime/blob/master/src/core/time.d#L2283

 This makes it seem like a message will be printed in the case 
 where ticksPerSecond was 0. but in reality it simply throws a 
 segfault.

 Whether this happens or not in debug mode is pretty much 
 irrelevant -- druntime is used in release mode by the vast 
 majority of all developers, and this passes unit tests for us. 
 It's the whole impetus for this thread, because someone 
 actually did find a case where it gets there.

 So why have a message with the clock name that failed? Why not 
 just assert(0)? The only purpose I see for such a message is to 
 trick the reviewer into accepting it (not that this was the 
 intention of course) as being sufficiently explanatory when an 
 error occurs.

 We should always review such code with the view that when it 
 *doesn't* print the message, is the error sufficient to a user 
 such that they know where to look. I find it hard to believe 
 it's *ever* sufficient, if you needed to have a message in the 
 first place.

 We can look at it this way -- if you need to add a message to 
 an assert(0) for it to make sense, you should find a different 
 way to communicate that.


It was never expected that a user to see any of those messages 
anyway. The idea was that if they failed, there was something 
seriously wrong, and the program needed to be killed. If the 
message prints when it fails, great. It's more explanatory that 
way, but it's a bonus (and one that you do get if druntime is 
build in debug mode). The purpose was to kill the program, 
because it was in an invalid state.

- Jonathan M Davis
Aug 04 2015
parent "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Wednesday, 5 August 2015 at 02:52:40 UTC, Jonathan M Davis 
wrote:

 On Tuesday, 4 August 2015 at 22:13:40 UTC, Steven Schveighoffer 
 wrote:

 On 8/4/15 5:39 PM, Jonathan M Davis wrote:


 I'm certainly not opposed to have a message be printed before 
 the HLT
 instruction with assert(0), but I don't at all agree that the 
 fact that
 the message is not seen in -release is a reason not to have a 
 message.


 For instance:

 https://github.com/D-Programming-Language/druntime/blob/master/src/core/time.d#L2283

 This makes it seem like a message will be printed in the case 
 where ticksPerSecond was 0. but in reality it simply throws a 
 segfault.

 Whether this happens or not in debug mode is pretty much 
 irrelevant -- druntime is used in release mode by the vast 
 majority of all developers, and this passes unit tests for us. 
 It's the whole impetus for this thread, because someone 
 actually did find a case where it gets there.

 So why have a message with the clock name that failed? Why not 
 just assert(0)? The only purpose I see for such a message is 
 to trick the reviewer into accepting it (not that this was the 
 intention of course) as being sufficiently explanatory when an 
 error occurs.

 We should always review such code with the view that when it 
 *doesn't* print the message, is the error sufficient to a user 
 such that they know where to look. I find it hard to believe 
 it's *ever* sufficient, if you needed to have a message in the 
 first place.

 We can look at it this way -- if you need to add a message to 
 an assert(0) for it to make sense, you should find a different 
 way to communicate that.


 It was never expected that a user to see any of those messages 
 anyway. The idea was that if they failed, there was something 
 seriously wrong, and the program needed to be killed. If the 
 message prints when it fails, great. It's more explanatory that 
 way, but it's a bonus (and one that you do get if druntime is 
 build in debug mode). The purpose was to kill the program, 
 because it was in an invalid state.


Maybe in this case, it would have made more sense to simply throw 
an Error rather than use assert(0), since it inadvertently ended 
up depending on the system's environment, even though it was 
supposed to be guaranteed to work (differences in kernel versions 
was not taken into account). And I assume that if we threw an 
Error, then we'd get a message.

- Jonathan M Davis
Aug 04 2015
prev sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 8/4/2015 2:07 PM, Steven Schveighoffer wrote:

 With druntime it's difficult to actually print the message


     fprintf(stderr, "You left the headlights on.");
     assert(0);
Aug 04 2015
next sibling parent reply "deadalnix" <deadalnix gmail.com> writes:
On Tuesday, 4 August 2015 at 23:33:17 UTC, Walter Bright wrote:

 On 8/4/2015 2:07 PM, Steven Schveighoffer wrote:

 With druntime it's difficult to actually print the message


     fprintf(stderr, "You left the headlights on.");
     assert(0);


It seems that on american car, you bip as annoyingly as possible 
instead of announcing what's wrong. Preferably for anything and 
its reverse.
Aug 04 2015
parent Walter Bright <newshound2 digitalmars.com> writes:
On 8/4/2015 4:56 PM, deadalnix wrote:

 It seems that on american car, you bip as annoyingly as possible instead of
 announcing what's wrong. Preferably for anything and its reverse.


It turns out you can make quite understandable speech simply by connecting an 
on/off I/O port to a speaker. So there is no excuse for using tones instead of 
"You left the headlights on, you ******* ****!"
Aug 04 2015
prev sibling parent Steven Schveighoffer <schveiguy yahoo.com> writes:
On 8/4/15 7:33 PM, Walter Bright wrote:

 On 8/4/2015 2:07 PM, Steven Schveighoffer wrote:

 With druntime it's difficult to actually print the message


      fprintf(stderr, "You left the headlights on.");
      assert(0);


Going under the assumption that the program isn't fully "with it", I 
don't want to use such complicated libraries as stdio.

In any case, the PR I have does it with system calls only.

-Steve
Aug 04 2015
prev sibling parent Walter Bright <newshound2 digitalmars.com> writes:
On 8/4/2015 1:56 PM, Jonathan M Davis wrote:

 I can definitely see an argument that we should be
 printing out something before the HLT instruction in -release mode,


     writeln("Somebody forgot to turn off the water.");
     assert(0);
Aug 04 2015
prev sibling next sibling parent "Meta" <jared771 gmail.com> writes:
On Monday, 3 August 2015 at 23:57:36 UTC, Steven Schveighoffer 
wrote:

 OK, this brings up another debate. The thing that triggered all 
 this is an issue with core.time, see issue 
 https://issues.dlang.org/show_bug.cgi?id=14863

 [...]


Why not just " stderr.writeln(errMsg); assert(0);"?
Aug 03 2015
prev sibling next sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 8/3/2015 4:57 PM, Steven Schveighoffer wrote:

 OK, this brings up another debate. The thing that triggered all this is an
issue
 with core.time, see issue https://issues.dlang.org/show_bug.cgi?id=14863

 Essentially, we wrote code to get all the clock information at startup on a
 posix system that supports clock_gettime, which is immutable and can be read
 easily at startup. However, how we handled the case where a clock could not be
 fetched is to assert(0, "don't support clock " ~ clockname).

 The clear expectation was that the message will be printed (along with
file/line
 number), and we can go fix the issue.

 On Linux 2.6.32 - a supported LTS release I guess - one of these clocks is not
 supported. This means running a simple "hello world" program crashes at startup
 in a segfault, not a nice informative message.

 So what is the right answer here?


The answer is the code is misusing asserts to check for environmental errors. I 
cannot understand how I consistently fail at explaining this. There have been 
many thousand message threads on exactly this topic.

Asserts are for CHECKING FOR PROGRAM BUGS.

enforce(), etc., are for CHECKING FOR INPUT ERRORS. Environmental errors are 
input errors, not programming bugs.
Aug 03 2015
parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 8/3/15 9:11 PM, Walter Bright wrote:


 enforce(), etc., are for CHECKING FOR INPUT ERRORS. Environmental errors
 are input errors, not programming bugs.


enforce isn't available in druntime. In this case, we don't want to 
throw an exception anyways, or we would have to remove nothrow from 
core.time. Technically, usage of this unsupported clock will ALWAYS 
throw, or NEVER throw. That's difficult to encode into the system.

The only answer I can think of is to write a message to stderr, and then 
assert(0) as meta suggested.

-Steve
Aug 04 2015
parent Walter Bright <newshound2 digitalmars.com> writes:
On 8/4/2015 4:58 AM, Steven Schveighoffer wrote:

 The only answer I can think of is to write a message to stderr, and then
 assert(0) as meta suggested.


Print an error message and exit via abort() or whatever. No need to generate a 
halt - the program hasn't entered an unknown state.
Aug 04 2015
prev sibling parent reply "Joseph Rushton Wakeling" <joseph.wakeling webdrake.net> writes:
On Monday, 3 August 2015 at 23:57:36 UTC, Steven Schveighoffer 
wrote:

 At the very least, assert(0, "message") should be a compiler 
 error, the message is unused information.


Only if you compile in -release mode.  Without wanting to get 
into a bikeshedding debate, I think that flag name may be 
mislead: it's not about release vs. development, it's about the 
tradeoff you are making between speed and safety.
Aug 05 2015
parent "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Wednesday, 5 August 2015 at 08:09:49 UTC, Joseph Rushton 
Wakeling wrote:

 On Monday, 3 August 2015 at 23:57:36 UTC, Steven Schveighoffer 
 wrote:

 At the very least, assert(0, "message") should be a compiler 
 error, the message is unused information.


 Only if you compile in -release mode.  Without wanting to get 
 into a bikeshedding debate, I think that flag name may be 
 mislead: it's not about release vs. development, it's about the 
 tradeoff you are making between speed and safety.


That's arguably true, but historically, release builds are where 
you turn off assertions and turn on optimizations. So, it does 
pretty much what most folks would expect from a release build 
(though some folks might assume that it turns on optimizations as 
well, which it doesn't). Now, there are good reasons to leave 
assertions in in release builds, but then you're arguably just 
using your debug builds in production (probably because you don't 
need the efficiency gain and are too paranoid to risk turning the 
assertions off).

So, you have a good point, but really, -release does with 
assertions what's normally expected of release builds, so I don't 
think that it's actually misleading at all.

- Jonathan M Davis
Aug 05 2015
prev sibling parent reply Iain Buclaw via Digitalmars-d <digitalmars-d puremagic.com> writes:
On 4 August 2015 at 00:54, Walter Bright via Digitalmars-d <
digitalmars-d puremagic.com> wrote:


 On 8/3/2015 8:50 AM, Steven Schveighoffer wrote:


 1. They aren't removed, they are replaced with a nearly useless segfault.


 Not useless at all:

 1. The program does not continue running after it has failed. (Please,
 let's not restart that debate.
 2. Running it under a debugger, the location of the fault will be
 identified.


 2. If we are going to put something in there instead of "assert", why not

 just
 throw an error?


 Because they are expensive.

 To keep asserts in all their glory in released code, do not use the
 -release switch.


I think as a guideline, there are really only be two places where using an
assert makes sense in druntime/phobos.

1. Internal modules, such as rt and core.internal.
2. Inside contracts (in, out, invariant) or unittests.


Everywhere else - *especially* if it's part of a public API like core.time
- you should throw an Exception or Error based on what you see fit.

Because of this view, I'm not really in agreement with the addition of a
core.abort module.

Regards
Iain
Aug 05 2015
parent reply "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On Wednesday, 5 August 2015 at 10:31:40 UTC, Iain Buclaw wrote:

 I think as a guideline, there are really only be two places 
 where using an assert makes sense in druntime/phobos.

 1. Internal modules, such as rt and core.internal.
 2. Inside contracts (in, out, invariant) or unittests.


 Everywhere else - *especially* if it's part of a public API 
 like core.time - you should throw an Exception or Error based 
 on what you see fit.


I think that it makes perfect sense to use assertions inside of 
druntime or Phobos functions when testing the correctness of 
their internal state, but it obviously can't be expected that 
they run normally (at least in non-templated code), and it can't 
be expected that much beyond the unit tests run them (at least in 
non-templated code), since almost no one is going to be using a 
debug version of druntime/Phobos. Certainly, I don't see anything 
special about druntime or Phobos with regards to assertions 
except for the fact that not much beyond the unit tests is going 
to be using druntime/Phobos in debug mode, so assertions in 
non-templated code simply aren't going to be doing anything 
outside of the unit test builds.

As for assert(0) specifically, it makes sense in cases where the 
code should never ever hit that point, and there's a serious 
problem if it does (e.g. the default branch of a switch statement 
where it should be impossible for that branch to be hit), but it 
was a mistake to use it in core.time, since that was failing 
based on a system call returning a failure (which should 
basically never happen in this case, but it still depends on the 
environment and thus could conceivably fail as opposed to only 
ever failing if druntime's code is definitely broken).


 Because of this view, I'm not really in agreement with the 
 addition of a core.abort module.


Well, then please comment on the PR: 
https://github.com/D-Programming-Language/druntime/pull/1337

It seems to me that the whole point of abort is to work around 
the fact that Walter doesn't want assert(0) to print messages 
before the HLT instruction in release mode. Certainly, I can't 
see any other reason for it.

- Jonathan M Davis
Aug 05 2015
parent Steven Schveighoffer <schveiguy yahoo.com> writes:
On 8/5/15 12:46 PM, Jonathan M Davis wrote:

 Well, then please comment on the PR:
 https://github.com/D-Programming-Language/druntime/pull/1337

 It seems to me that the whole point of abort is to work around the fact
 that Walter doesn't want assert(0) to print messages before the HLT
 instruction in release mode. Certainly, I can't see any other reason for
 it.


No, the reason is, if you want to print something before halting, and 
you don't want to use any subsystems that you suspect are corrupt (e.g. 
stdio), it's not a simple task. This makes it as simple as calling 
assert. I thought it would be a useful utility.

Surely, we can improve core.time to throw in this case instead, since 
the case where the ticksPerSecond is zero only happens when the OS did 
not provide it. A slight possibility is that the memory was corrupted.

But I think the abort function still has use in other cases.

-Steve
Aug 05 2015
prev sibling parent reply "Ola Fosheim =?UTF-8?B?R3LDuHN0YWQi?= writes:
On Monday, 3 August 2015 at 15:18:12 UTC, Dicebot wrote:

 On Monday, 3 August 2015 at 14:34:52 UTC, Steven Schveighoffer 
 wrote:

 Why do we do this?


 Because all asserts must be completely removed in -release

 Yet assert(0) effectively mean "unreachable code" (it is 
 actually defined that way in spec) and thus it is possible to 
 ensure extra "free" bit of safety by crashing the app.


It isn't free. The point of having unreachable as primitive is 
that you can remove all branching that leads to it.
Aug 03 2015
parent reply "Dicebot" <public dicebot.lv> writes:
On Tuesday, 4 August 2015 at 02:37:00 UTC, Ola Fosheim Grøstad 
wrote:

 On Monday, 3 August 2015 at 15:18:12 UTC, Dicebot wrote:

 On Monday, 3 August 2015 at 14:34:52 UTC, Steven Schveighoffer 
 wrote:

 Why do we do this?


 Because all asserts must be completely removed in -release

 Yet assert(0) effectively mean "unreachable code" (it is 
 actually defined that way in spec) and thus it is possible to 
 ensure extra "free" bit of safety by crashing the app.


 It isn't free. The point of having unreachable as primitive is 
 that you can remove all branching that leads to it.


"free compared to throwing exception" would be a better wording 
indeed. I remember Iain complaining about presence of HLT (and 
saying he won't do that for gdc) for exactly this reason.
Aug 03 2015
parent reply "Ola Fosheim =?UTF-8?B?R3LDuHN0YWQi?= writes:
On Tuesday, 4 August 2015 at 03:04:14 UTC, Dicebot wrote:

 indeed. I remember Iain complaining about presence of HLT (and 
 saying he won't do that for gdc) for exactly this reason.


Yes. I think a lot of this confusion could have been avoided by 
using "halt()" instead of "assert(0)" and "unreachable()" for 
marking code that has been proven unreachable. Right now it isn't 
obvious to me as a programmer what would happen for 
"assert(x-x)". I would have to look it up.

A more principled approach would be:

- "enforce(...)" test for errors

- "assert(...)" are (lint-like) annotations that don't affect 
normal execution, but can be requested to be dynamically tested.

- "halt()" marks sudden termination

- "unreachable()" injects a proven assumption into the deductive 
database

-"assume(...)" injects a proven assumption into the deductive 
database

- contracts define interfaces between separate pieces of code 
(like a plugin or dynamically linked library from multiple 
vendors e.g. sub system interface) that you may turn on/off based 
on what you interface with.

The input/environment/code distinction does not work very well. 
Sometimes input is a well defined part of the system, sometimes 
input is code (like dynamic linking a plugin), etc...
Aug 03 2015
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 8/3/2015 8:37 PM, Ola Fosheim =?UTF-8?B?R3LDuHN0YWQi?= 
<ola.fosheim.grostad+dlang gmail.com> wrote:

 The input/environment/code distinction does not work very well.


Sure it does. If your user ever sees an assert failure message, your program
has 
a bug in it. Keep that in mind when designing the code, and the distinction
will 
become clear.
Aug 04 2015
parent reply "Ola Fosheim =?UTF-8?B?R3LDuHN0YWQi?= writes:
On Tuesday, 4 August 2015 at 20:21:24 UTC, Walter Bright wrote:

 On 8/3/2015 8:37 PM, Ola Fosheim =?UTF-8?B?R3LDuHN0YWQi?= 
 <ola.fosheim.grostad+dlang gmail.com> wrote:

 The input/environment/code distinction does not work very well.


 Sure it does. If your user ever sees an assert failure message, 
 your program has a bug in it.


Yes.


 Keep that in mind when designing the code, and the distinction 
 will become clear.


The input/code distinction is too simplistic.

Example 1: It makes perfect sense to assert (or assume) that a 
value from a hardware register or cpu instruction is within 
range. If the assert fires, it is the spec/code that is wrong, 
not the input. So you are testing the specification in the code 
against well defined input.

Example 2: It makes perfect sense to enforce that a return value 
from a plugin library is within range to maintain main program 
integrity and shut out a misbehaving plugin.

And so on.
Aug 04 2015
parent reply "John Colvin" <john.loughran.colvin gmail.com> writes:
On Tuesday, 4 August 2015 at 22:06:06 UTC, Ola Fosheim Grøstad 
wrote:

 On Tuesday, 4 August 2015 at 20:21:24 UTC, Walter Bright wrote:

 On 8/3/2015 8:37 PM, Ola Fosheim =?UTF-8?B?R3LDuHN0YWQi?= 
 <ola.fosheim.grostad+dlang gmail.com> wrote:

 The input/environment/code distinction does not work very 
 well.


 Sure it does. If your user ever sees an assert failure 
 message, your program has a bug in it.


 Yes.


 Keep that in mind when designing the code, and the distinction 
 will become clear.


 The input/code distinction is too simplistic.

 Example 1: It makes perfect sense to assert (or assume) that a 
 value from a hardware register or cpu instruction is within 
 range. If the assert fires, it is the spec/code that is wrong, 
 not the input. So you are testing the specification in the code 
 against well defined input.

 Example 2: It makes perfect sense to enforce that a return 
 value from a plugin library is within range to maintain main 
 program integrity and shut out a misbehaving plugin.

 And so on.


I'm 90% on Walter's side here as he's right for the majority of 
common cases, but on the other hand there are plenty of 
situations where the boundary between code error and environment 
error get blurred.
Aug 04 2015
parent reply "Ola Fosheim =?UTF-8?B?R3LDuHN0YWQi?= writes:
On Tuesday, 4 August 2015 at 23:06:44 UTC, John Colvin wrote:

 I'm 90% on Walter's side here as he's right for the majority of 
 common cases, but on the other hand there are plenty of 
 situations where the boundary between code error and 
 environment error get blurred.


Well, the principled difference is that asserts are 
program-specification-annotations and should not in any way 
affect execution within the code unit, and if it does it should 
happen as if it was detected by a hypothetical supervisor 
external to the program.

E.g. for a batch program terminate and have the calling context 
unwind any side-effects (as in a transaction), or for an 
interactive program to enter emergency mode, save state in a 
temporary file and try to recover after restart.

Asserts are reflecting system specifications (not code, or just 
applying to code).

If you want to take height for code errors you should use enforce 
or throw. It makes plenty of sense to assume that code have 
errors, and to recover from it, but using asserts does not assume 
that you have errors. It is just a way to add specification to 
the code for validation/documentation purposes.

I don't want those in release at all. I have way to many asserts 
for that.

So this goes much deeper than you and Walter suggest.
Aug 04 2015
parent "Ola Fosheim =?UTF-8?B?R3LDuHN0YWQi?= writes:
On Wednesday, 5 August 2015 at 05:51:32 UTC, Ola Fosheim Grøstad 
wrote:

 specification to the code for validation/documentation purposes.


Err... Verification, usually not validation.
Aug 05 2015
prev sibling parent reply "Dicebot" <public dicebot.lv> writes:
General advice  - simply don't ever use -release unless you are 
_very_ sure about program correctness (to the point of 100% test 
coverage and previous successful debug runs)
Aug 03 2015
next sibling parent Steven Schveighoffer <schveiguy yahoo.com> writes:
On 8/3/15 11:59 AM, Dicebot wrote:

 General advice  - simply don't ever use -release unless you are _very_
 sure about program correctness (to the point of 100% test coverage and
 previous successful debug runs)


So in other words, only release code that has no bugs. Got it ;)

-Steve
Aug 03 2015
prev sibling parent reply Nick Sabalausky <SeeWebsiteToContactMe semitwist.com> writes:
On 08/03/2015 11:59 AM, Dicebot wrote:

 General advice  - simply don't ever use -release unless you are _very_
 sure about program correctness (to the point of 100% test coverage and
 previous successful debug runs)


This is very true. I never disable asserts or bounds checking for 
exactly that reason - you can NEVER conclusively determine through 
prerelease testing that none of those conditions are going to get 
tripped in real-world usage. ANY developer who thinks they can is 
absolutely fooling themself. And what happens for the end user WHEN one 
of those conditions does occur? Memory corruption or otherwise invalid 
state. Things go boom. Whee. BAD idea.

There is only ONE time when asserts or bounds checking should EVER be 
disabled and that's on a per-function basis (split it out into a 
separate module if you need to) AFTER profiling has determined that 
specific location to be a significant bottleneck, and the code in 
question has been (and will continue to be during all future 
maintenance) VERY carefully combed-over and peer-reviewed to ensure (as 
much as possible) that disabling asserts/bounds checks on that localized 
function cannot lead to corruption, exploits or invalid state.
Aug 04 2015
parent reply "Dicebot" <public dicebot.lv> writes:
On Tuesday, 4 August 2015 at 14:40:16 UTC, Nick Sabalausky wrote:

 On 08/03/2015 11:59 AM, Dicebot wrote:

 General advice  - simply don't ever use -release unless you 
 are _very_
 sure about program correctness (to the point of 100% test 
 coverage and
 previous successful debug runs)


 This is very true. I never disable asserts or bounds checking 
 for exactly that reason - you can NEVER conclusively determine 
 through prerelease testing that none of those conditions are 
 going to get tripped in real-world usage. ANY developer who 
 thinks they can is absolutely fooling themself. And what 
 happens for the end user WHEN one of those conditions does 
 occur? Memory corruption or otherwise invalid state. Things go 
 boom. Whee. BAD idea.

 There is only ONE time when asserts or bounds checking should 
 EVER be disabled and that's on a per-function basis (split it 
 out into a separate module if you need to) AFTER profiling has 
 determined that specific location to be a significant 
 bottleneck, and the code in question has been (and will 
 continue to be during all future maintenance) VERY carefully 
 combed-over and peer-reviewed to ensure (as much as possible) 
 that disabling asserts/bounds checks on that localized function 
 cannot lead to corruption, exploits or invalid state.


Recently we had quite a lengthy discussion at work regarding 
possible guidelines for using asserts, contracts and enforce (we 
have similar own implementation) that would actually allow using 
-release flag for release builds. And got to certain principles 
that I believe may work in practice (even though they violate DbC 
ideology). I will check if I can publish those here.
Aug 04 2015
parent Walter Bright <newshound2 digitalmars.com> writes:
On 8/4/2015 8:04 AM, Dicebot wrote:

 Recently we had quite a lengthy discussion at work regarding possible
guidelines
 for using asserts, contracts and enforce (we have similar own implementation)
 that would actually allow using -release flag for release builds. And got to
 certain principles that I believe may work in practice (even though they
violate
 DbC ideology). I will check if I can publish those here.


That's worthy of more than just a newsgroup post. How about an article or a
wiki 
entry?
Aug 04 2015