New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix Issue 21821 - Optimizer assumes immutables do not change, but the… #12424
Conversation
Thanks for your pull request, @WalterBright! Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#12424" |
112c93d
to
ad7fa5f
Compare
As replied on the issue, changing |
buildkite fails with heisenbug:
@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. |
This should help: dlang/ci#444 |
@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. |
Casting away immutability is fine, it's mutating previously-immutable data that isn't. I'll take a look at |
I'd say that most (all?) of the valid use cases of @Geod24 I think the signature of
|
The code was introduced by dlang/druntime@0868bef 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 |
Apparently so: dlang/phobos#7655 / dlang/phobos#7661 |
@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. |
0fc83d3
to
15a0e2b
Compare
15a0e2b
to
ba1f1c3
Compare
Ready to go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Yep, receiveOnly has wrong design, the |
A simple hack, closure:
|
@WalterBright @RazvanN7 How do we know this fix works? The test is never ran by the testsuite! |
…y can in @System code
Another problem unearthed by #12409
The fix is to tell the backend if a function is @safe