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 issue 21203: Accept pragma(mangle) on aggregate types #11835

Merged
merged 1 commit into from
Mar 24, 2021

Conversation

thewilsonator
Copy link
Contributor

@thewilsonator thewilsonator commented Oct 9, 2020

The basic idea is to have a way to substitute the name of the class as a value type and then modify it (e.g. with ref) to get the correct mangling for symbols that are otherwise impossible to represent.

Todo

  • namespaces
  • ABI tags
  • reserved names (e.g. std::function)

spec PR dlang/dlang.org#2875

cc @TurkeyMan @AndrejMitrovic

@thewilsonator thewilsonator added the Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org label Oct 9, 2020
@Geod24
Copy link
Member

Geod24 commented Oct 9, 2020

What is issue 21203 ?

@thewilsonator
Copy link
Contributor Author

@Geod24
Copy link
Member

Geod24 commented Oct 9, 2020

This was a subtle attempt to not repeat myself yet one more time by saying:

As usual, please give a proper title to both PR and commits.
Remember the Contributing guidelines include an article about good commit messages.

Time to make this a keyboard macro.

@thewilsonator
Copy link
Contributor Author

Well usually the bot would link that but this is a draft PR.

@Geod24
Copy link
Member

Geod24 commented Oct 9, 2020

Well usually the bot would link that but this is a draft PR.

It's not about the link. It's about:

  • Not forcing the people that watch this repository to open then notification to see if that's something of interest to them;
  • Allowing the people that use git blame or the history to open bugzilla to see if a commit is relevant to their interest;

@thewilsonator thewilsonator force-pushed the issue21203 branch 2 times, most recently from c8021f6 to 22e398a Compare October 14, 2020 07:55
@thewilsonator thewilsonator added the Review:Needs Changelog A changelog entry needs to be added to /changelog label Oct 16, 2020
@thewilsonator thewilsonator force-pushed the issue21203 branch 5 times, most recently from baaabd4 to 914d6f1 Compare October 23, 2020 03:40
@thewilsonator thewilsonator force-pushed the issue21203 branch 2 times, most recently from 5a7060b to 0a532a0 Compare November 10, 2020 01:36
@thewilsonator thewilsonator changed the title fix issue 21203 Fix issue 21203: Accept pragma(mangle) on aggregate types Nov 10, 2020
@thewilsonator thewilsonator marked this pull request as ready for review November 10, 2020 01:38
src/dmd/aggregate.d Outdated Show resolved Hide resolved
@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 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
21203 enhancement Accept pragma(mangle) on aggregate 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 "master + dmd#11835"

src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved
@WalterBright
Copy link
Member

I'm afraid I find this very complex for something that should be really simple - just use "abc" as the name for a symbol instead of ident when mangling it. I can't figure out what that optional 3rd argument is for.

@WalterBright
Copy link
Member

But if you want to call a function with C++ linkage that takes an argument of type stat, what won't work since the type is named stat_t in D and it will result in the wrong mangling.

I have a hard time with adding a very confusing feature to D just to pass stat to a C++ function. The easiest way to do this without language changes is for the user to:

extern (C++):
struct stat { stat_t s; }
void func(stat* s);

then just cast your stat_t* to stat*. There won't be any name collision because module symbols override imported symbols. Yeah, it's a kludge. But at least it's not a kludge in the core language.

Copy link
Member

@WalterBright WalterBright left a comment

Choose a reason for hiding this comment

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

See my comments.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 14, 2020

But if you want to call a function with C++ linkage that takes an argument of type stat, what won't work since the type is named stat_t in D and it will result in the wrong mangling.

I have a hard time with adding a very confusing feature to D just to pass stat to a C++ function. The easiest way to do this without language changes is for the user to:

extern (C++):
struct stat { stat_t s; }
void func(stat* s);

then just cast your stat_t* to stat*. There won't be any name collision because module symbols override imported symbols. Yeah, it's a kludge. But at least it's not a kludge in the core language.

But you won't be able to mix that with code that uses core.sys.posix.sys.stat.

@12345swordy
Copy link
Contributor

But if you want to call a function with C++ linkage that takes an argument of type stat, what won't work since the type is named stat_t in D and it will result in the wrong mangling.

I have a hard time with adding a very confusing feature to D just to pass stat to a C++ function. The easiest way to do this without language changes is for the user to:

extern (C++):
struct stat { stat_t s; }
void func(stat* s);

then just cast your stat_t* to stat*. There won't be any name collision because module symbols override imported symbols. Yeah, it's a kludge. But at least it's not a kludge in the core language.

You asserting that the user have the ability to change the source code, that is not always the case here.

@WalterBright
Copy link
Member

You asserting that the user have the ability to change the source code, that is not always the case here.

The user can change his own source code, and that is all that's necessary.

But you won't be able to mix that with code that uses core.sys.posix.sys.stat.

Sure you can, because D has different name spaces. That's what my example relied on. You can locally create which definition of stat is locally needed.

@12345swordy
Copy link
Contributor

12345swordy commented Dec 15, 2020

You asserting that the user have the ability to change the source code, that is not always the case here.

The user can change his own source code, and that is all that's necessary

If it is not their own code, then what? You are making assertions regarding @ibuclaw , @TurkeyMan , among others that are interested in this feature without knowing their situation that requires solution like this to exist.

@thewilsonator Can you explain the motivation behind this feature and what problems can't be fixed with current solutions for Walter please?

@Geod24
Copy link
Member

Geod24 commented Dec 16, 2020

The issue contains a motivation for the feature.

@12345swordy
Copy link
Contributor

pragma(mangle) is only accepted on hard functions and variables.
Adding support to AggregateDecl will allow the user to customise the mangling for type names, and that mangle override should be used when composing mangled names for functions/templates.

This is the key to allowing comprehensive C++ class interop.

For instance, this becomes possible:

struct ScopeClass(C)
if (is(C == class)
{
static if (__traits(getLinkage, C) == "C++")
{
extern(C++, class)
pragma(mangle, C.mangleof) // <- here's the magic!
struct ScopeClass
{
char[__traits(classInstanceSize, C)] buffer;

  //... all the things ...
}

}
}

And then consider these functions:

void fun(MyClass); // mangles MyClass*
void fun(const MyClass); // mangles const MyClass* const
void fun(ScopedClass!MyClass); // mangles MyClass
void fun(const ScopedClass!MyClass); // mangles const MyClass
void fun(ref ScopedClass!MyClass); // mangles MyClass&
void fun(ref const ScopedClass!MyClass); // mangles const MyClass&

By allowing pragma(mangle) to apply to AggregateDecl, we can override the mangling for the type names, and with this, we can write in-language tooling that can handle classes in ALL the ways that C++ can express.

I take this above is what you referring to right?

@thewilsonator
Copy link
Contributor Author

@thewilsonator Can you explain the motivation behind this feature and what problems can't be fixed with current solutions for Walter please?

It is impossible to bind to anything that involves std::function without this. This also solves the issue of C++ classes passed by value and [const] ref as above.

I can't figure out what that optional 3rd argument is for.

Template argument list mangling where the desired name is different from the name in the source (e.g. std::function).

@WalterBright
Copy link
Member

I'm not objecting to attaching mangling to an aggregate. It's that 3rd argument that baffles me.

Template argument list mangling where the desired name is different from the name in the source (e.g. std::function).

I still don't understand that. I have no idea what that does or why it is needed. @12345swordy 's comment does not use the third argument.

@thewilsonator
Copy link
Contributor Author

I have no idea what that does or why it is needed

extern(C++)
struct Foo {}
struct Bar(T) {}
extern(C++, "std")
{
        class _function(T) { } // want to be std::function
}
template ScopeClass(C)
{
    extern(C++, class)
    pragma(mangle, stuff_goes_here)
    struct ScopeClass { ... }
}

By necessity of ScopeClass being a template, the mangling of ScopeClass!Foo must not include a template parameter Foo i.e. it must not mangle as Foo<Foo>, so template parameters must be stripped. For this case stuff_goes_here being a string suffices and one argument is fine.
However ScopeClass!(Bar!int) does have template parameters and needs to be mangled as Bar<int>. Because the outer template ScopeClass must be stripped from the mangling, and the <int> must be retained we need a symbol that has the template signature to copy. We can also copy the name of the symbol while we're at it, for this case stuff_goes_here being a symbol suffices and one argument is fine.
For ScopeClass!(_function!(myfunc)) we want the identifier that goes into the mangler to be "function" (without the leading underscore) so we need a string, but it is a template so we need to provide a symbol to copy the template parameters from. For this case stuff_goes_here needs both a string and a symbol.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Jan 7, 2021

@thewilsonator What is the status on this? I see that the Azure builds are failing, however, when I try to check the error it says "Build is missing"

@TurkeyMan
Copy link
Contributor

TurkeyMan commented Jan 7, 2021

Nick: the example you gave above is suspect, ScopeClass!(_function!(myfunc)) is not a useful thing.
std::function could be declared something like:

template _function(ArgSeq...)
{
    extern(C++, "std")
    {
        pragma(mangle, "function", ArgSeq)
        extern(C++, class)
        struct _function(T) { ... }
    }
}

We don't need to copy the args from a symbol in this case, we have the template arg tuple which could be supplied to pragma(mangle) directly as I show above.

That said, this leads to some background on this discussion...
What I initially preferred was:
pragma(mangle, StringExp, [Optional template args for mangling]) (ie, my example above), which is suitable for cases like std::function above.

However, there is a case where giving a tuple of args to pragma(mangle) falls over, which is the zero-template-args case. We can't distinguish these 2 cases:

  pragma(mangle, "myName")
  pragma(mangle, "myName", AliasSeq!())

Trouble is though, that there is a critical difference in mangling between a not-a-template, and a var-arg template with zero args.

  struct X {}
  struct Y(Args...) {}

X.mangleof and Y!().mangleof are very different things.

This is what lead Nick to prefer copying template args from a symbol, rather than receiving a list of template args to use for the mangling. The symbol being copied from can express being not-a-template, or having zero or more template args.
I'm still not super happy with that decision, but it does address the issue.

I initially thought of a pair of pragma(mangle) and pragma(mangleTemplate), but then it doesn't support 'forwarding' in a way that works... if you're supplying a symbol to a piece of meta like ScopedClass, you need to select mangle or mangleTemplate based on whether the source symbol is a template or not... and that's very awkward to achieve.

So, the situation right now; I think the current implementation supports these cases:
pragma(mangle, StringExp) <- simple case, this should exist
pragma(mangle, Symbol, StringExp) <- I think this is backwards; the string should come first always
pragma(mangle, Symbol) <- Copy the mangle name AND template args from the symbol; I don't think we should support this case

I think we can do with these 2 forms:
pragma(mangle, StringExp) <- simple case, this should exist
pragma(mangle, StringExp, Symbol) <- optional symbol, always second arg

And the user can express: pragma(mangle, mySymbol.stringof, mySymbol) which will allow to take the name and args both from a reference symbol.

I recall making this recommendation to Nick, and I think he may have had a reason he didn't do this...?

An alternative solution to solve the not-a-template vs zero-template-arg ambiguity would also be a consideration.

I think StringExp should be emphasised as the first parameter; the reason being that the most obvious common-case where pragma(mangle) is useful is when trying to override mangling in cases where a C++ class has a name that collides with a D keyword. That will be the typical user-facing use of this feature. The advanced use of pragma(mangle) being discussed here will be wrapped up in a ScopeClass implementation which I think should go in druntime, and so most users will never write that in user code.


On a side note; I just want to mention that this is actually a REALLY powerful feature. What this feature can do is solve the long-standing extern(C++) for functions that accept polymorphic classes by value issue, which is something we can't achieve in D today, and it's the single issue that almost everyone that tries to use extern(C++) runs into and ruins their day.
With this mangle override, we can avoid a language solution to the problem by creating a ScopeClass utility library that handles a by-val version of a D class, and also mangles it correctly. As nick roughly demonstrated above:

template ScopeClass(C)
{
  extern(C++, class)
  pragma(mangle, C.stringof, C) // <- here's the magic; we override `ScopeClass!C` mangling to that which mataches C++!
  struct ScopeClass
  {
    char[__traits(classInstanceSize, C)] buffer;
    //... all the expected things ...
  }
}

With this tool, we finally gain by-value binding compatibility with C++. This is HUGE.

@WalterBright
Copy link
Member

pragma(mangle, C.stringof, C) // <- here's the magic; we override ScopeClass!C mangling to that which mataches C++!

I'm sorry, all I see are bits and pieces of what that 3rd argument does. How about a real, minimal, actual example?

@thewilsonator
Copy link
Contributor Author

I still don't understand that. I have no idea what that does or why it is needed ... real, minimal, actual example?

std::function. The case for the utility that 3 arguments provides only arises when trying to mangle something that collides with a D keyword that is templated.
In this case you need to call the class (or struct) in D a different name than what it is called in C++ (because otherwise the compiler will fail to pass your declaration).
You also need to provide a symbol to fake the template parameters from, as you do for any other templated aggregate (because otherwise there is ambiguity between a non-template and a pro arg template).
Unlike the case where the name of the aggregate is not a D keyword where you can use the identifier from the aggregate, you can't do that for std::function because you can't call a struct or class function.

template ScopeClass(C , string name /*= C.stringof*/) // sensible defaults don't work for something you can't name
{
    enum ns = __traits(getCppNamespaces,C);
    extern(C++, class)
    {
        extern(C++,(ns))
        {
            pragma(mangle, C, name) // <<<
            struct ScopeClass
            {
                char[__traits(classInstanceSize, C)] buffer;
                //... all the things ...
            }
        }
    }
}

struct _function(T) { ... }

extern(C++) void funk(ScopeClass!(_function!(void function(int)),"function") a ){ }

Copy link
Contributor

@12345swordy 12345swordy left a comment

Choose a reason for hiding this comment

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

Everything LGTM. Just need @WalterBright approval on this.

@thewilsonator thewilsonator dismissed WalterBright’s stale review March 22, 2021 03:07

comments addressed

@thewilsonator thewilsonator added Merge:72h no objection -> merge The PR will be merged if there are no objections raised. Merge:auto-merge labels Mar 22, 2021
@dlang-bot dlang-bot merged commit 49374b6 into dlang:master Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge:auto-merge Merge:72h no objection -> merge The PR will be merged if there are no objections raised. Severity:Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants