Hello DMD devs,

Since we switched to DDMD, there has been a major regression hanging around: asserts are stripped out of the version we ship (except for assert(0)), because for performance reason we ship with `-release`.
This can lead to very subtle and hard to spot bug in user code, and requiring someone to build a debug version of DMD in order to post a bug report is a clear red flag.

There has been discussions on what to do with this, in this P.R. : https://github.com/dlang/dmd/pull/5716

One of the solution proposed by Martin was to provide a switch to allow `assert`. This has the disadvantage of still stripping `invariant` and `in`/`out` contracts, and might be confusing to user code.

One other solution - which doesn't require a language change - would be to use exception for ICE (which are rare anyway), and provide a comprehensive handler at the top level to:
- Provide a nice error message
- Provide a dump of the code / AST that produced the error
- Point to bugzilla, as suggested in a recent P.R. by Dicebot: https://github.com/dlang/dmd/pull/6103

Thoughts ?

_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

On 09/27/2016 11:40 AM, Mathias Lang via dmd-internals wrote:
> Hello DMD devs,
> 
> Since we switched to DDMD, there has been a major regression hanging around: asserts are stripped out of the version we ship (except for assert(0)), because for performance reason we ship with `-release`. This can lead to very subtle and hard to spot bug in user code, and requiring someone to build a debug version of DMD in order to post a bug report is a clear red flag.

It is actually worse than that because if any of non-assert(0) check fails, developer want even see any clear ICE - it may or may not crash in some random place later.

Because of that issue we currently have no idea how many ICE issues may still remain in the frontend part.

> There has been discussions on what to do with this, in this P.R. : https://github.com/dlang/dmd/pull/5716

I must admit I was sure this PR got merged long time ago. The fact that Martin decided 5% performance is more important than risk of generating wrong code is worrying :(

> One other solution - which doesn't require a language change - would be to use exception for ICE (which are rare anyway)

Would be interesting to check all usage of `assert` and see:

a) how many of them protect from something truly bad
b) are there many "real" asserts (i.e. not assert(0)) in general

On 09/28/2016 02:19 PM, Михаил Страшун via dmd-internals wrote:
>
>> One other solution - which doesn't require a language change - would be
>> to use exception for ICE (which are rare anyway)
> Would be interesting to check all usage of `assert` and see:
>
> a) how many of them protect from something truly bad
> b) are there many "real" asserts (i.e. not assert(0)) in general
>
>

I'd say quite a lot:

grep -nR "assert\b*(" src/*.d | grep -v "static assert" | grep -v "assert\b*(0)" | wc -l
1083


It is hard to investigate how many prevents bug, but I had a quick look and it's not a neglectible amount sadly.
_______________________________________________
dmd-internals mailing list
dmd-internals@puremagic.com
http://lists.puremagic.com/mailman/listinfo/dmd-internals

On 09/27/2016 11:40 AM, Mathias Lang via dmd-internals wrote:
> One of the solution proposed by Martin was to provide a switch to allow `assert`. This has the disadvantage of still stripping `invariant` and `in`/`out` contracts, and might be confusing to user code.

We don't really use contracts/invariants in dmd. And honestly, except
for class hierarchies (where the implementation is fairly buggy though),
contracts/invariants don't provide much over asserts.
We also guessed that invariants are mainly responsible for the high cost
of disabling release.

The idea for the switch was -release=assert,invariant,contracts to reenable particular checks.

> One other solution - which doesn't require a language change - would be to use exception for ICE (which are rare anyway), and provide a comprehensive handler at the top level to:

Assertions already are using EH (they are Errors), but not in release
builds.
Currently EH still is far from zero cost (not sure about nothrow Errors
in release).
For now I'd be in favor of a pragmatic iceAssert or so method that just
prints file+line and exits (in fact just reuse much like the old
assert). In fact, just reuse util_assert and be done.

> - Provide a nice error message

Nobody will write/maintain nice errors for ICEs, they should simply never happen. Exact compiler version (dmd --version provided by reporter), file, line number, and maybe some extra arguments is all that is needed.

> - Provide a dump of the code / AST that produced the error

Fancy, but w/ Dustmite we already have a tool to reduce test cases. Also ICEs are usually rather simple to fix.

> - Point to bugzilla, as suggested in a recent P.R. by Dicebot: https://github.com/dlang/dmd/pull/6103

Yes, that's a good idea.