-
-
Notifications
You must be signed in to change notification settings - Fork 608
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 23535 - extend pragma(crt_constructor) with semantics that … #14669
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for your pull request, @WalterBright! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. 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#14669" |
7085de7
to
af9666c
Compare
|
Fixing this is necessary to make progress on #14665 |
compiler/src/dmd/dsymbolsem.d
Outdated
| @@ -3050,7 +3050,7 @@ private extern(C++) final class DsymbolSemanticVisitor : Visitor | |||
| if (sc.flags & SCOPE.compile) | |||
| funcdecl.isCompileTimeOnly = true; // don't emit code for this function | |||
|
|
|||
| funcdecl._linkage = sc.linkage; | |||
| funcdecl._linkage = (funcdecl.isCrtCtor || funcdecl.isCrtDtor) ? LINK.c : sc.linkage; | |||
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.
extern(C) is problematic wrt. name clashes due to no mangling. It's the main reason LDC hasn't rejected extern(D) CRT c/dtors - they are ABI-compatible anyway.
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.
I know, but them being ABI-compatible is happenstance. I figure that to be pedantically correct, they should use C linkage. I can add a "best practice" note in the documentation that adding a module name prefix to it would be good.
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.
It's happenstance for Win32 only, otherwise the D ABI spec defines it as C-compatible. IMO, requiring the user to unique the names is clearly a PITA and in no way justifies this pedantic-ness about the linkage.
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.
Some evidence:
dmd/druntime/src/core/memory.d
Lines 186 to 206 in ad31ad8
| // The reason for this elaborated way of declaring a function is: | |
| // | |
| // * `pragma(crt_constructor)` is used to declare a constructor that is called by | |
| // the C runtime, before C main. This allows the `pageSize` value to be used | |
| // during initialization of the D runtime. This also avoids any issues with | |
| // static module constructors and circular references. | |
| // | |
| // * `pragma(mangle)` is used because `pragma(crt_constructor)` requires a | |
| // function with C linkage. To avoid any name conflict with other C symbols, | |
| // standard D mangling is used. | |
| // | |
| // * The extra function declaration, without the body, is to be able to get the | |
| // D mangling of the function without the need to hardcode the value. | |
| // | |
| // * The extern function declaration also has the side effect of making it | |
| // impossible to manually call the function with standard syntax. This is to | |
| // make it more difficult to call the function again, manually. | |
| private void initialize(); | |
| pragma(crt_constructor) | |
| pragma(mangle, `_D` ~ initialize.mangleof) | |
| private extern (C) void initialize() @system |
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.
A technique I usually use:
alias SigHandlerT = extern(C) void function (int sig) nothrow;
/// Returns a signal handler
/// This routine is there solely to ensure the function has a mangled name,
/// and doesn't accidentally conflict with other code.
private SigHandlerT getSignalHandler () @safe pure nothrow @nogc
{
extern(C) void signalHandler (int signal) nothrow
{ }
return &signalHandler;
}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.
I wouldn't expect this to get D mangling, but it's what this PR would do currently.
There are other existing cases where this happens, you can see it in the function where I made this change. If crt_constructors are used as intended, one would never see the name mangling.
and also clearly visible from Walter's pragma(mangle) stuff to keep it backwards-compatible.
What was done in the gc code in this PR was the original code was clearly the wrong way to do things, it just happened to work. To minimize the diffs in this PR, I just made it work. The better solution is to put the constructors in their own module, so the name monkey business would not be necessary, it would just work.
@kinke it did not break existing code with the C mangling it did before, but you asked to change it. What else can I do? I cannot fix the ModuleInfo bugs many are complaining about without changing something in the way crt_constructors work. What do you suggest?
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.
but you asked to change it.
Nope. I'm just advocating for leaving the linkage alone - as LDC did from the start, or for a very long time at least. No ModuleInfo bugs require an extern(C) linkage for CRT c/dtors.
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.
I.e., keeping up https://dlang.org/changelog/2.101.0.html#dmd.crt_constructor_signature.
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.
Leaving it alone means it has D linkage, which is assumed to be the same as C linkage. It is not, even if it is the same for some platforms. It is a hack to rely on them being the same.
Thanks for the reference. This is recent, and documenting what is going on is not fixing it. C linkage is required because the C runtime library expects it. We cannot define it away.
No ModuleInfo bugs require an extern(C) linkage for CRT c/dtors.
Right. It's the C runtime library that requires it. Fixing the ModuleInfo problem requires fixing the problems with crt_constructors so they are specified properly.
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.
Fixing the ModuleInfo problem requires fixing the problems with crt_constructors so they are specified properly.
Oh, I see where you are going with this, it's for the patch function!
EDIT: on second thought, maybe I'm thinking too far ahead oops lol
|
See also #13751 |
3f7f942
to
86c18d6
Compare
|
Since D's static constructors already have the desirable semantics, can we make them use the same ABI implementation as CRT constructors, if they have the |
|
They are and always will be ABI-compatible ( |
C linkage is defined by the associated C compiler. D linkage is defined by us. C runtime library constructors are defined by C. Not D. |
|
I know all that, and I know that the D ABI spec is C-compatible (and DMD breaks it wrt. params order, but doesn't matter here for |
|
The point of having a D ABI is so we are not dependent on the vagaries of the C one, and we can customize it as we like. Having consistency in the specification is also important. Special corners make the language harder to document and understand. We can do special cases, but they need a really strong reason, like there being no other way. That isn't the case here. |
|
I'll just be repeating myself, so one last emphasis: you're being pedantic here and trying to solve an issue that will never ever be a problem (and a trivial test can make sure it doesn't break). While namespace pollution is a very real issue and requires ugly workarounds such as the druntime one I linked above (incl. a long explanatory comment). At work, we have a project consisting of more than 150 D libs, and numerous C++ and C libs. Symbol collisions is a very real threat. I'll definitely keep accepting |
Yes, I am. D shouldn't be a collection of hacks if we can help it.
And this PR solves it. |
An |
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.
There's no test for marking a crt constructor @safe or calling it from @safe code.
|
Otherwise, I'm fine with going through with the |
|
Well, I think the current proposal goes against the principle of least surprise - the linkage being overwritten to The end result is the same though (no real symbol conflict potential), and that's what I care most about, so I could reluctantly live with the current proposal. ;) |
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.
I'll just be repeating myself, so one last emphasis: you're being pedantic here and trying to solve an issue that will never ever be a problem (and a trivial test can make sure it doesn't break). While namespace pollution is a very real issue and requires ugly workarounds such as the druntime one I linked above (incl. a long explanatory comment). At work, we have a project consisting of more than 150 D libs, and numerous C++ and C libs. Symbol collisions is a very real threat.
I'll definitely keep accepting
extern(D)for LDC. One such CRT ctor is used for DSO registration and so well tested.
Agreed, forcing extern(C) linkage makes no sense. It's a void function without any parameters (and indeed, called without any parameters too).
Except that the C calls to it expect C linkage and this change doesn't hurt anything. |
Both extern(C) and extern(D) get external linkage, even then I don't see it mattering so long as its address is callable through a pointer (so even functions with internal linkage would work with |
|
Expecting a C ABI but being provided a D ABI is just poor design, even if it happens to work. And there's no good reason to do this. The language specification shouldn't be a barrage of funky corner cases and special exceptions if we can avoid it. We can avoid it in this case. |
|
After reading with a clear head, I think there are two ways forward here:
Will this strategy satisfy everyone? @WalterBright @kinke @ibuclaw |
86c18d6
to
0e15d3a
Compare
|
I removed the hackish nonsense in the gc constructors so that the mangling of the |
9baff4c
to
810169c
Compare
810169c
to
0cad3c8
Compare
…static constructors have