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

[Aaron Ballman]

On Wed, Jul 17, 2013 at 8:59 AM, "C. Bergström"
<cbergstrom at pathscale.com> wrote:
> On 07/17/13 07:57 PM, Aaron Ballman 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.
>>
>>>   __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).
>
> 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.

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