Skip to content

[WIP] Fix mangling of extern(C++) function with two real arguments. #2953

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.

Sign up for GitHub

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

Closed
JohanEngelen wants to merge 3 commits into from
Closed

[WIP] Fix mangling of extern(C++) function with two real arguments. #2953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f471c10
Fix mangling of extern(C++) function with two `real` arguments.
JohanEngelen Dec 23, 2018
95f7692
Different fix.
JohanEngelen Dec 23, 2018
31db68a
Fix C++ mangling of `real` for MSVC targets
kinke Dec 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions dmd/cppmangle.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1220,10 +1220,14 @@ else
// Handle any target-specific basic types.
if (auto tm = Target.cppTypeMangle(t))
{
if (substitute(t))
return;
else
append(t);
// Only do substitution for mangles that are longer than 1 character.
if (tm[1] != 0 || t.isConst())
{
if (substitute(t))
return;
else
append(t);
}
CV_qualifiers(t);
buf.writestring(tm);
return;
Expand Down
12 changes: 12 additions & 0 deletions dmd/cppmanglewin.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,10 @@ public:
case Tuns64:
case Tint128:
case Tuns128:
version (IN_LLVM) {} else
{
case Tfloat80:
}
case Twchar:
if (checkTypeSaved(type))
return;
Expand Down Expand Up @@ -241,10 +244,19 @@ public:
break;
// unsigned int
case Tfloat80:
version (IN_LLVM)
{
// unlike DMD, LDC uses 64-bit `real` for Windows/MSVC targets,
// corresponding to MSVC++ long double
buf.writeByte('O'); // Visual C++ long double
}
else
{
if (flags & IS_DMC)
buf.writestring("_Z"); // DigitalMars long double
else
buf.writestring("_T"); // Intel long double
}
break;
case Twchar:
if (flags & IS_DMC)
Expand Down
23 changes: 23 additions & 0 deletions tests/codegen/mangling_real_real.d
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// Tests that repeated `real` return types are treated as built-in types in C++ mangling (no substitution).
Copy link
Copy Markdown
Member

@kinke kinke Dec 23, 2018

Choose a reason for hiding this comment

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

Extending a dmd-testsuite test (e.g., runnable/cppa.d) would be better, as that makes really sure linking C++ and D succeeds, instead of testing an assumption of a correct hardcoded mangle, which already proved not to work too well before (I converted some according tests upstream not too long ago).

Copy link
Copy Markdown
Member Author

@JohanEngelen JohanEngelen Dec 23, 2018

Choose a reason for hiding this comment

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

But then I cannot test Windows, Linux, and Android on one single dev platform. Lit-tests are better for that.

Copy link
Copy Markdown
Member

@kinke kinke Dec 23, 2018

Choose a reason for hiding this comment

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

Yep, but you're only testing an assumption, which may break with newer C++ ABIs, and at the same time exclude OSX and exotic other OS from being tested.

Copy link
Copy Markdown
Member Author

@JohanEngelen JohanEngelen Dec 23, 2018

Choose a reason for hiding this comment

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

Yeah, true. So I'll add it to cppa aswell then...

What to do for real on Windows? It won't match C++ long double, is that a bug or intended?

Copy link
Copy Markdown
Member Author

@JohanEngelen JohanEngelen Dec 23, 2018

Choose a reason for hiding this comment

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

Is real even guaranteed to link to C++ long double? Or must people always use c_long_double for that guarantee?

Copy link
Copy Markdown
Member

@kinke kinke Dec 23, 2018

Choose a reason for hiding this comment

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

Hmm good question. I would have assumed real to match long double for Windows/MSVC though, so that's probably a bug.


// REQUIRES: target_X86

// RUN: %ldc -mtriple=x86_64-linux -c -output-ll -of=%t.ll %s && FileCheck %s --check-prefix=LINUX < %t.ll
// RUN: %ldc -mtriple=x86_64-android -c -output-ll -of=%t.android.ll %s && FileCheck %s --check-prefix=ANDROID < %t.android.ll
// RUN: %ldc -mtriple=x86_64-windows -c -output-ll -of=%t.windows.ll %s && FileCheck %s --check-prefix=WINDOWS < %t.windows.ll

import core.stdc.config;

// LINUX: define {{.*}}Z8withrealee
// ANDROID: define {{.*}}Z8withrealgg
// WINDOWS: define {{.*}}?withreal@@YAXOO@Z
extern (C++) void withreal(real a, real b)
{
}

// LINUX: define {{.*}}Z15withclongdoubleee
// ANDROID: define {{.*}}Z15withclongdoublegg
// WINDOWS: define {{.*}}?withclongdouble@@YAXOO@Z
extern (C++) void withclongdouble(c_long_double a, c_long_double b)
{
}