Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Fix issue 22124 - Corrupted closure with -dip1000 #3520

Closed
wants to merge 3 commits into from

Conversation

dkorpel
Copy link
Contributor

@dkorpel dkorpel commented Jul 15, 2021

Depends on dlang/dmd#12880

This is really hard to test. The original test case is timing and stack-layout sensitive, so it can pass by luck. I thought maybe !__traits(compiles, (int i) @nogc => new Thread({i = 3;})) but sinceThread is a class, that will fail anyway. I could do scope t = new Thread(), but then it's technically valid to be @nogc, so that's why I call __ctor directly.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @dkorpel! 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
22124 critical Corrupted closure when compiling with -preview=dip1000

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 "stable + druntime#3520"

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Jul 15, 2021
@dkorpel
Copy link
Contributor Author

dkorpel commented Jul 15, 2021

Okay, so it turns out that you can't assign scope variables to class fields, because scope on a class applies to the pointer to the class instance, not the members. I can't think of another workaround for now, the issue has to wait until dlang/dmd#12010 gets merged.

@dkorpel dkorpel closed this Jul 15, 2021
@Geod24
Copy link
Member

Geod24 commented Jul 16, 2021

I'm not sure why return scope would be legitimate here ? Surely we want the compiler to allocate a closure for what we pass to Thread ?
Note that it's broken at a much more fundamental level, because the delegate should have its context be shared, but attributed contexts aren't even supported yet.

@dkorpel
Copy link
Contributor Author

dkorpel commented Jul 16, 2021

I'm not sure why return scope would be legitimate here ? Surely we want the compiler to allocate a closure for what we pass to Thread ?

Yes, and currently it doesn't because the constructor is pure so the parameter is assumed scope. return scope could be legitimate because then the compiler will allocate a closure when passed a delegate literal, and when a scope delegate is passed, it would mark the Thread variable scope so you can't call start on it. That could work if Thread were a struct, but it's a class, so you can't have scope members.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Fix Include reference to corresponding bugzilla issue
Projects
None yet
3 participants