Skip to content

Conversation

@RazvanN7
Copy link
Contributor

cc @TurkeyMan as you introduced this in the first place.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @RazvanN7! 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 coverage diff by visiting the details link of the codecov check)
  • 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

Auto-close Bugzilla Severity Description
22014 normal Wrong MSVC++ mangling of wchar_t

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#13540"

@thewilsonator
Copy link
Contributor

The issue is wrong and confuses D's wchar (which is C++'s char16_t) with wchar_t which is neither of those.
This PR and that issue should be closed as working as intended.

@thewilsonator
Copy link
Contributor

Hmm, I thought we had a wchar_t akin to c_long and friends: see dlang/druntime#2349 #8958

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

see comments

@ibuclaw
Copy link
Member

ibuclaw commented Jan 17, 2022

Agreed, wchar_t is 16-bit on Windows, not 32-bit, and we already have __c_wchar_t which does the correct mangling.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 17, 2022

The fix should instead go in druntime, the core.stdc.stddef module is not right for C++ compatibility.

version (Windows)
{
    ///
    alias wchar wchar_t;
}
else version (Posix)
{
    ///
    alias dchar wchar_t;
}

That should instead be something like.

version (Windows)
    enum __c_wchar_t : wchar;
else version (Posix)
    enum __c_wchar_t : dchar;
///
alias wchar_t = __c_wchar_t;

@ljmf00
Copy link
Member

ljmf00 commented Jan 17, 2022

Agreed, wchar_t is 16-bit on Windows, not 32-bit, and we already have __c_wchar_t which does the correct mangling.

Just for context, __c_wchar_t is hardcoded as a special TY in the compiler? Is it detected as TYwchar? This may affect DWARF generation, since we are using that TY for both types.

@maxhaton
Copy link
Member

Agreed, wchar_t is 16-bit on Windows, not 32-bit, and we already have __c_wchar_t which does the correct mangling.

Just for context, __c_wchar_t is hardcoded as a special TY in the compiler? Is it detected as TYwchar? This may affect DWARF generation, since we are using that TY for both types.

else if (id == Id.__c_wchar_t)

@maxhaton
Copy link
Member

not sure what the backend actually gets in that case, worth investigating.

@ljmf00
Copy link
Member

ljmf00 commented Jan 17, 2022

Agreed, wchar_t is 16-bit on Windows, not 32-bit, and we already have __c_wchar_t which does the correct mangling.

Just for context, __c_wchar_t is hardcoded as a special TY in the compiler? Is it detected as TYwchar? This may affect DWARF generation, since we are using that TY for both types.

else if (id == Id.__c_wchar_t)

I'm going to make a patch on the DWARF generator to fix the wrong behavior and discuss it there. I'm talking about the TY enum. If we have __c_wchar_t as a special type, we should distinguish it from TYwchar, because on DMC, the backend interprets TYwchar as wchar_t (from C), but on DMD, the backend interprets TYwchar as wchar. If we don't have any special TY for that, we need to hardcode the string on the backend. I didn't know about this case but the DWARF generator reports a different typedef for that type and the debugger can't understand it as wchar_t.

@RazvanN7 RazvanN7 closed this Jan 20, 2022
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.

6 participants