www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Make using compiled libs with debug code better

reply Steven Schveighoffer <schveiguy gmail.com> writes:
I wrote about this in a reply to a learn thread, but realized I should 
really post it here on its own.

A user posted seemingly innocuous code that caused a segmentation fault 
[here](https://forum.dlang.org/post/nkfejgkrzntybqetkvkj forum.dlang.org), 
and also asked about it on Discord. We worked through trying to figure 
out the issue, and it turns out it was a memory corruption (the seg 
fault was deep inside the garbage collector).

But not one that I would have expected. It was an out-of-bounds access 
passed into a 
[std.bitmanip.BitArray](https://dlang.org/phobos/std_bitmanip.html#BitArray), 
which corrupted GC metadata, and caused a later allocation to fail. Like 
any reasonable array type, `BitArray` uses asserts to enforce bounds. 
And he did NOT turn off bounds checks.

So what happened? Phobos is compiled in release mode, even if *your code 
is not*. So bounds checks are disabled based on the type.

If `BitArray` was a template, it might have bounds checks enabled. But 
maybe not, if the compiler detected it was already instantiated inside 
Phobos and decided it didn't need to emit the code for it. This kind of 
Russian Roulette seems prone to cause hours of debugging when it could 
give you an answer instantly.

How do we fix such a thing? The easiest thing I can think of is to ship 
an assert-enabled version of Phobos, and use that when requested. This 
at least allows a quick mechanism to test your code with bounds checks 
(and other asserts) enabled. LDC actually already has this, see 
https://d.godbolt.org/z/feETE8W78 (courtesy of Max Haughton)

But this feels awkward. If you build your code with bounds checks, you 
would like the libraries you use to have them also. And a compiler 
switch isn't going to help you when you are using other libs.

Now consider that `BitArray`, being quite old, uses contracts to enforce 
its bounds checks. However, D runs the contracts inside the function 
itself, instead of at the call site. But this seems wrong to me, as the 
contracts are checking the *caller's* code, not the *callee's*.

Would it be a reasonable thing to change contracts to be called by the 
caller instead of the callee? Is that something that could make its way 
into D, such that the input checking of functions that are compiled for 
release still can run when you compile your code in non-release mode?

-Steve
Oct 18 2021
next sibling parent Ola Fosheim =?UTF-8?B?R3LDuHN0YWQ=?= <ola.fosheim.grostad gmail.com> writes:
On Monday, 18 October 2021 at 13:04:47 UTC, Steven Schveighoffer 
wrote:

 Would it be a reasonable thing to change contracts to be called 
 by the caller instead of the callee? Is that something that 
 could make its way into D, such that the input checking of 
 functions that are compiled for release still can run when you 
 compile your code in non-release mode?


Isn't this how contracts should work in general? You test the 
preconditions before calling the function, hoping that the 
optimizer will resolve them at compiletime?
Oct 18 2021
prev sibling next sibling parent Imperatorn <johan_forsberg_86 hotmail.com> writes:
On Monday, 18 October 2021 at 13:04:47 UTC, Steven Schveighoffer 
wrote:

 I wrote about this in a reply to a learn thread, but realized I 
 should really post it here on its own.

 [...]


The first step is to just have an assert-enabled version at least.
Oct 18 2021
prev sibling next sibling parent "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
On Mon, Oct 18, 2021 at 09:04:47AM -0400, Steven Schveighoffer via
Digitalmars-d wrote:
[...]

 Would it be a reasonable thing to change contracts to be called by the
 caller instead of the callee?


We've brought this up before years ago.  For whatever reason nothing
came of that discussion.  But yeah, it doesn't make sense for contracts
to be called in the callee; it should be called in the caller. Well,
in-contracts, anyway. Out-contracts probably should be in the callee.



 Is that something that could make its way into D, such that the input
 checking of functions that are compiled for release still can run when
 you compile your code in non-release mode?


[...]

That would be very nice.


T

-- 
If it breaks, you get to keep both pieces. -- Software disclaimer notice
Oct 18 2021
prev sibling next sibling parent reply Elronnd <elronnd elronnd.net> writes:
On Monday, 18 October 2021 at 13:04:47 UTC, Steven Schveighoffer 
wrote:

 Would it be a reasonable thing to change contracts to be called 
 by the caller instead of the callee? Is that something that 
 could make its way into D, such that the input checking of 
 functions that are compiled for release still can run when you 
 compile your code in non-release mode?


What if I take a pointer to a function with contracts and call it 
from someplace that doesn't know about those contracts?

Better to leave assertions on by default, and make you have to go 
out of your way to turn them off.  IMO.
Oct 18 2021
parent reply Steven Schveighoffer <schveiguy gmail.com> writes:
On 10/18/21 5:51 PM, Elronnd wrote:

 On Monday, 18 October 2021 at 13:04:47 UTC, Steven Schveighoffer wrote:

 Would it be a reasonable thing to change contracts to be called by the 
 caller instead of the callee? Is that something that could make its 
 way into D, such that the input checking of functions that are 
 compiled for release still can run when you compile your code in 
 non-release mode?


 
 What if I take a pointer to a function with contracts and call it from 
 someplace that doesn't know about those contracts?


A good point. There are also issues with virtual functions along these 
lines.

The only thing I can think of is to compile the contract code, provide 2 
entry points for the function, and let the caller pick the one they 
want. This would have the same effect.


 
 Better to leave assertions on by default, and make you have to go out of 
 your way to turn them off.  IMO.


This would leave Phobos in pretty bad shape for performance. Especially 
with invariants. I know for instance that RedBlackTree has an invariant 
that validates the tree correctness on every operation (and this runs 
before and after every function). However, if you build your project 
with dub, every library you depend on (with source available) will be 
built the same as your application. So Phobos is kind of alone in this 
regard based on the way we distribute libraries.

I'm not sure what the right answer is, but that is what this thread is 
for -- can we solve this in a non-disruptive or minimally-disruptive way?

-Steve
Oct 19 2021
next sibling parent Paul Backus <snarwin gmail.com> writes:
On Tuesday, 19 October 2021 at 13:57:31 UTC, Steven Schveighoffer 
wrote:

 Better to leave assertions on by default, and make you have to 
 go out of your way to turn them off.  IMO.


 This would leave Phobos in pretty bad shape for performance. 
 Especially with invariants. I know for instance that 
 RedBlackTree has an invariant that validates the tree 
 correctness on every operation (and this runs before and after 
 every function). However, if you build your project with dub, 
 every library you depend on (with source available) will be 
 built the same as your application. So Phobos is kind of alone 
 in this regard based on the way we distribute libraries.

 I'm not sure what the right answer is, but that is what this 
 thread is for -- can we solve this in a non-disruptive or 
 minimally-disruptive way?


1. Add a `-preview=releaseModeChecks` switch that enables 
assertions and contract checks in `-release` builds.
2. Build Phobos's unit tests with and without the new `-preview` 
switch and check for performance regressions.
3. Add `debug` to any assertions or contracts that cause 
significant performance regressions.
Oct 19 2021
prev sibling next sibling parent Tim <tim.dlang t-online.de> writes:
On Tuesday, 19 October 2021 at 13:57:31 UTC, Steven Schveighoffer 
wrote:

 On 10/18/21 5:51 PM, Elronnd wrote:

 What if I take a pointer to a function with contracts and call 
 it from someplace that doesn't know about those contracts?


 A good point. There are also issues with virtual functions 
 along these lines.


For virtual functions it would be better to evaluate the in 
contract at the call site, because the caller has to fulfill the 
in contract. Here is the relevant bug report: 
https://issues.dlang.org/show_bug.cgi?id=6857
Oct 19 2021
prev sibling parent bauss <jj_1337 live.dk> writes:
On Tuesday, 19 October 2021 at 13:57:31 UTC, Steven Schveighoffer 
wrote:

 On 10/18/21 5:51 PM, Elronnd wrote:

 On Monday, 18 October 2021 at 13:04:47 UTC, Steven 
 Schveighoffer wrote:

 Would it be a reasonable thing to change contracts to be 
 called by the caller instead of the callee? Is that something 
 that could make its way into D, such that the input checking 
 of functions that are compiled for release still can run when 
 you compile your code in non-release mode?


 
 What if I take a pointer to a function with contracts and call 
 it from someplace that doesn't know about those contracts?


 A good point. There are also issues with virtual functions 
 along these lines.

 The only thing I can think of is to compile the contract code, 
 provide 2 entry points for the function, and let the caller 
 pick the one they want. This would have the same effect.


 
 Better to leave assertions on by default, and make you have to 
 go out of your way to turn them off.  IMO.


 This would leave Phobos in pretty bad shape for performance. 
 Especially with invariants. I know for instance that 
 RedBlackTree has an invariant that validates the tree 
 correctness on every operation (and this runs before and after 
 every function). However, if you build your project with dub, 
 every library you depend on (with source available) will be 
 built the same as your application. So Phobos is kind of alone 
 in this regard based on the way we distribute libraries.

 I'm not sure what the right answer is, but that is what this 
 thread is for -- can we solve this in a non-disruptive or 
 minimally-disruptive way?

 -Steve


The solution to that problem would be assertion levels, like you 
declare how important an assertion is and then you can toggle 
based on that.
Oct 19 2021
prev sibling next sibling parent apz28 <home home.com> writes:
On Monday, 18 October 2021 at 13:04:47 UTC, Steven Schveighoffer 
wrote:

 I wrote about this in a reply to a learn thread, but realized I 
 should really post it here on its own.

 A user posted seemingly innocuous code that caused a 
 segmentation fault


Learn from Delphi. It implement long time ago.
It always deploy library in release (default) and debug. The 
compiler has option to compile/use Debug Library (Use Debug 
DCUs). So for D, just deploy ...lib\debug and add compiler option 
to use it

Happy coding!
Oct 18 2021
prev sibling next sibling parent reply Paul Backus <snarwin gmail.com> writes:
On Monday, 18 October 2021 at 13:04:47 UTC, Steven Schveighoffer 
wrote:

 Would it be a reasonable thing to change contracts to be called 
 by the caller instead of the callee? Is that something that 
 could make its way into D, such that the input checking of 
 functions that are compiled for release still can run when you 
 compile your code in non-release mode?


My radical idea (which I also brought up on Discord) is that we 
should enable *all* contract checks and asserts in release mode 
by default, and tell programmers to use `debug assert(...)` if 
they want a particular check to be removed in release builds.

Of course, if you really wanted to disable contract checks or 
assertions at build time, you would still be able to do so with 
`-check=assert=off` and `-check=[in|out|invariant]=off`. But 
requiring an explicit opt-in would make this much less of a 
footgun than it currently is.
Oct 18 2021
parent reply rikki cattermole <rikki cattermole.co.nz> writes:
On 19/10/2021 12:17 PM, Paul Backus wrote:

 My radical idea (which I also brought up on Discord) is that we should 
 enable *all* contract checks and asserts in release mode by default, and 
 tell programmers to use `debug assert(...)` if they want a particular 
 check to be removed in release builds.


That isn't radical at all.

"I believe that range checking should be used far more often than it 
currently is, but not everywhere. On the other hand I am really assuming 
infallible hardware when I say this; surely I wouldn't want to remove 
the parity check mechanism from the hardware, even under a hypothetical 
assumption that it was slowing down the computation. Additional memory 
protection is necessary to prevent my program from harming someone 
else's, and theirs from clobbering mine. My arguments are directed 
towards compiled-in tests, not towards the hardware mechanisms which are 
really needed to ensure reliability." - 1974 Structured Programming with 
go to Statements - Donald Knuth.

In context having the default be sanity checks turned on, is probably 
the right way to go.
Oct 18 2021
next sibling parent reply jfondren <julian.fondren gmail.com> writes:
On Monday, 18 October 2021 at 23:29:09 UTC, rikki cattermole 
wrote:

 On 19/10/2021 12:17 PM, Paul Backus wrote:

 My radical idea (which I also brought up on Discord) is that 
 we should enable *all* contract checks and asserts in release 
 mode by default, and tell programmers to use `debug 
 assert(...)` if they want a particular check to be removed in 
 release builds.


 That isn't radical at all.


Nim has the same division, with the opposite default, in `assert` 
(erased in release builds) and `doAssert` (retained in release 
builds). D can simulate a bunch of different kinds of asserts by 
abusing dmd flags and different places that asserts can appear in 
- function bodies, contract bodies, unittests, and debug 
statements, version blocks, but what D's missing is the 
*convention* of using these asserts differently.
Oct 18 2021
parent Paul Backus <snarwin gmail.com> writes:
On Monday, 18 October 2021 at 23:37:51 UTC, jfondren wrote:

 Nim has the same division, with the opposite default, in 
 `assert` (erased in release builds) and `doAssert` (retained in 
 release builds). D can simulate a bunch of different kinds of 
 asserts by abusing dmd flags and different places that asserts 
 can appear in - function bodies, contract bodies, unittests, 
 and debug statements, version blocks, but what D's missing is 
 the *convention* of using these asserts differently.


My guess is that if we changed the behavior of `-release`, D 
programmers would learn the new convention pretty quickly.

Even if they didn't, though, the change is fail-safe--it will not 
break any code that isn't already broken in debug builds. So as 
compiler changes go, this one is relatively low-risk.
Oct 18 2021
prev sibling parent reply Tejas <notrealemail gmail.com> writes:
On Monday, 18 October 2021 at 23:29:09 UTC, rikki cattermole 
wrote:

 On 19/10/2021 12:17 PM, Paul Backus wrote:

 [...]


 That isn't radical at all.

 "I believe that range checking should be used far more often 
 than it currently is, but not everywhere. On the other hand I 
 am really assuming infallible hardware when I say this; surely 
 I wouldn't want to remove the parity check mechanism from the 
 hardware, even under a hypothetical assumption that it was 
 slowing down the computation. Additional memory protection is 
 necessary to prevent my program from harming someone else's, 
 and theirs from clobbering mine. My arguments are directed 
 towards compiled-in tests, not towards the hardware mechanisms 
 which are really needed to ensure reliability." - 1974 
 Structured Programming with go to Statements - Donald Knuth.

 In context having the default be sanity checks turned on, is 
 probably the right way to go.



I thought `enforce` was our `-release` mode error checking 
mechanism?

Why don't we encourage that instead of trying to change 
things(which will result in a battle of inertia).
Oct 18 2021
next sibling parent rikki cattermole <rikki cattermole.co.nz> writes:
On 19/10/2021 3:02 PM, Tejas wrote:

 Why don't we encourage that instead of trying to change things(which 
 will result in a battle of inertia).


Right now code that could fail loudly, is failing silently.

There is no inertia to worry about.
Oct 18 2021
prev sibling parent reply Paul Backus <snarwin gmail.com> writes:
On Tuesday, 19 October 2021 at 02:02:31 UTC, Tejas wrote:

 I thought `enforce` was our `-release` mode error checking 
 mechanism?

 Why don't we encourage that instead of trying to change 
 things(which will result in a battle of inertia).


`enforce` and `assert` are not interchangeable. Walter discusses 
the difference at length in this thread from 2014:

https://forum.dlang.org/thread/m07gf1$18jl$1 digitalmars.com

TL;DR: exceptions are for recoverable failures; assertions are 
for non-recoverable failures.
Oct 18 2021
parent Tejas <notrealemail gmail.com> writes:
On Tuesday, 19 October 2021 at 03:09:16 UTC, Paul Backus wrote:

 On Tuesday, 19 October 2021 at 02:02:31 UTC, Tejas wrote:

 I thought `enforce` was our `-release` mode error checking 
 mechanism?

 Why don't we encourage that instead of trying to change 
 things(which will result in a battle of inertia).


 `enforce` and `assert` are not interchangeable. Walter 
 discusses the difference at length in this thread from 2014:

 https://forum.dlang.org/thread/m07gf1$18jl$1 digitalmars.com

 TL;DR: exceptions are for recoverable failures; assertions are 
 for non-recoverable failures.


I was thinking more about exceptions that should not be 
caught(the class `Error` inheriting from `object.throwable`).

Stuff like [this](https://dlang.org/phobos/object.html#.Error)
Oct 18 2021
prev sibling parent reply bauss <jj_1337 live.dk> writes:
On Monday, 18 October 2021 at 13:04:47 UTC, Steven Schveighoffer 
wrote:

 I wrote about this in a reply to a learn thread, but realized I 
 should really post it here on its own.

 A user posted seemingly innocuous code that caused a 
 segmentation fault 
 [here](https://forum.dlang.org/post/nkfejgkrzntybqetkvkj forum.dlang.org), and
also asked about it on Discord. We worked through trying to figure out the
issue, and it turns out it was a memory corruption (the seg fault was deep
inside the garbage collector).

 But not one that I would have expected. It was an out-of-bounds 
 access passed into a 
 [std.bitmanip.BitArray](https://dlang.org/phobos/std_bitmanip.html#BitArray),
which corrupted GC metadata, and caused a later allocation to fail. Like any
reasonable array type, `BitArray` uses asserts to enforce bounds. And he did
NOT turn off bounds checks.

 So what happened? Phobos is compiled in release mode, even if 
 *your code is not*. So bounds checks are disabled based on the 
 type.

 If `BitArray` was a template, it might have bounds checks 
 enabled. But maybe not, if the compiler detected it was already 
 instantiated inside Phobos and decided it didn't need to emit 
 the code for it. This kind of Russian Roulette seems prone to 
 cause hours of debugging when it could give you an answer 
 instantly.

 How do we fix such a thing? The easiest thing I can think of is 
 to ship an assert-enabled version of Phobos, and use that when 
 requested. This at least allows a quick mechanism to test your 
 code with bounds checks (and other asserts) enabled. LDC 
 actually already has this, see 
 https://d.godbolt.org/z/feETE8W78 (courtesy of Max Haughton)

 But this feels awkward. If you build your code with bounds 
 checks, you would like the libraries you use to have them also. 
 And a compiler switch isn't going to help you when you are 
 using other libs.

 Now consider that `BitArray`, being quite old, uses contracts 
 to enforce its bounds checks. However, D runs the contracts 
 inside the function itself, instead of at the call site. But 
 this seems wrong to me, as the contracts are checking the 
 *caller's* code, not the *callee's*.

 Would it be a reasonable thing to change contracts to be called 
 by the caller instead of the callee? Is that something that 
 could make its way into D, such that the input checking of 
 functions that are compiled for release still can run when you 
 compile your code in non-release mode?

 -Steve


In theory, this issue means that any code using Phobos is open to 
buffer overflows :)

Which is a huge problem for D, considering it has marketed itself 
as a language where it's (in theory again) impossible to have 
such problems.
Oct 19 2021
parent Imperatorn <johan_forsberg_86 hotmail.com> writes:
On Tuesday, 19 October 2021 at 11:46:25 UTC, bauss wrote:

 On Monday, 18 October 2021 at 13:04:47 UTC, Steven 
 Schveighoffer wrote:

 [...]


 In theory, this issue means that any code using Phobos is open 
 to buffer overflows :)

 Which is a huge problem for D, considering it has marketed 
 itself as a language where it's (in theory again) impossible to 
 have such problems.


Yeah, we should take this seriously
Oct 19 2021