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 issue 3031 - static var and func declaration conflict in different sub scope #11250

Closed
wants to merge 1 commit into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Jun 7, 2020

I'm glad to propose a fix for this very old (in IT time, 11 years is much !) issue.

The approach of the fix is based on the observation that the fundamental (and only) problem was that allowing such declarations would produce same mangling.
Now when the insertion in the scope fails, and if the declaration was static, its name is changed and a local alias declaration, taking the previous name, is generated.

concrectly, some code showing the lowering:

void main()
{
    // the following ...
    { static int a; }
    { static int a; } // same mangling, but mangling is not useful !
    // is rewritten as
    { static int a; }
    { static int newID; alias a = newID; }
}

This works because:

  • the static declaration, the one that needs a unique mangle, has now an unique identifier.
  • the code following the declaration use the new alias.

@ghost ghost requested a review from Geod24 as a code owner June 7, 2020 23:02
@dlang-bot
Copy link
Contributor

dlang-bot commented Jun 7, 2020

Thanks for your pull request, @NilsLankila!

Bugzilla references

Auto-close Bugzilla Severity Description
3031 major scoped static var conflicts

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

@Geod24
Copy link
Member

Geod24 commented Jun 8, 2020

I think this is also related to the aggregated bug Kenji created: https://issues.dlang.org/show_bug.cgi?id=14831
You might want to look it up to see which issues he ran into, particularly the extern (C[++]) ones.

src/dmd/dscope.d Outdated Show resolved Hide resolved
src/dmd/expressionsem.d Outdated Show resolved Hide resolved
src/dmd/expressionsem.d Outdated Show resolved Hide resolved
@ghost ghost requested a review from ibuclaw June 14, 2020 19:43
src/dmd/dscope.d Outdated Show resolved Hide resolved
@WalterBright
Copy link
Member

My idea was to fix this in the name mangling code. I like your idea of using an alias better.

@ghost ghost requested a review from WalterBright June 21, 2020 22:41
src/dmd/expressionsem.d Outdated Show resolved Hide resolved
test/compilable/test1537.d Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Jun 25, 2020

Is this good now ?

@@ -0,0 +1,23 @@
void main()
{
foo();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test should create multiple different instantiations of foo given #11250 (comment)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow you, it's only called once.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you expand the problem ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I meant is that there are only 2 statics generated in foo, one generating the original symbol, one with the aliased symbol. A third static would verify that the generated symbols don't collide. IIRC the version I commented on didn't use generateId so it might be solved now to some extend, but a test (a third static generated at the same location) could verify that.

There is still a problem with generateId, though: with separate compilation units, the same symbol name can be generated, but might have different implementations.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks for the explanations. Actually I'm embarassed because this was not supposed to be covered by the PR but it had appeared that the test passed.

The approach of the fix is based on the observation that the fundamental (and only) problem was that allowing such declarations would produce same mangling.
Now when the insertion in the scope fails, and if the declaration was static, its name is changed and a **local** alias declaration, taking the previous name, is generated.

This works because:
- the static declaration, the one that needs a unique mangle, has now an unique identifier.
- the code following the declaration use the new alias.
@MoonlightSentinel
Copy link
Contributor

Another solution was implemented

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.

6 participants