[Mesa-dev] [PATCH 5/5] osx: fix asm support on darwin

[Jon TURNEY]

On 19/06/2015 00:36, Julien Isorce wrote:
> On 18 June 2015 at 19:46, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 18 June 2015 at 06:53, Julien Isorce <julien.isorce at gmail.com> wrote:
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90908

I don't think it's necessary to create a bugzilla entry just to link to 
it in your patch.

A bit more commentary about what this patch is doing and why it is right 
would help review it.

This patch seems to be doing 2 things: Changing xform4.S to use 
assyntax.h, and turning on the use of asm for darwin, so might be best 
broken in two.

>>> diff --git a/src/mesa/x86-64/xform4.S b/src/mesa/x86-64/xform4.S
>>> index c185f62..17eb7fa 100644
>>> --- a/src/mesa/x86-64/xform4.S
>>> +++ b/src/mesa/x86-64/xform4.S
>>> @@ -24,14 +24,15 @@
>>>
>>>   #ifdef USE_X86_64_ASM
>>>
>>> +#include "x86/assyntax.h"
>>>   #include "matypes.h"
>>>
>>>   .text
>>>
>>>   .align 16
>>> -.globl _mesa_x86_64_cpuid
>>> -.hidden _mesa_x86_64_cpuid
>>> -_mesa_x86_64_cpuid:
>>> +GLOBL GLNAME(_mesa_x86_64_cpuid)
>>> +HIDDEN(_mesa_x86_64_cpuid)
>>> +GLNAME(_mesa_x86_64_cpuid):

Whilst the use of HIDDEN is necessary, I'm don't think there are any 
x86_64 platforms which use a leading underscore, so I'm not sure if 
using GLNAME makes sense in an x86_64 specific file.

>>> -
>>> +
>>> +#if defined (__ELF__) && defined (__linux__)
>>>   .section .rodata
>>> +#endif

This looks wrong.  I don't see why this is linux/ELF specific.

Perhaps something like:

#ifdef __APPLE__
.const
#else
.section .rodata
#endif

maybe that needs to be  conditional on __MACH__?
maybe that needs to explicitly use a .section directive with segment and 
section?

>>> --- a/src/mesa/x86/assyntax.h
>>> +++ b/src/mesa/x86/assyntax.h
>>> @@ -255,7 +255,7 @@
>>>   #endif /* ACK_ASSEMBLER */
>>>
>>>
>>> -#if defined(__QNX__) || defined(Lynx) || (defined(SYSV) ||
>> defined(SVR4)) && !defined(ACK_ASSEMBLER) || defined(__ELF__) ||
>> defined(__GNU__) || defined(__GNUC__) && !defined(__MINGW32__)
>>> +#if defined(__QNX__) || defined(Lynx) || (defined(SYSV) ||
>> defined(SVR4)) && !defined(ACK_ASSEMBLER) || defined(__ELF__) ||
>> defined(__GNU__) || defined(__GNUC__) && !defined(__MINGW32__) &&
>> !defined(__APPLE__)
>>>   #define GLNAME(a)      a
>>>   #else
>>>   #define GLNAME(a)      CONCAT(_,a)
>>
>> Considering that this is a fragile area in mesa, can you confirm what
>> kind of testing you've done ? Would be nice to avoid breaking things
>> in various subtle ways.
>>
>
> Very minimal tests: only glxgears and es2gears_x11, on osx.
> Also "make check" succeeds until it reaches glx-test which fails to build
> (link error). I need to submit a bug.

In my OSX dev enviroment (which I haven't used for a year :D), I have 
CFLAGS containing "-arch i386 -arch x86_64" (because that's what [1] 
says to use).   With this patch applied, that fails to build for i386.

That's a tricky problem to solve within the mesa build system.  I don't 
know if building for OSX i386 is useful anymore?

[1] http://xquartz.macosforge.org/trac/wiki/DeveloperInfo