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 15984 - [REG2.071] Interface contracts retrieve garbage instead of parameters #9782

Merged
merged 2 commits into from
May 15, 2019

Conversation

SSoulaimane
Copy link
Member

Copied from LDC func.d with slight modifications.

Essentially the parameters are passed explicitly to the contract functions. The major modification that I made from LDC is that I pass all the parameters by ref.

@SSoulaimane SSoulaimane force-pushed the interface_contractsx branch 4 times, most recently from 3b22862 to 28331c6 Compare May 13, 2019 05:59
src/dmd/func.d Outdated Show resolved Hide resolved
@thewilsonator
Copy link
Contributor

cc @FeepingCreature

@SSoulaimane SSoulaimane force-pushed the interface_contractsx branch 3 times, most recently from 4826af9 to ae983e5 Compare May 14, 2019 00:07
@SSoulaimane SSoulaimane marked this pull request as ready for review May 14, 2019 01:36
@SSoulaimane SSoulaimane requested a review from ibuclaw as a code owner May 14, 2019 01:36
@SSoulaimane SSoulaimane force-pushed the interface_contractsx branch from ae983e5 to ba6e2fc Compare May 15, 2019 07:59
@dlang-bot
Copy link
Contributor

dlang-bot commented May 15, 2019

Thanks for your pull request and interest in making D better, @SSoulaimane! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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
15984 regression [REG2.071] Interface contracts retrieve garbage instead of parameters

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + dmd#9782"

@thewilsonator
Copy link
Contributor

../generated/linux/release/64/dmd -conf= -m64 -fPIC -g -lowmem -odtest_results -oftest_results/d_do_test tools/d_do_test.d
test_results/d_do_test.o: In function `_D3std5regex8internal6parser__T6ParserTAyaTSQBqQBpQBmQBg7CodeGenZQBi11parseEscapeMFNeZv':
/home/circleci/dmd/test/../../phobos/std/regex/internal/parser.d:983: undefined reference to `_D3std3uni__T5StackTkZQj3topMFNaNbNcNdNiNfZk'
collect2: error: ld returned 1 exit status
Error: linker exited with status 1
Makefile:202: recipe for target 'test_results/d_do_test' failed

@SSoulaimane SSoulaimane force-pushed the interface_contractsx branch from 3de0f0a to b865171 Compare May 15, 2019 08:07
@thewilsonator
Copy link
Contributor

Also please target stable as this is a very critical bug fix for Funkwerk AG.

@SSoulaimane
Copy link
Member Author

../generated/linux/release/64/dmd -conf= -m64 -fPIC -g -lowmem -odtest_results -oftest_results/d_do_test tools/d_do_test.d
test_results/d_do_test.o: In function `_D3std5regex8internal6parser__T6ParserTAyaTSQBqQBpQBmQBg7CodeGenZQBi11parseEscapeMFNeZv':
/home/circleci/dmd/test/../../phobos/std/regex/internal/parser.d:983: undefined reference to `_D3std3uni__T5StackTkZQj3topMFNaNbNcNdNiNfZk'
collect2: error: ld returned 1 exit status
Error: linker exited with status 1
Makefile:202: recipe for target 'test_results/d_do_test' failed

Rebased. Now it's gone.

Also please target stable as this is a very critical bug fix for Funkwerk AG.

Ok.

@thewilsonator
Copy link
Contributor

thewilsonator commented May 15, 2019

Also Also if this is an ABI change it should have a changelog entry stating that. Can be added later.

@SSoulaimane
Copy link
Member Author

Also Also if this is an ABI change it should have a changelog entry stating that.

Yes, it is an ABI change.

@wilzbach
Copy link
Member

We don't require changelog entries for ABI changes, but as this is very important bug fix it wouldn't hurt at all and just help to increase the visibility and awareness of its fix.

@SSoulaimane SSoulaimane force-pushed the interface_contractsx branch from b865171 to 4b47a3c Compare May 15, 2019 08:21
@SSoulaimane SSoulaimane changed the base branch from master to stable May 15, 2019 08:23
@SSoulaimane SSoulaimane force-pushed the interface_contractsx branch from 4b47a3c to be4013b Compare May 15, 2019 08:31
@thewilsonator
Copy link
Contributor

That too.

@SSoulaimane SSoulaimane force-pushed the interface_contractsx branch from 12c5b7a to ee18863 Compare May 15, 2019 09:08
@dlang-bot dlang-bot merged commit b10a805 into dlang:stable May 15, 2019
kinke added a commit to kinke/dmd that referenced this pull request May 19, 2019
kinke added a commit to ldc-developers/dmd-testsuite that referenced this pull request May 19, 2019
dlang-bot added a commit that referenced this pull request May 19, 2019
[stable] Remove unused leftover after #9782
merged-on-behalf-of: Jacob Carlborg <jacob-carlborg@users.noreply.github.com>
@kinke
Copy link
Contributor

kinke commented May 22, 2019

Also please target stable as this is a very critical bug fix for Funkwerk AG.

I don't think that was the right decision. It's a pretty big change in general, and ABI changes in a patch release are highly questionable IMO. The bug has been old, and a company welcoming a fix isn't a valid argument IMO.

Anyway, this caused unexpected trouble for LDC. One test also fails for DMD stable, so that's a regression from 2.086.0:

alias Bar = extern (C) void function(void*);

final class C
{
    void foo(Bar bar)
    in { assert(bar); }
    do {}
}
Error: function mod.C.foo.__require(ref void function(void*) bar) is not callable using argument types (extern (C) void function(void*))
       cannot pass argument bar of type extern (C) void function(void*) to parameter ref void function(void*) bar

I guess it's a syntaxCopy() issue (extern (C) linkage lost somewhere); TypeFunction.syntaxCopy() seems to copy the linkage though.

@SSoulaimane
Copy link
Member Author

SSoulaimane commented May 22, 2019

@kinke here #9843 is a fix for that.

dlang-bot added a commit that referenced this pull request May 23, 2019
Fix interface contracts regression recently introuced in #9782
merged-on-behalf-of: Nicholas Wilson <thewilsonator@users.noreply.github.com>
@ibuclaw
Copy link
Member

ibuclaw commented Aug 23, 2019

You forgot to clean-up the backend. With this, all the horrible special casing of require/ensure becomes unnecessary as there is no special frame needed anymore to get to parameters that were originally outside the contract context.

@SSoulaimane
Copy link
Member Author

Yes it's still there.

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.

7 participants