[WIP] Fix mangling of extern(C++) function with two real arguments.#2953
[WIP] Fix mangling of extern(C++) function with two real arguments.#2953JohanEngelen wants to merge 3 commits intoldc-developers:masterfrom
real arguments.#2953Conversation
| if (cptr[1] != 0) | ||
| goto default; | ||
| c = cptr[0]; | ||
| break; |
There was a problem hiding this comment.
I think the better fix would be fixing the default case - checking for > 1 chars OR t.isConst(), that's what writeBasicType() does.
There was a problem hiding this comment.
The thing is: I don't know what the default case is there for and whether it is tested. So I don't want to muck with that. Perhaps it is needed to do substitutions for single char default-case types.
There was a problem hiding this comment.
I also don't understand the isConst check btw, because I haven't been able to trigger it (C++ mangling discards const on basic type parameters).
There was a problem hiding this comment.
In our case it's simple - the default case errors out if cppTypeMangle() returns null, and we're only handling this Tfloat80 case there at the moment. But if you're unsure about that > 1 chars rule, sure, leave as-is.
Btw, I've just seen that we don't handle Timaginary80 and Tcomplex80 specially for Android; pinging @joakim-noah.
There was a problem hiding this comment.
I've just seen that we don't handle Timaginary80 and Tcomplex80 specially for Android
Indeed, that's probably needed too, and then it makes more sense to have a general solution for the default case substitution problem...
There was a problem hiding this comment.
I guess you mean Android/x64. I considered those manglings of complex numbers, but figured this is such a niche platform, I didn't want to deal with it. I had to do real because building LDC itself needs it.
| @@ -0,0 +1,23 @@ | |||
| // Tests that repeated `real` return types are treated as built-in types in C++ mangling (no substitution). | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
But then I cannot test Windows, Linux, and Android on one single dev platform. Lit-tests are better for that.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Is real even guaranteed to link to C++ long double? Or must people always use c_long_double for that guarantee?
There was a problem hiding this comment.
Hmm good question. I would have assumed real to match long double for Windows/MSVC though, so that's probably a bug.
|
Upstream PR: dlang/dmd#9129 |
|
Lgtm, upstream too, thx. I added a fix for MSVC. |
| case Tuns64: | ||
| case Tint128: | ||
| case Tuns128: | ||
| version (LDC) {} else |
There was a problem hiding this comment.
nice fix.
But shouldn't these be IN_LLVM? (LDC is for detecting the host compiler, right?)
Mangle as native Visual C++ long double. DMD has to special-case, as it's using 80-bit extended precision.
kinke
force-pushed
the
realrealmangle
branch
from
a54898d to
31db68a
Compare
|
Upstream fix has been merged. All good? |
|
Yep, should be all good. - 1.13.1 would probably make sense, given that DMD cannot be built with 1.13.0. I'm on vacation though, not using my dev box, so my possibilities are somewhat limited. In any case, I'd wait a bit with 1.13.1, as there may be more issues popping up (I'd have expected some more issues wrt. shipped MinGW libs to pop up tbh). |
|
|
JohanEngelen
changed the title
real arguments.real arguments.
|
Needs updating according to: dlang/dmd#9138 and dlang/dmd#9185 |
|
I guess we can close this as there's probably not that much point in backporting the fix and releasing a v1.13.1 now. |
|
yeah, let's not spend time on this right now |
3 participants
The only mangling I am not sure of is the Windows mangling of
extern (C++) void withreal(real a, real b). @kinke Can you confirm the checkWINDOWS: define {{.*}}?withreal@@YAX_T0@Zmakes sense? The online demangler I tested with cannot demangle it; but I don't know what exactly we want to be doing forrealarguments on windows w.r.t. mangling.To be safe for some future platform where
realwould be mangled by more than one character, I forward to the old default behavior (including type mangling substitution, such as is done for other types whose mangling consists of multiple characters likecfloat).See also: https://forum.dlang.org/post/dsrjmpnavxqsjqmlvotf@forum.dlang.org