This is an archive of the discontinued LLVM Phabricator instance.

compiler-rt: add support for mingw-w64 in builtins
ClosedPublic

Authored by martell on Jul 9 2015, 2:19 PM.

Details

Summary

The is so that we can avoid using libgcc and use compiler-rt with mingw-w64

Related driver patch
http://reviews.llvm.org/D11077

I have tested this with mingw-w64 and everything seems to be in order.
I also sent this patch to the mingw-w64 mailing list for them to look at.

Added Peter as a reviewer as he seems to be the one who commits windows compiler-rt related code :)
Yaron has been commiting all the llvm clang patches for me but has never comitted to compiler-rt before and asked me to find someone else to do this
So I have added Peter as a reviewer :)

Diff Detail

Repository
rL LLVM

Event Timeline

martell updated this revision to Diff 29390.Jul 9 2015, 2:19 PM
martell retitled this revision from to compiler-rt: add support for mingw-w64 in builtins.
martell updated this object.
martell added reviewers: yaron.keren, rnk.
martell added inline comments.Jul 9 2015, 2:25 PM
lib/builtins/CMakeLists.txt
275 ↗(On Diff #29390)

The only issue I have with my own patch is that I don't know how clang identifies itself to cmake for MSVC targets.
is MSVC still true when no using the cl frontend ?

It might be more correct to change this to

if(NOT WIN32 OR MINGW)

rnk accepted this revision.Jul 10 2015, 8:49 AM
rnk edited edge metadata.

lgtm

lib/builtins/CMakeLists.txt
275 ↗(On Diff #29390)

It should depend on the default triple.
If you build clang with mingw, it will use the GNU ABI by default (#define GNUC, _Z C++ names, etc).
If you build it with MSVC, it will use the MSVC ABI (#define _MSC_VER, '?' C++ names, etc).

So, for your purposes, this patch is probably fine. :)

lib/builtins/enable_execute_stack.c
53 ↗(On Diff #29390)

Given that we don't check the return value of mprotect either, it's probably OK to simply ignore failure.

lib/builtins/x86_64/chkstk.S
27 ↗(On Diff #29390)

s/ecx/rcx/

This revision is now accepted and ready to land.Jul 10 2015, 8:49 AM
martell updated this revision to Diff 29478.Jul 10 2015, 1:05 PM
martell edited edge metadata.

s/ecx/rcx
good spot :)

ping? can someone commit this ?

martell updated this revision to Diff 29593.Jul 13 2015, 12:06 PM
martell added a reviewer: compnerd.

compnerd highlighted the fact that this breaks itanium so I updated the patch to fix that

martell updated this object.Jul 13 2015, 12:14 PM
martell removed reviewers: compnerd, yaron.keren.
martell added subscribers: compnerd, yaron.keren, pcc.
martell updated this object.
martell removed a subscriber: pcc.

Added cfe-commits so that people can actually see the patch on the lists :)
Hopefully someone will consider merging this

As phab suggests, you probably want to restart the review if you didn't add
a community list at the time of creation. Phab only sends the initial
summary email with details and attached patch on that first mailing so the
list still has no context here.

This revision was automatically updated to reflect the committed changes.
rnk added a comment.Jul 17 2015, 9:25 AM

I went ahead and landed the patch. I missed the part of the message where you mentioned that Yaron wasn't going to land it. Thanks!

martell added a comment.EditedJul 17 2015, 9:28 AM

Hi rnk thanks for landing it.

its missing the 2 chkstk.S files in the commit ?

rnk added a comment.Jul 17 2015, 9:32 AM

Hi rnk thanks for landing it.

its missing the 2 chkstk.S files in the commit ?

Woops, r242540.

Hi!
So apparently LLVM lowers alloca's of large buffers into a call to _alloca (makes sense I guess - if the buffer is larger than a page).
Why wasn't it added along with _chkstk? Is _alloca available from some other mingw library?