fix Issue 21821 - Optimizer assumes immutables do not change, but the…

[WalterBright]

…y can in @system code

Another problem unearthed by https://github.com/dlang/dmd/pull/12409

The fix is to tell the backend if a function is @safe

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	21821	normal	Optimizer assumes immutables do not change, but they can in @system code

Testing this PR locally

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

dub run digger -- build "master + dmd#12424"

[Geod24]

As replied on the issue, changing immutable data is specified as UB currently, and IMHO should stay that way. Having @system bypass a scope checks is acceptable, but having it bypass type ctors would be a step back from the current state.

[WalterBright]

buildkite fails with heisenbug:

Command Exit Status | -1 (Agent Lost)

@Geod24 half of my PRs don't make it through the test suite with random failures in the test suite. The fix, of course, is to recognize that the failure is a networking problem and automatically restart it. Can we get some of these addressed?

More and more tests keep getting added to the test suite, and soon it will become impossible to get anything through it without failing somewhere.

[Geod24]

@Geod24 half of my PRs don't make it through the test suite with random failures in the test suite. The fix, of course, is to recognize that the failure is a networking problem and automatically restart it. Can we get some of these addressed?

This should help: https://github.com/dlang/ci/pull/444

[WalterBright]

@Geod24 if you'll fix core.lifetime, I'll be happy to enable this again. Though I wonder what else may break that's casting away immutability.

[Geod24]

Casting away immutability is fine, it's mutating previously-immutable data that isn't. I'll take a look at core.lifetime.

[WalterBright]

@Geod24 thanks! I'm currently busy tracking down another optimizer failure exposed by https://github.com/dlang/dmd/pull/12409

[PetarKirov]

I'd say that most (all?) of the valid use cases of emplace involve in-place initializing some piece of fresh memory (e.g. std.experimental.allocator : make). So it's going from mutable void[] to immutable T. The part about accepting ref T target is an attempt at providing a minor convenience and safety vs void[]. Accepting ref immutable(T) is plain wrong, IMO.

@Geod24 I think the signature of copyEmplace should be changed to:

ref inout(S) copyEmplacex(
    ref inout(S) source,
    return ref S target)
{
    // ...
    return target; // cast to `inout(S)` if necessary.
}

[Geod24]

The code was introduced by https://github.com/dlang/druntime/commit/0868bef718432b4deea70c40a81a19143308bb09 so CC @kinke . The commit does mention that this is needed to allow calling differently-qualified copy ctor (e.g. immutable copy ctor). Is that really something users of emplace need ?

[kinke]

Is that really something users of emplace need ?

Apparently so: https://github.com/dlang/phobos/pull/7655 / https://github.com/dlang/phobos/pull/7661

[WalterBright]

@kinke I see the code there that does the immutable, but it's missing the why code needs to run a postblit on an immutable object. Some design decision there looks terribly wrong.

[WalterBright]

Ready to go

[ibuclaw]

Meh... I don't mind a small performance regression in DMD's backend, but the real issue here is the UB from the code in druntime modifying immutable data.

Immutable is pretty much ignored anyway as it's initialized-once rather than read-only data.

I have no idea whether possible, but maybe the OS could assist, ie: mprotect() data after first write, so it'd cause a fault if written to again.

[anon17]

Yep, receiveOnly has wrong design, the ret tuple should be mutable storage and casted to Tuple!T on return.

[anon17]

A simple hack, closure:

    Tuple!(T) delegate() deret;

    thisInfo.ident.mbox.get((T val) {
        deret = (){ return val; };
    }...);
    static if (T.length == 1)
        return deret()[0];
    else
        return deret();

[ibuclaw]

@WalterBright @RazvanN7 How do we know this fix works? The test is never ran by the testsuite!