Fix issue 18095 - Add support for pragma(mangle) on alias declarations #7458

jacob-carlborg · 2017-12-17T17:11:10Z

This allows to override the mangling of a type in an alias declaration as follows:

pragma(mangle, "foo") alias foo = int;
static assert(foo.mangleof == "foo");
static assert(int.mangleof == "i");

Overriding the mangling of a type in an alias allows for more generic solution to get the correct mangling of c_long/c_ulong for C++ functions. With the current solution the compiler is looking for a struct with the name __c_long/__c_ulong and special cases the mangling for that particular struct. Since D doesn't have implicit conversions to structs one are also forced cast/explicitly construct a __c_long struct when calling a function which uses the __c_long type as a parameter. With an alias that's not necessary anymore.

dlang-bot · 2017-12-17T17:11:12Z

Thanks for your pull request, @jacob-carlborg! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✓	18095	Add support for pragma(mangle) on alias declarations

RazvanN7

How is the change in the backend related to this PR?

Later edit: Oh, I see, nevermind.

JinShil · 2017-12-26T03:39:16Z

test/runnable/mangle.d

+    @("basic type")
+    {
+        @("override mangling")
+        {


What is the purpose of all of these @("some string") UDAs?

It adds a description to each test indicating what is tested. If you don't like it I can remove it. I think it adds value.

I don't see where those descriptions are being used anywhere. How is this different from just adding a comment?

They could be used in the future and they allow me to add a form of scope for the description.

I think until we have something in our infrastructure that makes use of these descriptions, they're a bit of a distraction.

Fine 😞. I'll remove or move them to comments.

I think that now it's much more difficult to see which code belongs to which test and which description belongs to which test. In any other D project I would have used a separate unit test block for each test, but the DMD tests are not using unit test blocks.

You can easily resolve that with some kind of comment header...

/******************************************************* * Section 1 *******************************************************/ // Section 1.1 // Section 1.2 /******************************************************* * Section 2 *******************************************************/ // Section 2.1 // Section 2.2

... or something along those lines

FWIW, I see the value in what you were proposing, but I don't think this PR is the right place to introduce such idioms into the code base unless it solves a specific problem that we're currently facing and/or has broader support/adoption.

I think it solves specific problem we have but nobody else seems to agree with me.

rainers · 2017-12-26T08:03:30Z

I like the idea and also tried to implement it when Walter introduced those special structs that don't work well without implicit conversions.

I suspect it's not so easy, though. Doesn't this reintroduce some kind of "typedef", i.e. types are compared according to their mangling so the type with mangle overwrite is no longer the same as the aliased type?

pragma(mangle, "foo") alias a = int;
static assert(is(a == int));

Could you also add a test case where the mangle override is at some intermediate step?

alias a = int;
pragma(mangle, "3foo") alias b = a;
alias c = b;
c x;
static assert(x.mangle == "_D4test1x3foo");

jacob-carlborg · 2017-12-26T08:28:07Z

@rainers Done.

jacob-carlborg · 2017-12-26T09:45:14Z

I need to add a test for C++ mangling as well.

jacob-carlborg · 2017-12-26T09:46:24Z

Do we have any tests for the mangling in the actual binary? .mangleof has a slightly different implementation.

jacob-carlborg · 2017-12-26T19:12:22Z

Added tests for C++ mangling as well.

jacob-carlborg · 2017-12-28T14:17:11Z

I suspect it's not so easy, though. Doesn't this reintroduce some kind of "typedef", i.e. types are compared according to their mangling so the type with mangle overwrite is no longer the same as the aliased type?

@rainers I added a new field to the Type base class, mangleOverride, which is not used for anything but the mangled name. It's not used in any comparison of types. So no, this is not a new kind of typedef and aliases to the same type with different mangling will be considered the same type.

jacob-carlborg · 2017-12-28T14:17:25Z

Can we move forward with this?

rainers · 2017-12-29T08:04:54Z

I added a new field to the Type base class, mangleOverride, which is not used for anything but the mangled name.

I noticed, but Type.merge() uses the mangling to fold multiple types into one. I'm not sure this isn't used somewhere else. It even makes the definition of what actually is an identical type non-obvious: Can a function with a different mangling have the same type?

Can we move forward with this?

I guess it needs approval by @WalterBright or @andralex . A good example to show the merits would be to implement c_long et al. with this.

jacob-carlborg · 2017-12-29T11:54:51Z

I noticed, but Type.merge() uses the mangling to fold multiple types into one. I'm not sure this isn't used somewhere else.

I'm not sure I understand. I added a new field so I know it's not used anywhere else.

It even makes the definition of what actually is an identical type non-obvious: Can a function with a different mangling have the same type?

I don't understand this question either. It's already possible today to manually set the mangling of a function.

rainers · 2017-12-29T12:46:59Z

I'm not sure I understand. I added a new field so I know it's not used anywhere else.

Check the implementation of Type.merge(): two types is considered the same if they have the same mangling, i.e. a Type object is thrown away if another identical Type object already exists. Does this imply that types are different if their mangling differs?

Example:

pragma(mangle, "3foo") alias foo = int;
void fun1(int x);
void fun2(foo x);
void call1(void function(int));
void call2(void function(foo));

void main()
{
   call1(&foo2);
   call2(&foo1);
}

Does this still work? Are the function still compatible although the mangling of the type is different? It might be ok because checks for implicit conversions are satisfied.

It's already possible today to manually set the mangling of a function.

Yes, but not the mangling of its type.

jacob-carlborg · 2017-12-29T20:52:04Z

Does this still work? Are the function still compatible although the mangling of the type is different? It might be ok because checks for implicit conversions are satisfied.

@rainers the above works, but the following fails:

static assert(is(void function(int) == void function(foo)))

I'll have a look to see if I can figure something out.

This allows to override the mangling of a type in an alias declaration as follows: ```d pragma(mangle, "foo") alias foo = int; static assert(foo.mangleof == "foo"); static assert(int.mangleof == "i"); ``` Overriding the mangling of a type in an alias allows for more generic solution to get the correct mangling of `c_long/c_ulong` for C++ functions. With the current solution the compiler is looking for a struct with the name `__c_long/__c_ulong` and special cases the mangling for that particular struct. Since D doesn't have implicit conversions to structs one are also forced cast/explicitly construct a `__c_long` struct when calling a function which uses the `__c_long` type as a parameter. With an alias that's not necessary anymore.

andralex · 2018-01-24T03:46:24Z

status?

jacob-carlborg · 2018-01-24T08:46:19Z

@andralex the status is that it wasn't so easy to implement as I thought it would be. Currently I'm not working on this. The issue is, as Rainer pointed out, that the mangling is used for type comparison as well. An alias with pragma(mangle) is not intended to introduce a new type.

JinShil · 2018-02-16T13:13:28Z

I suggest this be closed if it's not being actively pursued. Any objections?

wilzbach · 2018-03-15T19:34:04Z

FWIW this would have been really helpful for #7893 (comment) where we can't do alias string = immutable(char)[], because the compiler doesn't know how to mangle immutable to C++.

jacob-carlborg · 2018-03-15T19:50:03Z

FWIW this would have been really helpful for #7893 (comment) where we can't do alias string = immutable(char)[], because the compiler doesn't know how to mangle immutable to C++.

Yes, but I haven't come up with a way to solve the type comparison problem that Rainer mentioned.

jacob-carlborg requested a review from WalterBright as a code owner December 17, 2017 17:11

dlang-bot added the Severity:Enhancement label Dec 17, 2017

RazvanN7 reviewed Dec 18, 2017

View reviewed changes

jacob-carlborg force-pushed the 18095-alias-mangle branch from 46bdb91 to e15d801 Compare December 20, 2017 13:57

jacob-carlborg mentioned this pull request Dec 24, 2017

fix Issue 18053 - Use stdint.h mangling for int64_t/uint64_t when man… #7416

Merged

JinShil reviewed Dec 26, 2017

View reviewed changes

jacob-carlborg force-pushed the 18095-alias-mangle branch from e15d801 to 9704601 Compare December 26, 2017 08:27

jacob-carlborg force-pushed the 18095-alias-mangle branch from 9704601 to 56db4b0 Compare December 26, 2017 19:05

jacob-carlborg force-pushed the 18095-alias-mangle branch 2 times, most recently from 2fe5fe0 to 41f6e94 Compare December 27, 2017 13:59

dlang-bot added Review:Needs Rebase Review:Needs Work labels Jan 1, 2018

jacob-carlborg force-pushed the 18095-alias-mangle branch from 41f6e94 to 68d36fe Compare January 1, 2018 21:05

dlang-bot added Review:Needs Rebase and removed Review:Needs Rebase labels Jan 2, 2018

jacob-carlborg mentioned this pull request Feb 14, 2018

[WIP] add extern (C++) struct Darray(T) and Dstring to simplify interop with C++ #7893

Closed

jacob-carlborg closed this Feb 16, 2018

This was referenced Apr 9, 2018

use C mangling for unsigned long long for now #7523

Merged

use special enums for __c_long, etc. instead of special structs #8152

Merged

jacob-carlborg mentioned this pull request Jan 7, 2019

posix stdlib, inttypes - compatibility with new wchar_t dlang/druntime#2394

Closed

Uh oh!

Fix issue 18095 - Add support for pragma(mangle) on alias declarations #7458

Fix issue 18095 - Add support for pragma(mangle) on alias declarations #7458

Uh oh!

Conversation

jacob-carlborg commented Dec 17, 2017

Uh oh!

dlang-bot commented Dec 17, 2017

Bugzilla references

Uh oh!

RazvanN7 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainers commented Dec 26, 2017

Uh oh!

jacob-carlborg commented Dec 26, 2017

Uh oh!

jacob-carlborg commented Dec 26, 2017

Uh oh!

jacob-carlborg commented Dec 26, 2017

Uh oh!

jacob-carlborg commented Dec 26, 2017

Uh oh!

jacob-carlborg commented Dec 28, 2017

Uh oh!

jacob-carlborg commented Dec 28, 2017

Uh oh!

rainers commented Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacob-carlborg commented Dec 29, 2017

Uh oh!

rainers commented Dec 29, 2017

Uh oh!

jacob-carlborg commented Dec 29, 2017

Uh oh!

andralex commented Jan 24, 2018

Uh oh!

jacob-carlborg commented Jan 24, 2018

Uh oh!

JinShil commented Feb 16, 2018

Uh oh!

wilzbach commented Mar 15, 2018

Uh oh!

jacob-carlborg commented Mar 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

RazvanN7 left a comment •

edited

Loading

rainers commented Dec 29, 2017 •

edited

Loading