Fix issue 19949 - C++ Mangling: add support for abi-tags from the Itanium ABI #9995

SSoulaimane · 2019-06-08T16:38:23Z

Implements "abi-tags" from the Itanium ABI specification.
https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangle.abi-tag.

By introducing a special compile time attribute @__gnu_abi_tag().

This is crucial in order to interface std::string and std::list on linux.

See: issue 14956 (comment 13).

jacob-carlborg · 2019-06-08T19:11:12Z

src/dmd/dsymbolsem.d

+        attribSemantic(uad);
+        if (uad.atts)
+        {
+            // detect the special attribute @__gnu_abi_tag(...)


This looks like a magic name. I would prefer to implement this as a true UDA which can use the fully qualified name to resolve any conflicts. This would avoid the need for the ugly double underscore prefix. You can have a look at how the @selector attribute is implemented.

I'll take a look.

Yes, that is a better approach. I switched to it. I didn't make it part of druntime yet though.
I also ended up making an exception for namespaces from the UDA inheritance rules to stop members from inheriting this special UDA from namespaces, this is how it behaves in C++.

Here is the core.attribute addition dlang/druntime#2639. Once it gets in I'll activate the module checks.

jacob-carlborg · 2019-06-08T19:11:41Z

src/dmd/cppmangle.d

@@ -150,18 +150,21 @@ private final class CppMangleVisitor : Visitor
    Objects components;         // array of components available for substitution
    OutBuffer* buf;             // append the mangling to buf[]
    Loc loc;                    // location for use in error messages
+    Strings* abiTags;           // record of the processed abi-tags


Can we use D arrays?

DMD arrays mange their own memory.

@SSoulaimane DMD is a mechanical translation from a C++ code base so it contains a lot of code that is not considered idiomatic D. Please prefer modern idiomatic D types and patterns (unless explicitly asked not to) for any new code so we can gradually transition DMD away from the cruft of the past.

Please use Ddoc comments.

The array is being filled by the visitor, I'm not sure that using D arrays is an improvement here because they rely on the GC. As I understand DMD is moving away from the GC.

As I understand DMD is moving away from the GC.

No, not that I've heard of. Also, D arrays can be allocated using other means than the GC.

No, not that I've heard of.

I thought using mem.xmalloc and mem.xfree was supposed to make DMD more responsible about memory.

Also, D arrays can be allocated using other means than the GC.

How do you append to it.

The array fed to the visitor is stack allocated hence automatically freed.

src/dmd/dsymbolsem.d

src/dmd/cppmangle.d

WalterBright · 2019-06-08T21:02:28Z

Why is this targeting stable rather than master?

WalterBright · 2019-06-08T21:03:11Z

This is a very large PR. Please factor out isPosix into a separate one.

SSoulaimane · 2019-06-08T21:04:34Z

Why is this targeting stable rather than master?

It just happened that way because it was based on #9946. I'll change that before it's merged.

SSoulaimane · 2019-06-08T21:06:05Z

This is a very large PR. Please factor out isPosix into a separate one.

Rebased

WalterBright · 2019-06-08T21:06:41Z

https://issues.dlang.org/show_bug.cgi?id=19949

jacob-carlborg · 2019-06-10T17:07:31Z

src/dmd/dsymbolsem.d

+            }
+            if (e.op == TOK.type)
+            {
+                e.error("`@%s` at least one argument expected", Id.udaGNUAbiTag.toChars());


Won't this be handled automatically?

No, that is the case where the UDA is a type i.e. @type instead of @type().

Hmm, I would expect the compiler to complain if the UDA is a function expecting one argument. But that seems to not be the case.

It's useful to have.

SSoulaimane · 2019-06-11T19:19:58Z

Because of the use of ancient C++ compilers in some the CIs, I will remove the runnable tests and stick to manual checks of the mangling with compilable tests instead. It has been demonstrated that it works at least.
Concrete support of the abi_tag only started in GCC 5 and Clang 3.9. Before that it was flimsy in GCC and unsupported in clang.

dlang-bot · 2019-06-12T01:56:57Z

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 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
✓	19949	normal	C++ Mangling: no support for abi-tags from the Itanium ABI

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#9995"

jacob-carlborg · 2019-06-12T07:36:08Z

src/dmd/cppmangle.d

     */
-    this(OutBuffer* buf, Loc loc)
+    this(OutBuffer* buf, Loc loc, Strings* abiTags = null)


I know Walter doesn’t like default values for parameters.

I think it's something with being less readable, but you'll have to ask him.

Well, pick your poison. Either this or an overload. Everything has a purpose.

I prefer a default value, but it’s not my decision.

It’s also possible to explicitly pass in the default value.

src/dmd/semantic2.d

test/compilable/cppmagle_abitag.d

Geod24 · 2019-06-12T23:12:05Z

Any chance to get a runnable test with this ? compilable tests are great for development and changes, because you clearly see what went wrong, but runnable are better in terms of coverage.

Geod24

And also, extern(C++, "namespace") is preferred (and code path are quite different).
You can either tests both or test only the string version (I'll have a deprecation PR for the non-string version up soon).

test/compilable/cppmagle_abitag.d

SSoulaimane · 2019-06-12T23:25:30Z

Any chance to get a runnable test with this ? compilable tests are great for development and changes, because you clearly see what went wrong, but runnable are better in terms of coverage.

1e58f7c. I deleted them because semaphore and OSX on the auto-tester have ancient c++ compilers.

Geod24 · 2019-06-13T00:01:48Z

1e58f7c. I deleted them because semaphore and OSX on the auto-tester have ancient c++ compilers.

Could easily disable it on OSX. Semaphore you'll need to read the environment variable I suppose.

SSoulaimane · 2019-06-13T00:09:27Z

Semaphore you'll need to read the environment variable I suppose.

To do what?

Geod24 · 2019-06-13T00:15:06Z

Disable it

src/dmd/attrib.d

…nium ABI

Uses a bash test to get version information about the host C++ compiler.

… function

Geod24 · 2020-03-28T11:40:58Z

The replacement PR has been merged. Thanks for getting us started on this, your work was a great help.

SSoulaimane force-pushed the cpp_abi_tag branch 7 times, most recently from a661c7d to ea0d336 Compare June 8, 2019 18:50

jacob-carlborg suggested changes Jun 8, 2019

View reviewed changes

SSoulaimane force-pushed the cpp_abi_tag branch 3 times, most recently from 2b44e5a to 70037d0 Compare June 10, 2019 16:16

jacob-carlborg reviewed Jun 10, 2019

View reviewed changes

SSoulaimane force-pushed the cpp_abi_tag branch 2 times, most recently from 4e4d22f to a2af683 Compare June 11, 2019 19:11

SSoulaimane force-pushed the cpp_abi_tag branch from a2af683 to 675e15a Compare June 11, 2019 19:32

SSoulaimane changed the base branch from stable to master June 11, 2019 19:33

SSoulaimane force-pushed the cpp_abi_tag branch from 675e15a to a2413ae Compare June 11, 2019 19:34

SSoulaimane marked this pull request as ready for review June 11, 2019 23:38

SSoulaimane requested a review from ibuclaw as a code owner June 11, 2019 23:38

SSoulaimane mentioned this pull request Jun 12, 2019

issue 19949 - C++ Mangling: add support for abi-tags from the Itanium ABI dlang/druntime#2639

Closed

dlang-bot added the Severity:Bug Fix label Jun 12, 2019

jacob-carlborg reviewed Jun 12, 2019

View reviewed changes

src/dmd/semantic2.d Outdated Show resolved Hide resolved

jacob-carlborg reviewed Jun 12, 2019

View reviewed changes

test/compilable/cppmagle_abitag.d Show resolved Hide resolved

SSoulaimane force-pushed the cpp_abi_tag branch from 3ac94ff to 1e58f7c Compare June 12, 2019 17:00

Geod24 reviewed Jun 12, 2019

View reviewed changes

test/compilable/cppmagle_abitag.d Outdated Show resolved Hide resolved

test/compilable/cppmagle_abitag.d Outdated Show resolved Hide resolved

test/compilable/cppmagle_abitag.d Outdated Show resolved Hide resolved

test/compilable/cppmagle_abitag.d Outdated Show resolved Hide resolved

SSoulaimane force-pushed the cpp_abi_tag branch 8 times, most recently from d9d43f7 to cca05a0 Compare June 16, 2019 17:13

ibuclaw reviewed Jun 19, 2019

View reviewed changes

src/dmd/attrib.d Show resolved Hide resolved

SSoulaimane force-pushed the cpp_abi_tag branch 4 times, most recently from 39bb395 to f3703cb Compare June 29, 2019 21:26

SSoulaimane added 3 commits July 15, 2019 13:16

Fix issue 19949 - C++ Mangling: add support for abi-tags from the Ita…

eeaa3f2

…nium ABI

Bash runnable tests

5ec82d8

Uses a bash test to get version information about the host C++ compiler.

Fix an infinite recursion were the return type is declared inside the…

4447ede

… function

SSoulaimane force-pushed the cpp_abi_tag branch from f3703cb to 4447ede Compare July 15, 2019 13:17

Geod24 mentioned this pull request Mar 15, 2020

Fix issue 19949: Implement support for C++ ABI tags #10927

Merged

Geod24 closed this Mar 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 19949 - C++ Mangling: add support for abi-tags from the Itanium ABI #9995

Fix issue 19949 - C++ Mangling: add support for abi-tags from the Itanium ABI #9995

SSoulaimane commented Jun 8, 2019 •

edited

Loading

jacob-carlborg Jun 8, 2019

SSoulaimane Jun 8, 2019

SSoulaimane Jun 10, 2019 •

edited

Loading

SSoulaimane Jun 12, 2019

jacob-carlborg Jun 8, 2019

SSoulaimane Jun 8, 2019

JinShil Jun 12, 2019 •

edited

Loading

jacob-carlborg Jun 12, 2019

SSoulaimane Jun 12, 2019 •

edited

Loading

jacob-carlborg Jun 12, 2019 •

edited

Loading

SSoulaimane Jun 12, 2019

WalterBright commented Jun 8, 2019

WalterBright commented Jun 8, 2019

SSoulaimane commented Jun 8, 2019

SSoulaimane commented Jun 8, 2019 •

edited

Loading

WalterBright commented Jun 8, 2019

jacob-carlborg Jun 10, 2019

SSoulaimane Jun 10, 2019

jacob-carlborg Jun 10, 2019

SSoulaimane Jun 10, 2019

SSoulaimane commented Jun 11, 2019

dlang-bot commented Jun 12, 2019 •

edited

Loading

jacob-carlborg Jun 12, 2019

SSoulaimane Jun 12, 2019

jacob-carlborg Jun 12, 2019

SSoulaimane Jun 12, 2019 •

edited

Loading

jacob-carlborg Jun 13, 2019

Geod24 commented Jun 12, 2019

Geod24 left a comment

SSoulaimane commented Jun 12, 2019 •

edited

Loading

Geod24 commented Jun 13, 2019

SSoulaimane commented Jun 13, 2019

Geod24 commented Jun 13, 2019

Geod24 commented Mar 28, 2020

Fix issue 19949 - C++ Mangling: add support for abi-tags from the Itanium ABI #9995

Fix issue 19949 - C++ Mangling: add support for abi-tags from the Itanium ABI #9995

Conversation

SSoulaimane commented Jun 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SSoulaimane Jun 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinShil Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SSoulaimane Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

jacob-carlborg Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WalterBright commented Jun 8, 2019

WalterBright commented Jun 8, 2019

SSoulaimane commented Jun 8, 2019

SSoulaimane commented Jun 8, 2019 • edited Loading

WalterBright commented Jun 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SSoulaimane commented Jun 11, 2019

dlang-bot commented Jun 12, 2019 • edited Loading

Bugzilla references

Testing this PR locally

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SSoulaimane Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geod24 commented Jun 12, 2019

Geod24 left a comment

Choose a reason for hiding this comment

SSoulaimane commented Jun 12, 2019 • edited Loading

Geod24 commented Jun 13, 2019

SSoulaimane commented Jun 13, 2019

Geod24 commented Jun 13, 2019

Geod24 commented Mar 28, 2020

SSoulaimane commented Jun 8, 2019 •

edited

Loading

SSoulaimane Jun 10, 2019 •

edited

Loading

JinShil Jun 12, 2019 •

edited

Loading

SSoulaimane Jun 12, 2019 •

edited

Loading

jacob-carlborg Jun 12, 2019 •

edited

Loading

SSoulaimane commented Jun 8, 2019 •

edited

Loading

dlang-bot commented Jun 12, 2019 •

edited

Loading

SSoulaimane Jun 12, 2019 •

edited

Loading

SSoulaimane commented Jun 12, 2019 •

edited

Loading