Bug 22277 – removing strongly pure function calls is an incorrect optimization

Currently, with -O -release and strongly pure nothrow functions, DMD likes to remove calls to it because it determines it has 'no side effects'. This goes wrong with: - functions that halt the program with `assert(0)` - fake pure functions (memory allocation) ``` import core.stdc.stdio; void free(immutable void* mem) pure nothrow { debug printf("memory was freed\n"); } noreturn panic(string msg) pure nothrow { assert(0, msg); } void assertPositive(int x) pure nothrow { if (x < 0) { assert(0); } } void main() { free(null); assertPositive(-3); panic("panic!"); } ``` Compile with -O -release -debug and you'll see no output. Remove -release and it works as intended. I want to make GC.addRange pure so it can be used in custom allocators (issue 16982) but it's very important dmd doesn't remove calls to it.

@dkorpel created dlang/dmd pull request #13047 "Fix issue 22277 - removing strongly pure function calls is an incorrect optimization" fixing this issue: - fix issue 22277 - removing strongly pure function calls is an incorrect optimization https://github.com/dlang/dmd/pull/13047

I agree that this is a problem, but if this is fixed, why do we even have `pure` anymore? I think that the fix should be the other way around: asserts should be forbidden in pure functions (except if the function is @noreturn).

This has been discussed a number of times in the past. One proposal was that `void` pure functions must always be called - the compiler must assume since they're `void` and pure (a contradiction) then they do something surreptitious and must not be elided.

(In reply to Andrei Alexandrescu from comment #3) > This has been discussed a number of times in the past. One proposal was that > `void` pure functions must always be called - the compiler must assume since > they're `void` and pure (a contradiction) then they do something > surreptitious and must not be elided. Could you explain why void and pure is a contradiction?

(In reply to Max Samukha from comment #4) > (In reply to Andrei Alexandrescu from comment #3) > > This has been discussed a number of times in the past. One proposal was that > > `void` pure functions must always be called - the compiler must assume since > > they're `void` and pure (a contradiction) then they do something > > surreptitious and must not be elided. > > Could you explain why void and pure is a contradiction? `void` implies "all of my work is done via side effects" and pure implies "there is no side effect". In D we have the relaxation of weak purity that makes things less clear cut. In the case of `free` in the example the parameter is `immutable` which makes the function strongly pure; a strongly pure function that returns `void` has no way to effect anything within the definition of the language.

Fake pure functions are a special case here, but I agree that functions that halt the program with `assert(0)` is an issue here.

(In reply to Andrei Alexandrescu from comment #5) > > > > Could you explain why void and pure is a contradiction? > > `void` implies "all of my work is done via side effects" I am not sure this premise is true. A function returning 'void' may do no work at all or do work without observable side effects. I need to think more about this, thank you. > and pure implies > "there is no side effect". In D we have the relaxation of weak purity that > makes things less clear cut. In the case of `free` in the example the > parameter is `immutable` which makes the function strongly pure; a strongly > pure function that returns `void` has no way to effect anything within the > definition of the language. Yes, I am familiar with D's concepts of purity.

> I am not sure this premise is true. A function returning 'void' may do no work at all or do work without observable side effects. I need to think more about this, thank you. If the function is strongly pure that statement is correct. To do some work, then it needs to be impure, or in the case of D, weakly pure. An example is all the pure void functions that mess around with values of `this`.

(In reply to João Lourenço from comment #8) > > I am not sure this premise is true. A function returning 'void' may do no work at all or do work without observable side effects. I need to think more about this, thank you. > > If the function is strongly pure that statement is correct. To do some work, > then it needs to be impure, or in the case of D, weakly pure. An example is > all the pure void functions that mess around with values of `this`. It's not correct at least in one case: when the function doesn't do work at all. 'void foo() {}' is a strongly pure function returning the value of the unit type. And it's not even useless - it would've found its use in generic code if D's unit type wasn't broken in other respects. Whether encapsulated side effects qualify to be called "work", I don't know.

I think the compiler is allowed to elide a `free` call implemented like that. First, you're compiling in release mode, debug statements are supposed to be skipped even if the function is called. Second, `debug` statements are not considered to be side effects that require executing the function - that's what allows them to reside in `pure`!. They are required to be executed only if the surrounding scope is executed. But the `panic` and `assertPositive` functions ought to be executed. If you had consecutive calls to `assertPositive` with the same argument, the compiler is free to remove the tailing calls, but not the first one. And that ought to happen regardless of the return value type. Increasing severity, as this thing may silently add bugs to the compiled code.

(In reply to Ate Eskola from comment #10) > I think the compiler is allowed to elide a `free` call implemented like > that. Agreed, I used a `debug` statement for demonstration purposes based on the knowledge that the compiler doesn't inspect the function body in this case. A proper test case would hide the implementation: ``` void free(immutable void* mem) pure nothrow; ``` And then compile the implementation separately.

Currently, the compiler is allowed to elide such a `free` call if it can prove that `free` has been called before with a similar value in the pointed object. In that case, any potential crash or infinite loop would have happened in the prior call. This is directly not a problem. The compiler cannot judge whether two void pointers have similar values in the object, since it does not know the type of the object. The exception to that rule might be a double free, but that is undefined behaviour anyway. However, this one is still a problem: ------ pure immutable void* mallocSomething(int arg); auto a = mallocSomething(8), b = mallocSomething(8); free(a); free(b); ------ Even without the knowledge of what the pointers point to, the compiler is free to assume the objects are similar since they are returned by a strongly pure function with similar arguments. Thus it may skip the latter `free` call. I'm not sure how to handle this. I guess the short-term solution is to simply forbid a pure `free` with an immutable pointer.

Created attachment 1831 __mutable early DIP draft This is precisely what `__mutable` functions were supposed to solve. `__mutable` draft proposal attached for reference.

And Andrei killed that before the draft review? What was the rationale?

(In reply to Ate Eskola from comment #14) > And Andrei killed that before the draft review? What was the rationale? I was too busy to spend a lot of time on it, especially given that Andrei was not a huge fan of the details of my idea. I think they just wanted a very specialized solution that works for reference counting. Andrei and Razvan tried to finish the proposal, but they completely changed it in the process: https://github.com/RazvanN7/DIPs/blob/Mutable_Dip/DIPs/DIP1xxx-rn.md I then requested to be removed as co-author as I did not think it was a good approach, and they ultimately did not go ahead with that DIP. Regarding Andrei's proposal of never eliding pure `void` functions, I think that is a bad idea, e.g.: auto a = [1,2,3]; writeln(a); // a is dead now void foo(int[] x)pure{ x[]*=2; } foo(a); // this call can be elided, as a will never be read again I really think the only sane way to make progress on this is to specify precisely which rewrites are allowed for data and functions of each qualification, giving special treatment low-level functions that implement the high-level semantics, as in my draft. `__mutable` fields and functions solve a lot of issues at once. (E.g., they can be used to initialize `immutable` arrays in druntime, for sound lazy initialization of `immutable` data, or to store metadata such as reference counts.) Until such a solution is specced out, I think DMD should probably refrain from optimizing on qualifiers.

dlang/dmd pull request #13047 "Fix issue 22277 - removing strongly pure function calls is an incorrect optimization" was merged into master: - f5a75f5834a3ee9efa14c0e2a4da86a0c356b6df by dkorpel: fix issue 22277 - removing strongly pure function calls is an incorrect optimization https://github.com/dlang/dmd/pull/13047