Skip to content
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.

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

Fix C++ mangling for repeated basic types that are target-specific. #9129

Merged
merged 1 commit into from
Dec 23, 2018

Conversation

JohanEngelen
Copy link
Contributor

See LDC issue: ldc-developers/ldc#2954

#7250 introduced substitution for target-specific basic types, which is helpful (e.g. with target specific Tcomplex80) but should only be done when multiple characters are used for the mangle. (So basically duplicating writeBasicType behavior).

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @JohanEngelen! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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

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 "stable + dmd#9129"

else
append(t);
// Only do substitution for mangles that are longer than 1 character.
if (tm[1] != 0 || t.isConst())
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I noticed this discrepancy with writeBasicType when submitting my ldc pull, traced both changes back to Walter's pull, saw he added both blocks, and figured that's what he wanted, since I didn't know what this substitution was about. I should have asked about it then.

@JohanEngelen
Copy link
Contributor Author

The Win32 failure looks like a spurious failure. Can someone retrigger please?

@joakim-noah
Copy link
Contributor

You can do it yourself by running git commit --amend, saving the same commit message with a different hash, then force pushing.

@wilzbach
Copy link
Member

Or just login at the Auto-Tester and deprecate the build (I just did so).

@dlang-bot dlang-bot merged commit 233af3c into dlang:stable Dec 23, 2018
@JohanEngelen JohanEngelen deleted the cpprealrealmangle branch December 24, 2018 00:58
@ibuclaw
Copy link
Member

ibuclaw commented Dec 24, 2018

I'm not sure about this.

@joakim-noah
Copy link
Contributor

BTW, one of the reasons @kinke and I didn't think this would break is that you seemed to be using this default case for GDC. This bug doesn't hit you already?

@ibuclaw
Copy link
Member

ibuclaw commented Dec 24, 2018

I'm not sure this is the correct fix.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 24, 2018

Examples that would fail the litmus test:

  • POWER's new 128-bit IEEE real type is mangled U10__float128.
  • IA64 reals are mangled u9__float80 on some configurations.
  • ARM half-float is mangled Dh (not that we support it :-)

@ibuclaw
Copy link
Member

ibuclaw commented Dec 24, 2018

Sadly, this whole block shouldn't even be in the default condition either. 128-bit long doubles won't get mangled as 'g' because of the Tfloat80 case.

@JohanEngelen
Copy link
Contributor Author

The __float128 on powerpc was already broken (because substitution would happen for all 'weird' types) while it should not be substituted according to gcc: https://cpp.godbolt.org/z/ufgbLO
Didn't check the other examples Iain posted.
I guess Target.cppTypeMangle should also set a flag whether to do substitution or not for that type. Here I assumed 2-or-more-chars requires substitution (because all that I could see need it), but there is at least one example now (U10__float128) that must not be substituted.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 24, 2018

On some further digging there are some non-target-specific examples.

  • char16_t is mangled Ds
  • char32_t is mangled Di
  • nullptr is mangled Dn

These would all go through the writeBasicType function, and should not be substituted either.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 24, 2018

The __float128 on powerpc was already broken (because substitution would happen for all 'weird' types) while it should not be substituted according to gcc: https://cpp.godbolt.org/z/ufgbLO

That compiler is far too old, I wouldn't bother trying to support it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants