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

Issue 14147 - Compiler crash on identical functions in a single module #7577

Merged
merged 7 commits into from Jan 24, 2018

Conversation

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

Auto-close Bugzilla Description
2789 Functions overloads are not checked for conflicts
14147 Compiler crash on identical functions in a single module

@@ -120,6 +120,7 @@ static foreach(member;__traits(allMembers,S)){
}

// print prime numbers using overload sets as state variables.
/+
static assert(is(typeof(bad57)));
static assert(!is(typeof(bad53)));
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure how I can "fix" this test. It generated bad57 a couple of times and AFAICT there's no break for static foreach yet.

CC @tgehr

@wilzbach wilzbach reopened this Jan 15, 2018
9rnsr and others added 7 commits January 22, 2018 16:33
If two extern (C) functions are just declared with different signatures,
they don't conflict.

 extern(C):
 alias sigfn_t  = void function(int);
 alias sigfn_t2 = void function(int) nothrow @nogc;
 sigfn_t  bsd_signal(int sig, sigfn_t  func);
 sigfn_t2 bsd_signal(int sig, sigfn_t2 func) nothrow @nogc;  // no error

This behavior must be reconsidered in the future.
@wilzbach
Copy link
Member Author

So we are all good here? Anything blocking this?

@JinShil
Copy link
Contributor

JinShil commented Jan 22, 2018

So we are all good here? Anything blocking this?

I'm just waiting for the tests to pass before giving this one last review

@wilzbach
Copy link
Member Author

I'm just waiting for the tests to pass before giving this one last review

FYI the Jenkins failure just means that the GCE has terminated the runner - it has nothing to do with this PR:

remote file operation failed: /var/lib/jenkins/dlang_projects@6 at hudson.remoting.Channel@4cea69e6:JNLP4-connect connection from 128.115.198.35.bc.googleusercontent.com/35.198.115.128:56402: hudson.remoting.ChannelClosedException: Remote call on JNLP4-connect connection from 128.115.198.35.bc.googleusercontent.com/35.198.115.128:56402 failed. The channel is closing down or has closed down

dlang/ci uses cheap preemptible GCE instances: https://cloud.google.com/compute/docs/instances/preemptible

A preemptible VM is an instance that you can create and run at a much lower price than normal instances. However, Compute Engine might terminate (preempt) these instances if it requires access to those resources for other tasks. Preemptible instances are excess Compute Engine capacity so their availability varies with usage.

@JinShil JinShil merged commit ab47b8b into dlang:master Jan 24, 2018
@wilzbach wilzbach deleted the pr-4396 branch February 7, 2018 23:17
@MartinNowak
Copy link
Member

This broke a subtler usage of extern(C) to just select the calling convention but not the mangling.
18385 – [REG 2.079] method cannot be overloaded with another extern(C) method

@jacob-carlborg
Copy link
Contributor

Can we revert this, due to issue 18385?

@wilzbach
Copy link
Member Author

Can we revert this, due to issue 18385?

Walter: Yes, it should never have compiled, and it cannot work, as two functions with the same mangled name cannot both exist in the executable.

but yeah, it looks like this needs a deprecation cycle, so we should probably revert it for now and re-add it with deprecations.

@jacob-carlborg
Copy link
Contributor

but yeah, it looks like this needs a deprecation cycle, so we should probably revert it for now and re-add it with deprecations.

I don't see why it has to be deprecated. The actual code that hit the regression have different mangled names:

class Foo
{
    extern (C) static void callback(int) {}
    extern (C) static void callback(char*) {}
}

In the above example the mangled names of callback is the same as for extern (D). I reduced the test case a bit too much.

@wilzbach
Copy link
Member Author

Well, let's go with #7969 for now. Once, that's in we can can still decide what is going to be deprecated with 2.080 and what not (the test cases of #7969 are definitely sth. that should be deprecated).

The actual code that hit the regression have different mangled names:

AFAICT the check already mangles the function signature and checks for that, but as said: "revert first, talk (in-depth) later". Sounds fair?

wilzbach added a commit to wilzbach/dmd that referenced this pull request Feb 28, 2018
wilzbach added a commit that referenced this pull request Mar 1, 2018
Temporarily disable #7577, s.t. a proper deprecation cycle can be started
@ibuclaw
Copy link
Member

ibuclaw commented Mar 1, 2018

Isn't the regression caused by your special handling of extern(C) symbols? Therefore that special handling should just be removed outright.

GDC has thrown errors about duplicate functions for a long time now, and it's surprising to see a couple of changes to the testsuite here that don't really cause symbol conflicts (unittest {} unittest {}). But I guess your patch complained about it away.

@jacob-carlborg
Copy link
Contributor

Isn't the regression caused by your special handling of extern(C) symbols? Therefore that special handling should just be removed outright.

Yeah, I think so. IIRC, Walter has also, in the extern(C++) discussions, been talking about that D should follow the same overloading rules for all symbols, i.e. no special treatment for extern(C++) symbols.

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