Fix Issue 18955 - extern(C++) default struct mangling is overridden when interacting with a cppmangle = class template

[JinShil]

I'm not all that confident in this fix. If this isn't the right fix, please help me with some advise. Thanks.

[dlang-bot]

Thanks for your pull request, @JinShil!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	18955	normal	extern(C++) default struct mangling is overridden when interacting with a cppmangle = class template

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

[JinShil]

There's some strange difference between Appveyor and the auto-tester. If I adjust the test to make Appveyor pass, then auto-tester fails. If I adjust the test to make the auto-tester pass, then Appveyor fails. Can anyone explain the discrepancy?

[TurkeyMan]

I think this LGTM, but I didn't do it because I didn't understand the surrounding code enough to have confidence. It kinda looked like the original code was deliberate.

[JinShil]

auto-tester Win32_64 expects:
?test@@YAXAEBV?$basic_string@DU?$char_traits@D@std@@@std@@@Z

auto-tester Win32 expects:
?test@@YAXABV?$basic_string@std@DU?$char_traits@std@D@1@@std@@@Z

appveyor (Win32 - I think) expects:
?test@@YAXABV?$basic_string@DU?$char_traits@D@std@@@std@@@Z

There's no way I can satisfy that. Any idea why the auto-tester and appveyor expect different results?

[TurkeyMan]

Did you get those mangled names from MSVC? It's not like DMC vs MSVC proper is it?

[JinShil]

I took them from the error messages generated by the auto-tester and Appveyor. I don't know which C++ compilers they are employing.

[TurkeyMan]

I just tried MSVC2015, it emits:

// 64bit
?test@@YAXAEBV?$basic_string@DU?$char_traits@D@std@@@std@@@Z

//32bit
?test@@YAXABV?$basic_string@DU?$char_traits@D@std@@@std@@@Z

That's what we should expect...

[JinShil]

Thanks, Then it must be the auto-tester's Win32 host that's giving the wrong result. @braddr, Any ideas?

[rainers]

autotester's Win32 test is the only server building with dmc.

Looking at cppmanglewin.d, there's a flag IS_DMC showing that dmc's mangling is slightly different than VS. You'll have to version the check with version(CRuntime_DigitalMars).

[rainers]

auto-tester Win32 expects:
?test@@YAXABV?$basic_string@std@DU?$char_traits@std@D@1@@std@@@Z

This string doesn't demangle with UnDecorateSymbolName.

[JinShil]

auto-tester and Appveyor are all green now. Thank you, @rainers.

[PetarKirov]

DAutoTest failed, due to a timeout:

.generated/stable_dmd-2.081.1/dmd2/linux/bin64/dub build --compiler=.generated/stable_dmd-2.081.1/dmd2/linux/bin64/dmd --root=ddoc && \
	mv ddoc/ddoc_preprocessor .generated/ddoc_preprocessor
Fetching stdx-allocator 2.77.2 (getting selected version)...
Fetching libdparse 0.8.7 (getting selected version)...
Timeout was reached on handle DF7600
posix.mak:920: recipe for target '.generated/ddoc_preprocessor' failed

Restarting...

[PetarKirov]

@JinShil, can you rebase to restart DAutoTest?

[PetarKirov]

@rainers is this PR good to go now?

[rainers]

@rainers is this PR good to go now?

Looks reasonable, but I'm not too familiar with the code.

I wonder why the tests are limited to Windows (I know mangling for structs and classes only differs there, but something else might break on other platforms).

Wouldn't it be better to actually try linking against the respective C++ code?

[JinShil]

I wonder why the tests are limited to Windows

The bug was specific to Windows. I could add a test for other platforms, but I'm not knowledgeable enough to know if the mangling produced is correct, so I may add a test that is verifying an incorrect result.

Wouldn't it be better to actually try linking against the respective C++ code?

We need the C++ bindings created first. That's currently getting under way at dlang/druntime#2286.

[JinShil]

@TurkeyMan If you could write a test case that links to libstdc++, I'll incorporate it into the PR and extend the test to multiple platforms.

[rainers]

I was rather thinking of adjusting the definition in cppa.d and add something matching in cppb.cpp:, e.g. rainers@194cdc1

That also matches the definition in <string> for MSC, but dmc doesn't seem to be able to compile its own stl.

[TurkeyMan]

@JinShil Writing any tests that link to stdc++ is super hard, since there are a lot of C++ tests that all use different C++ compilers, and different STL's, and most of the machinery to make correct decisions isn't merged yet (see my recent PR's).
What Rainer says is the right thing to do... or the test you have here is fine... hopefully it'll be self-testing with proper C++ link tests pretty soon.

[thewilsonator]

@JinShil https://godbolt.org/z/6QZzsf using MSVC 2015

[TurkeyMan]

Why isn't this merged? Is something holding it up?

[PetarKirov]

@JinShil can you incorporate the tests suggested by @rainers?

[rainers]

@JinShil can you incorporate the tests suggested by @rainers?

Maybe it's simpler to merge this and I make rainers@194cdc1 another PR? It might expose some non-Windows issues, though.

[PetarKirov]

@rainers sounds good, in this case feel free to approve and merge this PR.