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

MinGW-based libs: Add wide entry points #427

Merged
merged 2 commits into from
Feb 17, 2020
Merged

Conversation

kinke
Copy link
Contributor

@kinke kinke commented Feb 5, 2020

So that the user/druntime can use the wide wmain / wWinMain C entry points too.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @kinke! 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

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.

@kinke
Copy link
Contributor Author

kinke commented Feb 5, 2020

I still need to test this locally. Pinging @rainers.

@kinke
Copy link
Contributor Author

kinke commented Feb 6, 2020

Tested wmain and wWinMain with a little C program, seems to work fine.

kinke added a commit to ldc-developers/mingw-w64-libs that referenced this pull request Feb 6, 2020
kinke added a commit to ldc-developers/mingw-w64-libs that referenced this pull request Feb 6, 2020
kinke added a commit to ldc-developers/mingw-w64-libs that referenced this pull request Feb 6, 2020
@rainers
Copy link
Member

rainers commented Feb 7, 2020

LGTM. You'll have to change of the name of the artifact to retrigger an actual build. We are skipping it if the artifact is already uploaded to dlang.org, see https://github.com/dlang/installer/blob/master/windows/build_mingw.bat#L6
While at it, I'd prefer to use a variable for the artifact name as in https://github.com/dlang/installer/blob/master/windows/build_lld.bat#L7 to only have to make a single change when updating to a new version.

@kinke
Copy link
Contributor Author

kinke commented Feb 8, 2020

Done. I also skip the 2nd compilation of msvcrt_stub.c for DLLs with _UNICODE now, because it makes no difference and is thus just a duplicate object in the library.

@rainers
Copy link
Member

rainers commented Feb 8, 2020

Thanks. I was just doing the same and wondered why pushing failed ;-)

BTW: I think it might be good to just put the VC redistributables into the installer, so probably best to add them here with something like:

rem copy VC redistributable DLLs to bin folders
md dmd2\windows\bin
copy %WINDIR%\SysWOW64\msvcr100.dll dmd2\windows\bin

md dmd2\windows\bin64
copy %WINDIR%\System32\msvcr100.dll dmd2\windows\bin64

That's hoping for them to be installed on Azure, though.

@kinke
Copy link
Contributor Author

kinke commented Feb 8, 2020

The runtime, not the redist ;) (edit: got it now, I confused it with the redist installer first) - yes, that's probably a good idea, that DLL is not even 1MB. But then please upgrade to msvcr120.dll; VS 2013 added a lot of C99 math functions IIRC.

@rainers
Copy link
Member

rainers commented Feb 8, 2020

But then please upgrade to msvcr120.dll; VS 2013 added a lot of C99 math functions IIRC.

Ok, I'll look into it. Adding the DLLs might also be better in a separate PR.

@rainers
Copy link
Member

rainers commented Feb 9, 2020

I've tried to run the druntime unittests with the mingw-DLLs, but this failed miserably for x86 due to exceptions not working. Do you remember what the status was?
IIRC it was an lld issue? Probably also related: #346 (comment)

The VC runtime initalizes some cookie with __security_init_cookie(); and uses __try {} __except. Did you try that, too?

@rainers
Copy link
Member

rainers commented Feb 9, 2020

but this failed miserably for x86 due to exceptions not working.

In contrast to MS link, LLD sets the "No structured exception handler" flag:
https://github.com/llvm-mirror/lld/blob/64b024a57c56c3528d6be3d14be5e3da42614a6f/COFF/Writer.cpp#L1364

If I clear that flag in the binary, exceptions seem to work.

@kinke
Copy link
Contributor Author

kinke commented Feb 9, 2020

I'm pretty sure everything's working for LDC, incl. the 32-bit druntime/Phobos unittests. SafeSEH gave me some headaches, but IIRC, I've only had to modify LLVM's compiler-rt libs (making asm files SafeSEH compatible, getting rid of the 'security cookie' by compiling with /GS-, and making sure there are no libcmt refs via /Zl...). I probably also use those flags for the druntime and Phobos C parts. And explicitly set /SAFESEH in the linker cmdline for x86.

@rainers
Copy link
Member

rainers commented Feb 9, 2020

dmd doesn't emit SAFESEH compatible object files (missing "@feat.00" and handler tables AFAICT), and lld complains about all of them if /SAFESEH is supplied. Fixing dmd won't be much fun, so it's simpler to make lld more compatible to MS link: #433

@kinke
Copy link
Contributor Author

kinke commented Feb 9, 2020

Ah yes, makes sense.

@kinke
Copy link
Contributor Author

kinke commented Feb 14, 2020

@rainers: Merging is probably up to you...

@rainers
Copy link
Member

rainers commented Feb 15, 2020

If we follow the idea of using github releases to publish the results (see #433 (comment)) the url to check for the existing artifact changes. It should be something like https://github.com/dlang/installer/releases/download/mingw-%MINGW_VER%-2/%ARTIFACT%

Should we try this?

@wilzbach
Copy link
Member

It should be something like https://github.com/dlang/installer/releases/download/mingw-%MINGW_VER%-2/%ARTIFACT%

Yep.

Should we try this?

I would say so, because we could as a future step automate the upload.

@kinke
Copy link
Contributor Author

kinke commented Feb 15, 2020

If we follow the idea of using github releases to publish the results

Finally... ;)

@rainers
Copy link
Member

rainers commented Feb 17, 2020

I have added a check for the artifact on an expected github release. If it passes I'll merge and create the release with the respective tag...

@rainers
Copy link
Member

rainers commented Feb 17, 2020

I would say so, because we could as a future step automate the upload.

How would the creation of the sig file work? I've now struggled to use the "D Language Foundation" certificate to do that, but we don't have these files in the build pipeline.

@rainers rainers merged commit 24a447d into dlang:master Feb 17, 2020
@kinke kinke deleted the mingw_wide branch February 17, 2020 09:48
@kinke
Copy link
Contributor Author

kinke commented Feb 17, 2020

Thx Rainer. Are the libs now in use already, i.e., can we move forward with dlang/druntime#2932, or is the sig file a problem?

@rainers
Copy link
Member

rainers commented Feb 17, 2020

It still needs an update for the installer, but the larger change might be to update dmd to use msvcrt120.lib.

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

Successfully merging this pull request may close these issues.

4 participants