[cfe-dev] [compiler-rt] 2 nit Win32 patches

[Aaron Ballman]

On Wed, Jul 17, 2013 at 8:57 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Tue, Jul 16, 2013 at 5:10 PM, "C. Bergström"
> <cbergstrom at pathscale.com> wrote:
>> 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.  ;-)  Derp.

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

>
>>  __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