Cleanup and fix implementation of _d_array{setlengthT,catnTX}Trace

[Vild]

This should be the final fix needed to be able to get dlang/dmd#10106 to pass all the unittest.

This code uses a pure bypass technique I found inside of dmd's source code so no more ugly hacky casts are needed for calling GC.stats() and accumulate.
I've also refactored the two previous *Trace functions into HookTraceImpl to make sure that all the Trace functions are implemented correctly and to make it easier to change in the future.
The HookTraceImpl also contains a if (...) assert(0); check to make sure that the compiler does not remove the call to accumulate, which is what is currently happening in dlang/dmd#10106.

[dlang-bot]

Thanks for your pull request and interest in making D better, @Vild! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2673"

[aG0aep6G]

That "pure bypass technique" looks like a compiler bug.

[Vild]

That "pure bypass technique" looks like a compiler bug.

I don't fully understand why this "technique" exist. It (or at least the comment) was added 5 years ago:
dlang/dmd@0acb94c#diff-31ec3d6838ade0264adec0944948507dR2399

[aG0aep6G]

I don't fully understand why this "technique" exist. It (or at least the comment) was added 5 years ago:
dlang/dmd@0acb94c#diff-31ec3d6838ade0264adec0944948507dR2399

I can't say that I understand what that comment is trying to say. I suspect it might just be wrong.

Anyway, I'm sure that we're looking at a compiler bug. Calling an impure function from a pure one can't be allowed (without a cast).

Filed: https://issues.dlang.org/show_bug.cgi?id=20047

[JinShil]

I don't fully understand why this "technique" exist.

That behavior was introduced in dlang/dmd#4344 as a fix to https://issues.dlang.org/show_bug.cgi?id=14039

[JinShil]

@WalterBright, you filed Issue 14039 and approved and merged the fix at dlang/dmd#4344. Can you please help reconcile this? Is the technique employed in this PR a bug or a legitimate feature?

[aG0aep6G]

Anyway, I'm sure that we're looking at a compiler bug. Calling an impure function from a pure one can't be allowed (without a cast).

Filed: https://issues.dlang.org/show_bug.cgi?id=20047

And a PR to fix it: dlang/dmd#10172

[jpf91]

@Vild if you use the function pointer / literal / cast bypass technique instead, does that trigger the optimizer problem? AFAIK this is the commonly used technique to bypass pure.

[RazvanN7]

Is there a possibility that if dlang/dmd#10172 is merged, then this PR might not be needed?

[JinShil]

Is there a possibility that if dlang/dmd#10172 is merged, then this PR might not be needed?

I believe that if dlang/dmd#10172 is merged, this PR will no longer work.

[Vild]

@Vild if you use the function pointer / literal / cast bypass technique instead, does that trigger the optimizer problem? AFAIK this is the commonly used technique to bypass pure.

I could move back to the pointer cast method, I just felt that it looked too hacky compared to this bypass method when I found it inside of dmd's code.

Is there a possibility that if dlang/dmd#10172 is merged, then this PR might not be needed?

I still need to apply the if (!accumulate(...)) assert(0); fix to not allow the optimizer to remove that call. So I just need to use casts instead of this bypass method.

[Vild]

I've changed it so it uses cast instead of that other method to fake purity.

[jpf91]

BTW, reading this I think there's may be a deeper issue whenever we have 'fake' pure code which internally calls impure code.

I guess the intent of such code is to not influence purity of the caller (i.e. completely optimizing away the caller would be fine), but the callee is not really pure, so it should not be removed on it's own. I wonder whether this can be expressed in any theoretically sound way, @tgehr any insights here?

It seems to me that the problem is that we lie to the compiler and simply tell it this function is pure. Maybe we should rather tell it Ignore purity checks for this call, but the function is not pure. Intuitively I'd say a

pure void foo()
{
    __traits(forcepure, bar());
}

Should work: if bar is not inlined, this would tell the compiler to keep the call, but ignore purity for purity calculation of the callee. With inlining, the compiler should be able to see the side effects anyway and should not optimize these away.

[jpf91]

I can see how your assert code has the same effect though: It marks the return value as used, as long as the call is not inlined. When it's inlined, the compiler may remove the assert, but it will see the impure parts of accumulatePure and keep these. This can probably also somehow solved like this: https://stackoverflow.com/questions/40122141/preventing-compiler-optimizations-while-benchmarking, but I'd prefer a compiler hook which should also work for void functions.

[Vild]

I've been thinking about introducing HookCallExp or a bool in CallExp that ignore the purity and somehow allowed the code to be inline, which is disallowed right now pragma(inline, false), whilst being able to be detected as the hook (HookCompoundStatement?) so the CTFE intercept code works.

[thewilsonator]

That sounds like a great idea.

[tgehr]

BTW, reading this I think there's may be a deeper issue whenever we have 'fake' pure code which internally calls impure code.

I guess the intent of such code is to not influence purity of the caller (i.e. completely optimizing away the caller would be fine), but the callee is not really pure, so it should not be removed on it's own. I wonder whether this can be expressed in any theoretically sound way, @tgehr any insights here?
...

@jpf91 This was what __mutable functions were for in the original __mutable proposal. Inlining for such functions is not so easy if you want to preserve constraints on evaluation order precisely.