Skip to content
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 22031 - Allow modification of constants in crt_constructor's #13751

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MoonlightSentinel
Copy link
Contributor

@MoonlightSentinel MoonlightSentinel commented Mar 3, 2022

Allow functions marked as pragma(crt_constructor) to modify const /
immutable variables, which is currently restricted to module ctors.
This enables the use of lazily initialized global constants for
applications without druntime.

Such function may not be called at runtime because they might violate
the guarantees of const / immutable. Hence it is now an error to
call / take the address of a function marked as pragma(crt_constructor).

@dlang-bot
Copy link
Contributor

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
22031 minor crt_constructor functions can't initialize immutable/const variables

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

@MoonlightSentinel MoonlightSentinel marked this pull request as draft March 3, 2022 18:31
@pbackus
Copy link
Contributor

pbackus commented Mar 3, 2022

  1. Maybe it should be @system to call these functions at runtime rather than a hard error?
  2. Either way, taking the address of one of these functions should be subject to the same restrictions as calling it.

@maxhaton
Copy link
Member

maxhaton commented Mar 3, 2022

Should taking the address be banned too? Direct function calls aren't the only way to corrupt using a constructor like this.

@MoonlightSentinel MoonlightSentinel changed the title Fix 22031 - Allow modification of non-mutable in crt_[con|de]structor's Fix 22031 - Allow modification of constants in crt_constructor's Mar 3, 2022
@MoonlightSentinel
Copy link
Contributor Author

MoonlightSentinel commented Mar 3, 2022

  1. Maybe it should be @system to call these functions at runtime rather than a hard error?

Probably. As pointed out by @WebFreak001 on Discord:

  • additional calls to crt_constructor's might be safe
  • crt_constructor's must be called manually for targets without a C runtime

I've added a second commit that restricts the error to @safe code but I'll let others chime in.

  1. Either way, taking the address of one of these functions should be subject to the same restrictions as calling it.

Good catch

Allow functions marked as `pragma(crt_constructor)` to modify `const` /
`immutable` variables, which is currently restricted to module ctors.
This enables the use of lazily initialized global constants for
applications without druntime.

Such function may not be called at runtime because they might violate
the guarantees of `const` / `immutable`. Hence it is now an error to
call / take the address of a function marked as `pragma(crt_constructor)`.
…` code

Because there are valid reasons to explicitly call those functions,
e.g. when they are not automatically called without a C runtime.
@thewilsonator thewilsonator added the Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org label Mar 3, 2022
@thewilsonator
Copy link
Contributor

needs a spec PR at some point.

@WalterBright
Copy link
Member

I'm concerned about this. It has @safe code violating immutable guarantees. But currently even @System code is not allowed to violate immutability. This change allows any code, at any time, to change the state of those so-called immutable variables. This has adverse implications for compiler assumptions and optimizations. It sabotages any attempts to mechanically verify correctness of shared immutable code.

I understand that D's rigid interpretation of immutability leaves some people frustrated at not being able to write "logical const" code, and other techniques of mutating immutable data. But it's worth it.

If the programmer needs to mutate data, don't declare it immutable. immutable data should be confidently, reliably, consistently, absolutely, positively, immutable.

Since the user can declare the data to be mutable, and then initialized, I oppose this change.

BTW, the user can also do this today:

immutable int data;

@system void initialize()
{
    int* p = &cast()data;
    *p = 6;
}

`@system` already means dirty deeds done dirt cheap, we shouldn't hide it and pretend otherwise.

@MoonlightSentinel
Copy link
Contributor Author

It has @safe code violating immutable guarantees.

@safe code cannot call a crt_constructor unless it explicitly uses a @trusted wrapper.

But currently even @System code is not allowed to violate immutability.

Your proposed workaround disproves that statement - @system code most definitly can attempt to change an immutable value (success depends on whether the variable resides in ROM)

I understand that D's rigid interpretation of immutability leaves some people frustrated at not being able to write "logical const" code, and other techniques of mutating immutable data. But it's worth it.

That's why the initial implementation (see the first commit) explicitly disallows further calls to a crt_constructor to make it equivalent to a shared static this() - which currently may initialize immutable variables. I can discard the second commit if preferred.

@rikkimax
Copy link
Contributor

rikkimax commented Mar 4, 2022

If the programmer needs to mutate data, don't declare it immutable. immutable data should be confidently, reliably, consistently, absolutely, positively, immutable.

Initialization is not mutation, we make this distinction.

More importantly, pure function can access immutable globals, but it cannot access mutable ones, so making it mutable isn't the solution here.
That is how I rediscovered it and hence got brought up.

Also this works:

void main() {
    _sharedStaticCtor_L5_C1();
}

shared static this() {
    
}

@Geod24
Copy link
Member

Geod24 commented Mar 4, 2022

Also this works:

https://issues.dlang.org/show_bug.cgi?id=20607

I wish we just went with extern (C) shared static this instead of pragma(crt_constructor).
But at the time it would have been a silent change, see #10858
Perhaps we should reconsider now (EDIT: Or in two release, deprecation started v2.092.0) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants