cppmangle.d: Fix substitution logic in writeBasicType() by ibuclaw · Pull Request #9138 · dlang/dmd

ibuclaw · 2018-12-25T00:09:51Z

Specifically fixes nullptr_t and target-specific built-in type mangling (correcting patch in #9129) @JohanEngelen.

Later on, this will also work for char16_t and char32_t as well.

dlang-bot · 2018-12-25T00:09:52Z

Thanks for your pull request, @ibuclaw!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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

thewilsonator · 2018-12-25T03:38:46Z

runnable/extra-files/cppb.cpp:474:19: error: unknown type name 'nullptr_t'; did you mean 'std::nullptr_t'?
void testnullnull(nullptr_t n1, nullptr_t n2)
                  ^~~~~~~~~
                  std::nullptr_t

thewilsonator · 2018-12-25T03:53:13Z

Its also only defined in C++11 with all the fun dealing with that entails.
#9130 should make that doable without pulling hair.

WalterBright · 2018-12-25T09:35:10Z

+        if (auto tm = target.cppTypeMangle(t))
+        {
+            bool builtin = (t.ty == Tvoid || t.ty == Tbool || t.isintegral() || t.isreal());
+            writeBasicType(t, tm.toDString(), builtin);


If you need to do a strlen, I suggest having target.cppTypeMangle() return a string in the first place.

The function is implemented in c++, which doesn't know how D strings are passed around.

Can just drop the [] from writeBasicType instead.

( Reversing the order (first check target.cppTypeMangle before doing the hardcoded switch) is helpful: backends no longer need to modify dmdfe source to override the hardcoded mangles. Nice. )

This is how it was originally, but then it got refactored. This is just a partial revert to former behaviour.

WalterBright · 2018-12-25T09:37:39Z

I don't see where Tnull is handled in the code.

Most (all?) of the new code seems to be a refactoring, I'm not seeing where the fixes are. I'd prefer to see such significant refactoring done separately.

Also, the refactoring is doing a lot of replacing single character literals with strings. This looks slightly nicer, but comes at a cost in code bloat and speed. dmd has slowed down over time due to an accumulation of these sorts of things. I wrote the code the original way for speed reasons.

JohanEngelen · 2018-12-25T11:04:11Z

About performance: to me it looks like the new code is less performant (e.g. always doing a call to target.cppMangle where in most cases it won't be needed), but if mangling of C++ symbols is a measurable performance bottleneck I'd be very happy ;-) (what I mean is: we don't call these functions so many times, and dynamic alloc+resizing of the mangling string is probably the largest perf issue)

ibuclaw · 2018-12-27T20:39:58Z

I don't see where Tnull is handled in the code.

Tnull is mangled Dn.

Most (all?) of the new code seems to be a refactoring, I'm not seeing where the fixes are. I'd prefer to see such significant refactoring done separately.

This is a refactor. That it fixes two broken mangling cases is a side effect of this change.

Also, the refactoring is doing a lot of replacing single character literals with strings. This looks slightly nicer, but comes at a cost in code bloat and speed. dmd has slowed down over time due to an accumulation of these sorts of things. I wrote the code the original way for speed reasons.

Well speed or no, it was still producing bad symbols by assuming that all two character or greater mangles are non-fundamental types.

WalterBright · 2018-12-28T00:39:46Z

This is a refactor. That it fixes two broken mangling cases is a side effect of this change.

May I reiterate that fixing and refactoring should be different PRs. I can't find the fixes amid the refactorings. I know it's inconvenient for you, but the payoff is worth it, especially since the mangling code has been a prolific source of problems and regressions.

WalterBright · 2018-12-28T00:43:45Z

if mangling of C++ symbols is a measurable performance bottleneck I'd be very happy

I want to make sure it isn't. I haven't profiled it in years, but in the past it certainly has been measurable. An awful lot of symbols go through the mangler, although to be fair fewer of them go specifically through the C++ mangler.

ibuclaw · 2018-12-28T01:20:03Z

May I reiterate that fixing and refactoring should be different PRs. I can't find the fixes amid the refactorings. I know it's inconvenient for you, but the payoff is worth it, especially since the mangling code has been a prolific source of problems and regressions.

Other than removing the test, I don't see how. By forcing the caller to say whether they are a fundamental type or not just so happens to fix Tnull mangling, because when I looked at it, it turned out to be using writeBasicType inappropriately.

ibuclaw · 2018-12-29T19:10:26Z

Moved decision from caller of writeBasicType to callee.

Unlike the original patch that was questionably slower, this is definitely slower. :-)

Specifically fixes nullptr_t and target-specific mangling. Later on, this will also work for char16_t and char32_t as well.

Geod24 · 2018-12-30T01:20:14Z

 }

+/****************************************/
+/+ FIXME: Requires C++11 compiler.


I think the time has come to segment tests by C++ version.
You can create a separate file, let's say cppa_11.d, and use:

// EXTRA_CPP_SOURCES: cppa_11.cpp // CXXFLAGS: -std=c++11

To test C++11-specific behaviors

Nice idea though it can probably be done as another PR.

I don't think we should be rushing to merge PRs without any tests in them.
And it's not a large change to do either, moving a bit of code to a new file, so I really don't understand the hurry.

thewilsonator · 2018-12-30T12:35:30Z

Semaphore fails due to reproducible build differences.

ibuclaw · 2018-12-30T12:42:29Z

Semaphore fails due to reproducible build differences.

If only the diffing tool was smarter to actually pinpoint where it went wrong.

However, question...

Is it diffing host vs. newly built? Or newly built vs. bootstrap?

thewilsonator · 2018-12-30T12:46:35Z

(That was more a reminder to me not to bother to look again before merging it, not something that needs to be fixed.)

If only the diffing tool was smarter to actually pinpoint where it went wrong.

It would be nice.

Is it diffing host vs. newly built? Or newly built vs. bootstrap?

Don't know, don't really care. Reproducible build are nice in theory, but we're not building super secure software so it doesn't really matter.

ibuclaw · 2018-12-30T12:51:24Z

Don't know, don't really care.

Well, I ask because the former points to a bug in the CI script (it will fail if you make any ABI fixes), and the latter points to a bug introduced by this PR. :-)

thewilsonator · 2018-12-30T12:54:29Z

It happens frequently enough that I've lost count of the number of PRs I've merged that have had a similar issues, probably the tests fault, but as I said, its not a particularly meaningful test.

ibuclaw · 2018-12-30T13:47:42Z

I've just pulled the logs from semaphore, extracted all symbols from the diff and compared, and found nothing whatsoever.

Maybe what we're seeing is the infamous feature I've heard about where dmd backend has a built-in stopwatch, and will stop optimizing and skip straight to writing object code if it spends too long going through all passes.

thewilsonator · 2018-12-30T13:50:40Z

I've just pulled the logs from semaphore, extracted all symbols from the diff and compared, and found nothing whatsoever.

Hence why I ignore that test.

Maybe what we're seeing is the infamous feature I've heard about where dmd backend has a built-in stopwatch, and will stop optimizing and skip straight to writing object code if it spends too long going through all passes.

Maybe, all I know is that the SNR on that test is shockingly bad.

JohanEngelen · 2018-12-31T11:28:59Z

I'd very much appreciate enabling the C++11 tests.

Geod24 · 2019-01-03T00:19:54Z

I'd very much appreciate enabling the C++11 tests.

#9185

thewilsonator approved these changes Dec 25, 2018

View reviewed changes

WalterBright reviewed Dec 25, 2018

View reviewed changes

Comment thread src/dmd/cppmangle.d Outdated

WalterBright reviewed Dec 25, 2018

View reviewed changes

JohanEngelen reviewed Dec 25, 2018

View reviewed changes

Comment thread src/dmd/cppmangle.d Outdated

JohanEngelen reviewed Dec 25, 2018

View reviewed changes

Comment thread src/dmd/cppmangle.d Outdated

ibuclaw force-pushed the cppmangle2 branch 2 times, most recently from d4f55d4 to edf25f4 Compare December 27, 2018 20:29

ibuclaw force-pushed the cppmangle2 branch 2 times, most recently from 0da051d to 6461f11 Compare December 27, 2018 20:50

ibuclaw force-pushed the cppmangle2 branch 3 times, most recently from ae59c7a to 67fc6c2 Compare December 29, 2018 18:59

ibuclaw requested a review from MartinNowak as a code owner December 29, 2018 18:59

ibuclaw force-pushed the cppmangle2 branch 2 times, most recently from b472d0b to 19f1dfc Compare December 29, 2018 19:16

cppmangle.d: Fix substitution logic in writeBasicType()

131e694

Specifically fixes nullptr_t and target-specific mangling. Later on, this will also work for char16_t and char32_t as well.

ibuclaw force-pushed the cppmangle2 branch from 19f1dfc to 131e694 Compare December 29, 2018 21:13

Geod24 reviewed Dec 30, 2018

View reviewed changes

thewilsonator merged commit 7ae7a21 into dlang:master Dec 30, 2018

ibuclaw deleted the cppmangle2 branch December 31, 2018 09:05

JohanEngelen mentioned this pull request Dec 31, 2018

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

Closed

Geod24 mentioned this pull request Jan 3, 2019

Split C++11-specific tests in their own file #9185

Merged

Uh oh!

Conversation

ibuclaw commented Dec 25, 2018

Uh oh!

dlang-bot commented Dec 25, 2018

Bugzilla references

Testing this PR locally

Uh oh!

thewilsonator commented Dec 25, 2018

Uh oh!

thewilsonator commented Dec 25, 2018

Uh oh!

Uh oh!

WalterBright Dec 25, 2018

Choose a reason for hiding this comment

Uh oh!

ibuclaw Dec 25, 2018

Choose a reason for hiding this comment

Uh oh!

JohanEngelen Dec 25, 2018

Choose a reason for hiding this comment

Uh oh!

ibuclaw Dec 25, 2018

Choose a reason for hiding this comment

Uh oh!

WalterBright commented Dec 25, 2018

Uh oh!

JohanEngelen commented Dec 25, 2018

Uh oh!

Uh oh!

Uh oh!

ibuclaw commented Dec 27, 2018

Uh oh!

WalterBright commented Dec 28, 2018

Uh oh!

WalterBright commented Dec 28, 2018

Uh oh!

ibuclaw commented Dec 28, 2018

Uh oh!

ibuclaw commented Dec 29, 2018

Uh oh!

Geod24 Dec 30, 2018

Choose a reason for hiding this comment

Uh oh!

thewilsonator Dec 30, 2018

Choose a reason for hiding this comment

Uh oh!

Geod24 Dec 30, 2018

Choose a reason for hiding this comment

Uh oh!

thewilsonator commented Dec 30, 2018

Uh oh!

ibuclaw commented Dec 30, 2018

Uh oh!

thewilsonator commented Dec 30, 2018

Uh oh!

ibuclaw commented Dec 30, 2018

Uh oh!

thewilsonator commented Dec 30, 2018

Uh oh!

ibuclaw commented Dec 30, 2018

Uh oh!

thewilsonator commented Dec 30, 2018

Uh oh!

JohanEngelen commented Dec 31, 2018

Uh oh!

Geod24 commented Jan 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants