Fix Issue 23639 - Casting to shared not allowed with -preview=nosharedaccess by RazvanN7 · Pull Request #14836 · dlang/dmd

RazvanN7 · 2023-01-18T13:47:37Z

Casting to shared should be allowed even if the result is returned, read or written because the variable has just been "promoted" to shared so it's not being actually shared with anyone. This is currently blocking the templatization of the __d_new_class runtime hook (cc @teodutu ).

dlang-bot · 2023-01-18T13:47:42Z

Thanks for your pull request and interest in making D better, @RazvanN7! 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
✓	23639	blocker	Casting to shared not allowed with -preview=nosharedaccess

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#14836"

dkorpel

Given it's still in preview, and in the absence of anything better, I think this is okay.
However, I think this will permit this:

shared string s;
string s2 = cast(shared) s;

So it might need more thought long term.

RazvanN7 · 2023-01-18T14:19:10Z

No it doesn't. If you run that code it gives: "direct access to shared s is not allowed, see core.atomic"

dkorpel · 2023-01-18T14:20:52Z

Oh, that's good

maxhaton · 2023-01-18T20:35:24Z

Isn't this a special case patching over an issue with the nosharedaccess check?

IIRC this same issue also breaks nosharedaccess atomics with ldc because the cast is badly analyzed and/or then "optimized" to strip out the shared (this is actually tested with a static assert somewhere in the test suite)

thewilsonator

Couple of thing I'd like clarified before this is merged.

thewilsonator · 2023-01-18T22:06:40Z

compiler/src/dmd/expressionsem.d

-            if (e.type.isShared())
+            // https://issues.dlang.org/show_bug.cgi?id=23639
+            // Should be able to cast(shared)
+            if (!e.isCastExp() && e.type.isShared())


what does this do to const shared?

If you cast to const shared it just performs the cast.

please augment the test case then.

Why? Before this patch the cast would not have been allowed, but now it is allowed. Note that the code would fail because you cannot convert a const(shared) to a shared.

thewilsonator · 2023-01-18T22:06:58Z

compiler/test/compilable/test23639.d

+{
+    void *p;
+    // assume p is allocated here
+    return cast(shared(T))p;


what is the @safety of this operation?

Error: cast from void* to shared(T) not allowed in safe code

The expected @safety level - @System.

and if typeof(p) == T?

The modified code has nothing to do with the safety checks. Casts to differently qualified types remain unsafe, that part is not modified.

-preview=nosharedaccess is all about safety. even if the end result is that the code must be system. Perhaps I was thinking things wrong, but it seems to me that the only reason why you would want to do this with -preview=nosharedaccess is because you have a unique reference to an object you then want to share. Its fine if that needs to be trusted, then please disregard this request for changes, but please do document this discussion in the test case.

Yes, a prime example of this is memory allocation. You allocate heap memory which is naturally shared among threads and you want to express that. What other way of casting the mallocd memory to shared is there?

RazvanN7 · 2023-01-19T11:52:03Z

Isn't this a special case patching over an issue with the nosharedaccess check?

Well, -preview=nosharedaccess does not allow free access to shared variables. The implementation simply checks if the type of an expression is shared and then errors out (provided, core.atomic is not used). This includes cast expressions to shared types. Note that casting a shared type to an unshared type works without this patch, so it is reasonable that doing the opposite works.

You call this patch special casing. Fine, but I have no idea what is the general problem other than "casting to shared is not allowed".

IIRC this same issue also breaks nosharedaccess atomics with ldc because the cast is badly analyzed and/or then "optimized" to strip out the shared (this is actually tested with a static assert somewhere in the test suite)

Without a test case, I have no idea what you are referring to.

…daccess

RazvanN7 · 2023-01-20T11:47:15Z

@thewilsonator I have added a fail_compilation test.

@maxhaton Ping. This is blocking progress on templating druntime hooks.

RazvanN7 added the Review:Blocking Other Work review and pulling should be a priority label Jan 18, 2023

dlang-bot added the Severity:Bug Fix label Jan 18, 2023

dkorpel approved these changes Jan 18, 2023

View reviewed changes

teodutu mentioned this pull request Jan 18, 2023

Translate _d_newclass to a template #14837

Merged

thewilsonator requested changes Jan 18, 2023

View reviewed changes

Fix Issue 23639 - Casting to shared not allowed with -preview=noshare…

a799c1b

…daccess

RazvanN7 force-pushed the Issue_23639 branch from a705367 to a799c1b Compare January 20, 2023 11:46

thewilsonator approved these changes Jan 20, 2023

View reviewed changes

RazvanN7 merged commit 67c137a into dlang:master Jan 20, 2023

Uh oh!

Conversation

RazvanN7 commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

dkorpel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 commented Jan 18, 2023

Uh oh!

dkorpel commented Jan 18, 2023

Uh oh!

maxhaton commented Jan 18, 2023

Uh oh!

thewilsonator left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RazvanN7 commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

RazvanN7 commented Jan 18, 2023 •

edited

Loading

dlang-bot commented Jan 18, 2023 •

edited

Loading

dkorpel left a comment •

edited

Loading

RazvanN7 Jan 19, 2023 •

edited

Loading

RazvanN7 commented Jan 19, 2023 •

edited

Loading

RazvanN7 commented Jan 20, 2023 •

edited

Loading