Fix issue 20607 - Module constructors are visible as regular function #10834

Geod24 · 2020-02-25T04:08:46Z

Contain two small refactorings, and an actual fix.

Side note: __traits(allMembers) is quite inconsistent: It shows the ctors / dtors, some hidden symbols like _Dmain and the object import, but not __invariant1 (when used on Bar) or __unittest_L126_C1. And honestly I can see more reasons to call the unittest function directly than to call the [shared] static [~]this...

dlang-bot · 2020-02-25T04:08:48Z

Thanks for your pull request, @Geod24!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	20607	normal	[shared] static constructor & co can be called as regular function

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

Geod24 · 2020-02-25T04:21:12Z

@atilaneves : Looks like you guys rely on being able to do mixin(__traits(identifier, __traits(parent, {}))); to get the unittest. Why is that ? If you use __traits(parent, ...) you already get a reference to the symbol. I understand this is probably a synthetic use case, but interested to know if you require the ability to access the unittest using the identifier in unit-threaded (or anywhere else) ?

jacob-carlborg · 2020-02-25T08:33:34Z

src/dmd/func.d

@@ -3733,12 +3735,14 @@ extern (C++) class StaticDtorDeclaration : FuncDeclaration

    extern (D) this(const ref Loc loc, const ref Loc endloc, StorageClass stc)
    {
-        super(loc, endloc, Identifier.generateIdWithLoc("_staticDtor", loc), STC.static_ | stc, null);
+        this(loc, endloc, "_staticDtor", stc);


What happens now with multiple module constructors in the same file?

Nothing changes, because it forwards the call to the other ctor overload, which still uses generateIdWithLoc

atilaneves · 2020-02-25T10:57:31Z

test/compilable/issue18097.d

@@ -7,6 +7,6 @@ unittest // this first unittest is needed to trigger the bug

 unittest // second unittest
 {
-    auto a = &mixin(__traits(identifier, __traits(parent, {  })));
+    auto a = __traits(identifier, __traits(parent, {  }));


This isn't even remotely the same. Before it was a function pointer, now it's a string.

Should have added a comment that this was just to see if the other test passed. Indeed that change shouldn't go in as-is.

atilaneves · 2020-02-25T10:59:36Z

@atilaneves : Looks like you guys rely on being able to do mixin(__traits(identifier, __traits(parent, {}))); to get the unittest. Why is that ? If you use __traits(parent, ...) you already get a reference to the symbol. I understand this is probably a synthetic use case, but interested to know if you require the ability to access the unittest using the identifier in unit-threaded (or anywhere else) ?

The reason was presented in the issue itself:

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

I wrote the test so as to make sure I wouldn't break code that users had written that had previously worked when I change the unittest naming scheme. The test is now broken after this PR however.

Geod24 · 2020-02-25T15:56:35Z

I wrote the test so as to make sure I wouldn't break code that users had writtern that had previously worked when I change the unittest naming scheme. The test is now broken after this PR however.

True. I was more interested in the reason one would use &mixin(__traits(identifier, __traits(parent, {}))) over __traits(parent, {}) to get a function pointer to the unittest. I would imagine that this comes from a mixin that is used / injected into unittest ? My main concern is to know if there's a use case that can only be served by using the identifier.

@JohanEngelen : Can you tell us more about the reasoning for this ? Perhaps some code example ? And would you be able to replace your reliance on identifier.

In any case, it is clear that this would break code, so can't go in as is.
I will make it a deprecation, so that no code is broken. But I'd still like to know the rationale to be sure there's a replacement.

JohanEngelen · 2020-02-25T20:39:41Z

If &__traits(parent, {}) also works to get the address of the unittest function then I think we are happy. I'll need to test and report back later.

JohanEngelen · 2020-02-26T17:32:22Z

If &__traits(parent, {}) also works to get the address of the unittest function then I think we are happy. I'll need to test and report back later.

Looks like it worked; unittest functions are no longer used by identifier by Weka.

Geod24 · 2020-03-02T18:44:42Z

This is somehow taking longer than expected, as there's no really good way to do it.
Also found https://issues.dlang.org/show_bug.cgi?id=20626 in the process...

jacob-carlborg · 2020-03-02T19:51:20Z

FYI, I don’t think this is the only issue. I think basically all compiler generated symbols have the same issue. Another typical example is unnamed function parameters.

Geod24 · 2020-03-03T01:29:38Z

Yep, it's quite a big undertaking... So I decided to restrict this PR to disabling [shared] static {c,d}tors access, because they are the only ones I can think of that actually break the type system. Need to fix dmangle.d tho.

Geod24 · 2020-03-03T05:27:57Z

So apparently we have some module ctors in druntime which have extern(C) linkage, which means they end up with a very predictable and conflict-prone name in the executable:

$ nm /usr/local/lib/libphobos2.a | grep -E '__[sS].*tatic.*_L.*_C'
0000000000000784 S __sharedStaticDtor_L479_C1
00000000000002a8 S __sharedStaticDtor_L479_C1.eh
00000000000003c4 S __staticDtor_L376_C1
00000000000001a8 S __staticDtor_L376_C1.eh

Yeah that's pretty broken... I guess we should also deprecate this 🤦‍♂

Geod24 · 2020-03-03T08:00:50Z

Blocked on #10858

jacob-carlborg · 2020-03-03T08:08:11Z

If you want to be really thorough you should make the symbols have, what's called in LLVM (at least), private linkage [1]. But I'm not sure if that backend supports that kind of linkage.

[1] https://github.com/dlang/dmd/blob/master/docs/objective-c_abi.md#private-linkage

Geod24 · 2020-03-03T09:05:58Z

My original approach was to avoid putting things in symtab for Prot.Kind.none.
However visible symbols like unit tests and invariant need to be deprecated first. The only way to do this was to do a check in search when a symbol was found, which is a rather heavy price to pay because of how often this function is used.

But then I found that some places in the compiler use the identifier to access the symbol and so would trigger the depreciation message. Not to mention the other bug with typeof.

So I will probably go to them one by one, make sure nothing uses the identifier, deprecate accessing the identifier, and then make it not accessible. Whether or not we use Prot.Kind.none or anonymous symbol is up for discussion. At the moment those symbols semantic will forward to funcDeclarationSemantic which override the linkage with the scope's linkage, so the anonymous way seemed simpler and more robust.

Geod24 · 2020-03-06T03:21:33Z

Alright, this fails because without an identifier, duplicate manglings are generated for childs of [shared] static [~]this. Obviously not what we want.

This is luckily very obvious, because the compiler generates a gate variable for those symbols when they are inside a template, to prevent them being called multiple time for the same type (most of the time)

Any way I have attempted to fix it, I either found bugs, special cases, or just bloat (hello static foreach). So reverted to the old symtab approach.

This breaks the type system pretty easily. If the user *really* want to do something funky, they can still declare those as `extern(C)` and there isn't much we can (or will) do about it. But having those functions available in the symtab means they show up in `impHint` and can be called by accident (e.g. a bug in code generation that use `__allMembers`). There are many other instances in DMD where this happens, however we currently rely on magic identifiers too much to fix everything. The test case contains a few other instances which we'll hopefully gradually fix.

MoonlightSentinel · 2020-07-02T11:44:19Z

This would fix the windows debug info generation:

diff --git a/src/dmd/tocvdebug.d b/src/dmd/tocvdebug.d
index 57aaca5e0..b9aea9622 100644
--- a/src/dmd/tocvdebug.d
+++ b/src/dmd/tocvdebug.d
@@ -73,6 +73,7 @@ uint PROTtoATTR(Prot.Kind prot) pure nothrow @safe @nogc
 
     final switch (prot)
     {
+        case Prot.Kind.none: // No explicit mapping, treat as private
         case Prot.Kind.private_:       attribute = 1;  break;
         case Prot.Kind.package_:       attribute = 2;  break;
         case Prot.Kind.protected_:     attribute = 2;  break;
@@ -80,7 +81,6 @@ uint PROTtoATTR(Prot.Kind prot) pure nothrow @safe @nogc
         case Prot.Kind.export_:        attribute = 3;  break;
 
         case Prot.Kind.undefined:
-        case Prot.Kind.none:
             //printf("prot = %d\n", prot);
             assert(0);
     }

Geod24 · 2020-07-02T11:48:27Z

Ah thanks for the info! I need to put some time into this. I tried a bit last week, but as you can see from the history of comments, the issue is non-trivial... Starting with something simpler (like _value in out contracts) might be the best path at this point.

CyberShadow

As mentioned in the issue, I think this PR is moving in the wrong direction. Introspection in D and access to compiler/runtime internals is one of D's strengths as a system programming language with strong metaprogramming. Instead, it would be better to fix the problem and only the problem, without introducing breaking changes that affect the ecosystem:

Ensure that the type system breaking functionality is not accessible is @safe code
Explicitly label non-function module members as not functions

Removing them from the allMembers list is the wrong thing to do. They are members like any other.

dlang-bot added the Bug Fix label Feb 25, 2020

Geod24 mentioned this pull request Feb 25, 2020

Add pageSize and defaultPageSize dlang/druntime#2952

Merged

Geod24 force-pushed the ctor-visibility branch from b75a8a7 to 824a45d Compare February 25, 2020 04:53

thewilsonator approved these changes Feb 25, 2020

View reviewed changes

jacob-carlborg reviewed Feb 25, 2020

View reviewed changes

Geod24 requested review from WalterBright and atilaneves February 25, 2020 09:33

atilaneves reviewed Feb 25, 2020

View reviewed changes

atilaneves added the Needs Approval label Feb 25, 2020

Geod24 force-pushed the ctor-visibility branch 2 times, most recently from 809f7c4 to 1289ff1 Compare March 2, 2020 19:23

Geod24 force-pushed the ctor-visibility branch from 1289ff1 to d817e15 Compare March 3, 2020 04:25

Geod24 force-pushed the ctor-visibility branch from d817e15 to 509541d Compare March 5, 2020 04:20

dlang-bot added the Enhancement label Mar 5, 2020

Geod24 force-pushed the ctor-visibility branch from 509541d to 58d2454 Compare March 5, 2020 05:24

Geod24 removed Bug Fix Enhancement labels Mar 5, 2020

Geod24 added the Bug Fix label Mar 5, 2020

Geod24 force-pushed the ctor-visibility branch 2 times, most recently from 6916d5e to db23067 Compare March 6, 2020 03:20

Geod24 added 2 commits March 6, 2020 15:16

[Shared]Static{C,D}torDecl: Simplify and document constructor

5d2f412

Geod24 force-pushed the ctor-visibility branch from db23067 to 70dea3c Compare March 6, 2020 06:17

Geod24 mentioned this pull request May 15, 2020

std.math: hide parameter names for common math functions dlang/phobos#7480

Closed

Geod24 mentioned this pull request Nov 25, 2020

Improve DMD's JSON output: Always output visibility & give a special kind to module ctors #11993

Merged

CyberShadow reviewed Dec 8, 2020

View reviewed changes

dlang-bot added Needs Rebase stalled labels May 20, 2021

dlang-bot added the Needs Work label Dec 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 20607 - Module constructors are visible as regular function #10834

Fix issue 20607 - Module constructors are visible as regular function #10834

Geod24 commented Feb 25, 2020

dlang-bot commented Feb 25, 2020 •

edited

Geod24 commented Feb 25, 2020

jacob-carlborg Feb 25, 2020

Geod24 Feb 25, 2020

atilaneves Feb 25, 2020

Geod24 Feb 25, 2020

atilaneves commented Feb 25, 2020 •

edited

Geod24 commented Feb 25, 2020

JohanEngelen commented Feb 25, 2020

JohanEngelen commented Feb 26, 2020

Geod24 commented Mar 2, 2020

jacob-carlborg commented Mar 2, 2020 •

edited

Geod24 commented Mar 3, 2020 •

edited

Geod24 commented Mar 3, 2020

Geod24 commented Mar 3, 2020

jacob-carlborg commented Mar 3, 2020 •

edited

Geod24 commented Mar 3, 2020

Geod24 commented Mar 6, 2020

MoonlightSentinel commented Jul 2, 2020

Geod24 commented Jul 2, 2020

CyberShadow left a comment

Fix issue 20607 - Module constructors are visible as regular function #10834

Are you sure you want to change the base?

Fix issue 20607 - Module constructors are visible as regular function #10834

Conversation

Geod24 commented Feb 25, 2020

dlang-bot commented Feb 25, 2020 • edited

Bugzilla references

Testing this PR locally

Geod24 commented Feb 25, 2020

jacob-carlborg Feb 25, 2020

Choose a reason for hiding this comment

Geod24 Feb 25, 2020

Choose a reason for hiding this comment

atilaneves Feb 25, 2020

Choose a reason for hiding this comment

Geod24 Feb 25, 2020

Choose a reason for hiding this comment

atilaneves commented Feb 25, 2020 • edited

Geod24 commented Feb 25, 2020

JohanEngelen commented Feb 25, 2020

JohanEngelen commented Feb 26, 2020

Geod24 commented Mar 2, 2020

jacob-carlborg commented Mar 2, 2020 • edited

Geod24 commented Mar 3, 2020 • edited

Geod24 commented Mar 3, 2020

Geod24 commented Mar 3, 2020

jacob-carlborg commented Mar 3, 2020 • edited

Geod24 commented Mar 3, 2020

Geod24 commented Mar 6, 2020

MoonlightSentinel commented Jul 2, 2020

Geod24 commented Jul 2, 2020

CyberShadow left a comment

Choose a reason for hiding this comment

dlang-bot commented Feb 25, 2020 •

edited

atilaneves commented Feb 25, 2020 •

edited

jacob-carlborg commented Mar 2, 2020 •

edited

Geod24 commented Mar 3, 2020 •

edited

jacob-carlborg commented Mar 3, 2020 •

edited