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

[stable] Fix Issue 21591 - missed backrefs in mangled names wrt. unmerged function types #12166

Merged
merged 1 commit into from Feb 4, 2021

Conversation

kinke
Copy link
Contributor

@kinke kinke commented Jan 29, 2021

TypeFunctions are apparently special and not merged (see #1102), so make sure to use the generic merged function type as backref cache key.

Unfortunately, this requires special care to prevent an infinite recursion, as merging is based on mangling.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @kinke! 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 coverage diff by visiting the details link of the codecov check)
  • 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
21591 normal Mangling problem wrt. backrefs and function types

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 "stable + dmd#12166"

* passed to `mangleToBuffer()`: `rootType`
*/
if (t != rootType && t.isTypeFunction())
t = t.merge2();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pinging @rainers. I've also tried if (t.deco && t.isTypeFunction()) which worked in my case too and wouldn't require this ugly rootType field; I'm not sure whether it would be reliable though...

Copy link
Member

Choose a reason for hiding this comment

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

Is it actually possible to have a function type refer to itself? How would you declare that? Or is the rootType test just triggering for erroneous type declarations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's possible; it's just there to prevent an infinite recursion in case the deco hasn't been computed yet and merge2() calls mangleToBuffer(tf), where we end up in here again.

@kinke kinke added the Industry Applies to PRs pertaining to industry applications of D label Jan 29, 2021
@kinke kinke force-pushed the tf_mangle branch 4 times, most recently from 13d399a to 007b25b Compare January 30, 2021 00:51
test/compilable/test21591.sh Outdated Show resolved Hide resolved
@kinke kinke force-pushed the tf_mangle branch 5 times, most recently from 74302e8 to 09ebd8a Compare February 1, 2021 12:23
void function() fn3(void function()**, void function()*);
F fr3(F**, F*);
static assert(fn3.mangleof == "_D9test215913fn3FPPPFZvQfZQh");
static assert(fr3.mangleof == "_D9test215913fr3FPPPFZvQfZQh");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This 'reference' mangle changes as well; previously, the return type wasn't backref'd (_D9test215913fr3FPPPFZvQfZPFZv).

…tion types

TypeFunctions are apparently special and not merged, so make sure to
use the generic merged function type as backref cache key.

Unfortunately, this requires special care to prevent an infinite
recursion, as merging is based on mangling.
void function()** fn4(ref void function(), ref void function()*);
F** fr4(ref F, ref F*);
static assert(fn4.mangleof == "_D9test215913fn4FKPFZvKPQgZPQf");
static assert(fr4.mangleof == "_D9test215913fr4FKPFZvKPQgZPQf");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one too (2nd param wasn't backref'd: _D9test215913fr4FKPFZvKPPFZvZPQh).

@RazvanN7 RazvanN7 added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Feb 1, 2021
@kinke
Copy link
Contributor Author

kinke commented Feb 1, 2021

@MartinNowak: Please include this in v2.095.1-beta1, this bug is preventing Symmetry from bumping the compilers to v2.095. :)

@RazvanN7 RazvanN7 merged commit d8668e1 into dlang:stable Feb 4, 2021
@kinke kinke deleted the tf_mangle branch February 4, 2021 15:19
@MartinNowak MartinNowak mentioned this pull request Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
72h no objection -> merge The PR will be merged if there are no objections raised. Bug Fix Industry Applies to PRs pertaining to industry applications of D
Projects
None yet
4 participants