Fix Issue 20375 - RefCounted does not work with checkaction-context

[MoonlightSentinel]

The hidden temporary was simply blitted (without calling a constructor/postblit) but destructed at the end of the expression.This caused RefCounted to deallocate the payload a second time.

[dlang-bot]

Thanks for your pull request and interest in making D better, @MoonlightSentinel! 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

Auto-close	Bugzilla	Severity	Description
✓	20375	normal	std.typecons.RefCounted does not work with checkaction-context

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 + dmd#10550"

[MoonlightSentinel]

The Druntime failure is caused by a dubious lifetime violation in an unittest in core.atomic.

static struct S { int* _impl; }
shared S s;
static assert(is(typeof(atomicLoad(s)) : shared S));
static assert(is(typeof(atomicLoad(s)._impl) == shared(int)*));
auto u = atomicLoad(s);
assert(u._impl is null);

src\core\atomic.d(1146): Error: address of variable `u` assigned to `__assertOp666` with longer lifetime

Shouldn' t __assertOp666 = u be valid as the generated temporary (__assertOp666) is scope and declared after u?

[MoonlightSentinel]

Changed to approach to elide the destructor call (as this could execute arbitrary user code and make programs behave differently with -checkaction=context).

CC @PetarKirov, @Geod24

[RazvanN7]

Looks good to me

[MoonlightSentinel]

@RazvanN7 Switched the approach back to marking the temporary as ref or rvalue because it seems to be the correct way instead of an invisible copy.

The main problem in core.atomic seems to be a bug in DIP1000 because it concludes that the temporary outlives the assert expression (even after marking it as exptemp)

EDIT: exptemp isn't considered in escape.d

[SSoulaimane]

Thanks for the fix.

I did hit the same issue with dip1000 when using ref temporaries in this PR #10539. The problem is that escape analysis is not implemented for this type of ref variables, I had to fill it in b3a0d96.
If I can find a test case for it outside of that PR I can extract it to be reviewed and merged separately.

[SSoulaimane]

objection to the 72h no objection -> merge tag since the CI failure is persistent.

[SSoulaimane]

BTW is issue 20375 still present? I tested on dmd master and dmd-nightly on run.dlang.io and it seems it works now with -checkaction=context.

[MoonlightSentinel]

Thanks for the fix.

I did hit the same issue with dip1000 when using ref temporaries in this PR #10539. The problem is that escape analysis is not implemented for this type of ref variables, I had to fill it in b3a0d96.
If I can find a test case for it outside of that PR I can extract it to be reviewed and merged separately.

Maybe a unittest constructing an explicit AST could work

BTW is issue 20375 still present? I tested on dmd master and dmd-nightly on run.dlang.io and it seems it works now with -checkaction=context.

import std.stdio;

struct RefCounted
{
    static int instances;
    int equalsCalls;
    
    this(bool)
    {
        instances++;
        writefln!"this(bool): %s"(instances);
    }
    
    this(this)
    {
        instances++;
        writefln!"this(this): %s"(instances);
    }

    ~this()
    {
        instances--;
        writefln!"   ~this(): %s"(instances);
    }

    bool opEquals(RefCounted other)
    {
        equalsCalls++;
        return true;
    }
}

void main() {
    {
        auto a = RefCounted(true);
        assert(a == a);
        assert(a.equalsCalls == 1);
    }

    assert(RefCounted.instances == 0);
}

this(bool): 1
this(this): 2
   ~this(): 1
   ~this(): 0

Seems like master neither constructs nor destructs eliminates the hidden temporary (and as such the bug does not manifest - even though it is present)

EDIT:
Seems like the test case is now insufficient because testing phobos with checkaction=context (#7252) still yields the same failure (RefCounted in Array gets destroyed multiple times).

E.g. https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=3851526&isPull=true

[MoonlightSentinel]

Extended the test case to reliably reproduce the failure in RefCounted

[WalterBright]

I suspect the problem is with the lazy parameter. The escape checking code doesn't deal well with lazy. But I also checked core.atomic, and there is no use of lazy there.

Why does the test case use lazy ?

[MoonlightSentinel]

Theres no lazy in core.atomic yet it still fails to build with the aforementioned error. So lazy is not to blame, its just allows the test to omit redundant try-catch Blocks.

Applying @SSoulaimane patch resolves this issue

[MoonlightSentinel]

Included @SSoulaimane patch for demontration purposes

[thewilsonator]

@MoonlightSentinel please give this a force push, there was a network error in one of the testers.

[MoonlightSentinel]

I'm not sure if this should be merged as it includes @SSoulaimane changes to the -dip1000 implementation which needs more tests.

@WalterBright should probably have a look too.

[MoonlightSentinel]

Whats wrong with Windows complaining about unittest not found?

[rainers]

Whats wrong with Windows complaining about unittest not found?

That's DM makes' way of reporting crashes when executing unittest.exe ;-/

The crash happens in this line:
https://github.com/dlang/druntime/blob/998fcbce0c55bc00c78a6485f30181c061fc08ba/src/object.d#L212

AFAICT an unexpected _d_callfinalizer is called as cleanup in that statement.

[MoonlightSentinel]

Thanks. I guess this is a regression outside of this PR since it passes everything else (and Windows passed previously as well).

[rainers]

I compared the disassembly: the additional call to _d_callfinalizer is not there with master.

[rainers]

The additional calls are in the x64 build, too, but don't cause crashes (but probably still bad). Win32 crashes due to some mismatched calling conventions: no stack cleanup for a call to extern(C) _d_callfinalizer.

[MoonlightSentinel]

Strangely I failed to reproduce this locally but this seems to be a windows codegen issue anyway

[MoonlightSentinel]

Seems like the DIP 1000 issue got resolved by some other changes!

CI is green

[thewilsonator]

Whats the status of this? Is it good to go?

[MoonlightSentinel]

I think this is ready