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

[REG2.071] Issue 15984 - Interface contracts retrieve garbage instead… #8988

Closed
wants to merge 1 commit into from

Conversation

thewilsonator
Copy link
Contributor

… of parameters

Rebase of #5765

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @thewilsonator! 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 "master + dmd#8988"

@thewilsonator thewilsonator added the Review:WIP Work In Progress - not ready for review or pulling label Nov 21, 2018
@thewilsonator thewilsonator removed the Review:WIP Work In Progress - not ready for review or pulling label Nov 22, 2018
@thewilsonator thewilsonator force-pushed the fix-issue-15984 branch 2 times, most recently from 4245862 to 7a4a404 Compare November 22, 2018 03:00
@WalterBright
Copy link
Member

@rainers writes in #5765

"FYI: when porting the original fix to LDC, I chose to just pass the adjusted this pointer to the interface contracts. No magic required to change the actual pointer in ThisExp.
With this, your test case runs further than when built with DMD 2.071, but still asserts in C.f3.i."

I'm not sure what he means. Does he have a better fix? @rainers please clarify!

@rainers
Copy link
Member

rainers commented Nov 22, 2018

I'm not sure what he means. Does he have a better fix? @rainers please clarify!

I guess this referes to this commit: ldc-developers/ldc@628a560. This was 2 and a half year ago, so the following might not be accurate and also outdated:

IIRC dmd passes the object pointer to the interface contract, passes the offset as a parameter and goes to great length to correct both in the contract function when calling interface functions or accessing parameters. LDC instead passes the adjusted interface pointer as this so the contract just stays a regular unmodified function.

@WalterBright
Copy link
Member

Does the ldc commit fix bug 15984 for LDC?

@rainers
Copy link
Member

rainers commented Nov 23, 2018

Does the ldc commit fix bug 15984 for LDC?

The test in this PR passes with LDC but for the checks regarding this_ in f3 (this_ is a completely different pointer, maybe the capture). IIRC the LDC commit was a fix for existing tests in testcontracts.d that worked for dmd, but not LDC.

@FeepingCreature
Copy link
Contributor

FeepingCreature commented Nov 26, 2018

This PR does not seem to actually fix the issue in 15984.

import core.stdc.stdio;
interface I {
    void foo(Object obj)
    in { printf("%p\n", obj); obj.toString; }
}
class C : I { override void foo(Object obj) in { } do { } }
void main()
{
    auto c = new C;
    c.foo(new Object);
}

edit: Which as far as I can tell, comes about because C::foo() has no idea its parameter is actually used, and so blithely reuses its register.

@thewilsonator thewilsonator deleted the fix-issue-15984 branch May 15, 2019 20:02
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.

5 participants