[Mesa-dev] [PATCH v2] glx: fix crash with bad fbconfig

[Jeremy Huddleston Sequoia]

> On Jun 7, 2016, at 03:25, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> 
> On 31 May 2016 at 10:53, Tapani Pälli <tapani.palli at intel.com> wrote:
>> From: Daniel Czarnowski <daniel.czarnowski at intel.com>
>> 
>> GLX documentation states:
>>        glXCreateNewContext can generate the following errors: (...)
>>        GLXBadFBConfig if config is not a valid GLXFBConfig
>> 
>> Function checks if the given config is a valid config and sets proper
>> error code.
>> 
>> Fixes currently crashing glx-fbconfig-bad Piglit test.
>> 
>> v2: coding style cleanups (Emil, Topi)
>>    use DefaultScreen macro (Emil)
>> 
> Thanks for the update Topi. There is one question/suggestion for
> future work inline. But as is the patch is
> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
> 
> 
>> +   config_list = (struct glx_config **)
>> +      glXGetFBConfigs(dpy, screen, &list_size);
>> +
> Not 100% sure if we could/should reference the external symbols from
> within our implementation. I cannot give you concrete reason, yet with
> GLVND around the corner my gut isn't giving me a good feeling about
> this.
> 
> We have a similar case in glXCreateWindow (using
> glXGetVisualFromFBConfig for the GLX_APPLEGL path) and
> __glXGLVNDGetProcAddress(glXGetProcAddressARB).
> 
> Perhaps it's worth renaming those symbols (prefix with __) and using
> GLX_ALIAS ? Speaking of which...
> 
> Jeremy is __attribute__  alias working with Darwin ?

No, __attribute__((alias)) is not supported:

~ $ cat testalias.c 
int one = 1;
extern int anotherone __attribute__((alias("one")));

~ $ clang -c testalias.c 
testalias.c:2:38: error: only weak aliases are supported on darwin
extern int anotherone __attribute__((alias("one")));
                                     ^
1 error generated.

But there are options.  The main way that we accomplish this on darwin is by passing the list of aliases to the linker:

     -alias symbol_name alternate_symbol_name
                 Create an alias named alternate_symbol_name for the symbol symbol_name.  By
                 default the alias symbol has global visibility.  This option was previous the
                 -idef:indir option.

     -alias_list filename
                 The specified filename contains a list of aliases. The symbol name and its
                 alias are on one line, separated by whitespace.  Lines starting with # are
                 ignored.

However, it would be best to not have to maintain these data in two separate places for different toolchains.  Having this at the source code level next to the prototype or implementation is the most ideal option.  There are two source level ways to go: asm assignment and resolvers.  Resolvers only work for functions, and the asm path seems to fit in most cleanly with GLX_ALIAS, but I list examples of both here for completeness:

#if defined __APPLE__
#define ALIAS_HAX(name, aliasname) \
  asm(".text\n\t.globl _" # aliasname "\n\t_" # aliasname " = _" # name);
#else
#define ALIAS_HAX(name, aliasname) \
  extern __typeof (name) aliasname __attribute__ ((alias (#name)));
#endif

int one = 1;
int two(void) {
    return 2;
}

ALIAS_HAX(one, anotherone)
ALIAS_HAX(two, anothertwo)

---

GLAPI void GLAPIENTRY glBegin( GLenum mode );
GLAPI void GLAPIENTRY glBeginRenamed( GLenum mode );

void * glBegin_resolver(void) __asm__("_glBeginRenamed");
void * glBegin_resolver(void) {
    __asm__(".symbol_resolver _glBeginRenamed");
    return &glBegin;
}

> If so can we drop
> the GLX_ALIAS_UNSUPPORTED define (automake and scons builds) and
> cleanup the ifdef spaghetti in src/glx/{glxcmds.c,glxextensions.h}.
> Just looking for an ack/nack on the idea :-)

I'm always in favor of cleaning up ifdef spaghetti, so you definitely have my ack!

> 
> Thanks
> Emil

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4465 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160608/23611746/attachment.bin>