[compiler-rt] 2 nit Win32 patches

Clang Frontend
#1

Hi

If someone can give review and commit these 2 compiler-rt patches that would be great.

Patch 0 : compiler-rt-chkstk.patch - This allows building with recent mingw64 on Windows. We're hoping to get the build working and then possibly open a discussion on the benefits, if any, for more than a stub function.

Patch 1 : compiler-rt-hidden.patch - Per recommendation from a Windows dev it's not supported and we should disable it.

Thanks

./C

compiler-rt-chkstk.patch (675 Bytes)

compiler-rt-hidden.patch (1.6 KB)

#2

Hi

If someone can give review and commit these 2 compiler-rt patches that would
be great.

Patch 0 : compiler-rt-chkstk.patch - This allows building with recent
mingw64 on Windows. We're hoping to get the build working and then possibly
open a discussion on the benefits, if any, for more than a stub function.

Patch 1 : compiler-rt-hidden.patch - Per recommendation from a Windows dev
it's not supported and we should disable it.

diff --git a/lib/eprintf.c b/lib/eprintf.c
index b07d624..3626dbf 100644
--- a/lib/eprintf.c
+++ b/lib/eprintf.c
@@ -22,7 +22,9 @@
  * It should never be exported from a dylib, so it is marked
  * visibility hidden.
  */
+#ifndef _WIN32

What problem is this trying to solve with MinGW? I would imagine this
would be problematic with MSVC since it doesn't understand
__attribute__, but MinGW uses gcc under the hood.

__attribute__((visibility("hidden")))
+#endif
void __eprintf(const char* format, const char* assertion_expression,
  const char* line, const char* file)
{
diff --git a/lib/int_util.c b/lib/int_util.c
index 871d191..6d8922a 100644
--- a/lib/int_util.c
+++ b/lib/int_util.c
@@ -24,7 +24,9 @@
#ifdef KERNEL_USE

extern void panic(const char *, ...) __attribute__((noreturn));
+#ifndef _WIN32
__attribute__((visibility("hidden")))
+#endif
void compilerrt_abort_impl(const char *file, int line, const char *function) {
   panic("%s:%d: abort in %s", file, line, function);
}
@@ -35,8 +37,10 @@ void compilerrt_abort_impl(const char *file, int line, const char *function) {
extern void __assert_rtn(const char *func, const char *file,
                      int line, const char * message) __attribute__((noreturn));

+#ifndef _WIN32
__attribute__((weak))
__attribute__((visibility("hidden")))
+#endif
void compilerrt_abort_impl(const char *file, int line, const char *function) {
   __assert_rtn(function, file, line, "libcompiler_rt abort");
}
@@ -47,8 +51,10 @@ void compilerrt_abort_impl(const char *file, int line, const char *function) {
/* Get the system definition of abort() */
#include <stdlib.h>

+#ifndef _WIN32
__attribute__((weak))
__attribute__((visibility("hidden")))
+#endif
void compilerrt_abort_impl(const char *file, int line, const char *function) {
   abort();
}

I think a more clean approach would be to define a macro for the
__attribute__'s being #ifdef'ed out, and then use the macros. This is
the approach taken elsewhere in llvm (see Compiler.h).

~Aaron

#3

While not "clean" the intention is clear and only cosmetic - it's been applied and if you feel strongly that it's tooo fugly - Feel free to adjust, but please do so with with leaving it disable for mingw64 - There was an irc discussion about this and visibility isn't supported on Win and weak is buggy to the point of not being usable.

#4

Hi

If someone can give review and commit these 2 compiler-rt patches that would
be great.

Patch 0 : compiler-rt-chkstk.patch - This allows building with recent
mingw64 on Windows. We're hoping to get the build working and then possibly
open a discussion on the benefits, if any, for more than a stub function.

Patch 1 : compiler-rt-hidden.patch - Per recommendation from a Windows dev
it's not supported and we should disable it.

diff --git a/lib/eprintf.c b/lib/eprintf.c
index b07d624..3626dbf 100644
--- a/lib/eprintf.c
+++ b/lib/eprintf.c
@@ -22,7 +22,9 @@
  * It should never be exported from a dylib, so it is marked
  * visibility hidden.
  */
+#ifndef _WIN32

What problem is this trying to solve with MinGW? I would imagine this
would be problematic with MSVC since it doesn't understand
__attribute__, but MinGW uses gcc under the hood.

Answered my own question -- this isn't Patch 0. :wink: Derp.

My comments below stand with an addition -- this check should be
against _MSC_VER, not _WIN32.

#5

I don't feel too strongly, but I do feel that it's going to bite us at
some point because _WIN32 isn't the expected designator for
__attribute__ support given MinGW, cygwin, etc. The lack of comments
explaining that means people would have to find this email
conversation to understand the logic. Better to wrap it up cleanly,
with appropriate comments, to reduce the burden.

~Aaron

#6

In article <51E5B6DF.9090302@pathscale.com>,
    "C. Bergström" <cbergstrom@pathscale.com> writes:

Patch 1 : compiler-rt-hidden.patch - Per recommendation from a Windows
dev it's not supported and we should disable it.

I agree with the other comments; namely:

- __attribute__ is best worked around with a macro that expands into an
  __attribute__ decoration or nothing if the tool chain doesn't
  support it. Conditionalize the expansion by a test on the tool
  chain.

- Tests for the VC++ tool chain are best done with _MSC_VER and not _WIN32

#7

In article <CAAt6xTun0DrQSbAtzFHBDWJ3AN=nzSDonLeQRqJ30dSP_bft0w@mail.gmail.com>,
    Aaron Ballman <aaron@aaronballman.com> writes:

Better to wrap it up cleanly,
with appropriate comments, to reduce the burden.

Agreed.

#8

We're not testing with (MSVC) _MSC_VER - We also want to exclude recent MingW64 (since it's broken and what we're testing with)

#9

I may have missed this or misunderstood, but moving the "fix" outside of compiler-rt and depending on llvm/support is unwelcome. Currently compiler-rt builds fine standalone if you use the old old cmake files or maintain your own. /* Apologies if I misunderstood what you meant */

#10

I think it may be possible to achieve this using Compiler.h from
llvm\support, but I don't know the compiler-rt project well enough to
know if that's a viable suggestion or not. However,
LLVM_ATTRIBUTE_WEAK and LLVM_LIBRARY_VISIBILITY would do the trick
were they accessible.

~Aaron

#11

What if another compiler supports a non-standard mechanism to achieve the same/similar thing as a specific attribute? Best to define a macro for the intent, and then expand it to __attribute__, __declspec, or whatever else there is.

-Nico