On Mon, Oct 05, 2009 at 11:16:17AM -0600, tom fogal wrote:
> Chia-I Wu <olva...@gmail.com> writes:
> > The first two patches refactor glapi_getproc.c. They provide helper
> > functions to static dispatches and dynamic dispatches respectively,
> > and update the rests to use them. There is only one functional
> > change, the handling of MANGLE. Please see the comments in patch 1.
> Thanks for this; comments inline.
Thanks for the review. I will try to answer your concerns.
> > The third patch fixes a potential segfault. Calling a dynamic
> > dispatch which is not supported by a DRI driver would crash an
> > application. It is a bug of the application, since it should test
> > the corresponding extension before calling such functions. But it
> > is a simple fix to make such functions become no-op, just any static
> > function that is not supported by the DRI driver.
> IMHO, an app *should* segfault in this case. It's a big red "you have
> a bug" flag for developers, and I have definitely found it useful at
> times.
When glFooBar is never being SET_FooBar in mesa, it is dispatched to
generic_nop. The change is mainly to make a dynamic dispatch behave
exactly the same way. You can see the warnings when MESA_DEBUG is set.
Having it crash is helpful sometimes, but it is less OpenGL-ism IMHO.
> That said, I could definitely see the motivation for having a
> MESA_DISPATCH_NOOP environment variable, which causes the behavior you
> mention here.
> [snip]
> > The only real difference is in the handling of MANGLE. While
> > _glapi_add_dispatch ignored mangled name, add_function_name was
> > called with mangled name in _glapi_get_proc_address. It looks like a
> > bug, and is fixed.
> Yes; thanks. I've known about this for a month or two, but found it by
> looking at code and have not been able to devise a case where it can
> happen (the OSMesa path apparently does not hit most of this code). If
> you have a test for this...
glprocs.h and dynamic dispatches work with unmangled names internally.
I think it is reasonable to de-mangle the names in the entry points, and
let the internals always work with un-mangled names.
It was also how the code worked before. The thing is, add_function_name
should de-mangle the names but it didn't. The first patch fixes it
while refactoring. It is fixed because, otherwise, more code needs to
be written to reproduce a bug.
> > +#ifdef USE_X86_ASM
> > +extern const GLubyte gl_dispatch_functions_start[];
> > +#endif
> I'm a little concerned that something like this is changing. Your
> introductory comment doesn't mention anything about modifying anything
> to do with the dispatch itself; it seems like a table is changing from
> an unordered -> ordered, and an algorithm from linear search -> binary
> search. Thus changing the declaration type / decl. location of the
> table itself doesn't seem like it would happen.
There is no functional change here. The original code reads
-#ifdef USE_X86_ASM
-
-#if defined( GLX_USE_TLS )
-extern GLubyte gl_dispatch_functions_start[];
-extern GLubyte gl_dispatch_functions_end[];
-#else
-extern const GLubyte gl_dispatch_functions_start[];
-#endif
-
-#endif /* USE_X86_ASM */
Since gl_dispatch_functions_end is never used, and
gl_dispatch_functions_start is never modified, they are simplified.
> Not to say it's incorrect, just that I'd like to hear why something
> like this is changing, given the ostensible purpose of the patch
> series.
>
> > static const glprocs_table_t *
> > -find_entry( const char * n )
> > +_glapi_find_static_entry(const char *funcName)
> > {
> > GLuint i;
> > for (i = 0; static_functions[i].Name_offset >= 0; i++) {
> > const char *testName = gl_string_table +
> > static_functions[i].Name_offs
> > et;
> > -#ifdef MANGLE
> > - /* skip the "m" prefix on the name */
> > - if (strcmp(testName, n + 1) == 0)
> > -#else
> > - if (strcmp(testName, n) == 0)
> > -#endif
> > - {
> > - return &static_functions[i];
> > - }
> > + if (strcmp(testName, funcName) == 0)
> > + return &static_functions[i];
> The "MANGLE" case is missing here, no?
No. De-mangling is moved to _glapi_get_proc_address.
> > -#if !defined(XFree86Server) && !defined(XGLServer)
> > -#ifdef USE_X86_ASM
> > -
> > -#if defined( GLX_USE_TLS )
> > -extern GLubyte gl_dispatch_functions_start[];
> > -extern GLubyte gl_dispatch_functions_end[];
> > -#else
> > -extern const GLubyte gl_dispatch_functions_start[];
> > -#endif
> > -
> > -#endif /* USE_X86_ASM */
> Likewise, again, with the `unexpected changes'.
They are moved and simplified. See above.
> > @@ -537,12 +539,14 @@ _glapi_get_proc_address(const char *funcName)
> > GLuint i;
> >
> > #ifdef MANGLE
> > - if (funcName[0] != 'm' || funcName[1] != 'g' || funcName[2] != 'l')
> > + if (funcName[0] != 'm')
> > return NULL;
> > -#else
> > + /* skip the "m" prefix on the name */
> > + funcName++;
> > +#endif
> > +
> > if (funcName[0] != 'g' || funcName[1] != 'l')
> > return NULL;
>
> I like the updated logic here.
>
> > Subject: [PATCH 2/4] glapi: Refactor dynamic dispatches.
> >
> > Provide helper functions for handling dynamic dispatches. There is no
> > functional change.
> [snip]
> > +static struct _glapi_function *
> > +_glapi_find_dynamic_entry(const char *funcName)
> > +{
> > + struct _glapi_function *entry = NULL;
> > + GLuint i;
> > + for (i = 0; i < _glapi_num_dynamic_functions; i++) {
> > + if (strcmp(_glapi_dynamic_functions[i].name, funcName) == 0) {
> > + entry = &_glapi_dynamic_functions[i];
> > + break;
> > + }
> > + }
> This function needs an #ifdef MANGLE case as well, no? Or are dynamic
> symbols never given the "m" prefix? If so, it seems like we're
> relegating responsibility for removing the prefix to the caller. I'm
> not sure I have a preference either way, but such a note should make an
> intro comment to the function.
The names are de-mangled in _glapi_get_proc_address.
I think it may be better to ignore name mangling. The internals (the
logic, glprocs.h, and extension_helper.h) do not work with nor encode
mangled names. Names are de-mangled early in the user-accessible
function, _glapi_get_proc_address.
Also, MANGLE was not and is not supported here on non-x86 platforms
because of glprocs.h.
> > /**
> > + * Update the dispatch offset and parameter signature of a dynamic entry.
> > + */
> > +static void
> > +_glapi_update_dynamic_entry(struct _glapi_function *entry, GLuint offset,
> > + const char *parameter_signature)
> > +{
> > + if (entry->parameter_signature)
> > + free((char *) entry->parameter_signature);
> If we need to cast away the constness of parameter_signature ... maybe
> parameter_signature should not point to something which is const.
The const-ness is a hint that no function should modify the value
directly. It can be modified only through the update function.
But I am fine either way. I just feel like to keep the patch simpler.
> > @@ -455,55 +509,40 @@ _glapi_add_dispatch( const char * const *
> > function_name
> [snip]
> > - if (strcmp(ExtEntryTable[j].name, function_names[i]) == 0) {
> > - /* The offset may be ~0 if the function name was added by
> > - * glXGetProcAddress but never filled in by the driver.
> > - */
> [snip]
> > - }
> > +
> > + entry[i] = _glapi_find_dynamic_entry(function_names[i]);
> Ahh. It seems like here we're /not/ handling MANGLE. But it was also
> not handled before, either.
See my response above.
> [snip]
> > - entry[i]->parameter_signature = str_dup(real_sig);
> > - fill_in_entrypoint_offset(entry[i]->dispatch_stub, offset);
> > - entry[i]->dispatch_offset = offset;
> > + if (entry[i] == NULL) {
> > + entry[i] = _glapi_add_dynamic_entry(function_names[i]);
> > + if (entry[i] == NULL) {
> > + /* FIXME: Possible memory leak here.
> > + */
> What's leaked? You've simply copied/maintained the existing comment;
> presumably you know what's going on here now and could expand on the
> comment.
The error is that, there might already be some dynamic dispatches
generated when we bail out. The generated dynamic dispatches are
useable. It can be considered a leak or not.
This patch is about refactoring. I do not intend to introduce
functional changes if possible. If I do, I will try to document it.
> > + return -1;
> > + }
> > + }
> > + _glapi_update_dynamic_entry(entry[i], offset, real_sig);
> Should this be wrapped in an else? It seems like we want to either
> add or update, but the current case will do an update on every add.
> I guess the update should be a no-op, functionally, but it seems
> unnecessary and probably allocs memory (so is worth avoiding).
Dynamic dispatch generation is a two-stage process. The dispatch is
first generated, and it is then updated.
The second stage always happens here. The first stage might happen at
_glapi_get_proc_address.
>
> > Subject: [PATCH 3/4] glapi: Reserve last dynamic offset for generated stubs.
> >
> > The last dynamic offset is reserved for newly generated stubs. It fixes
> > segfaults when an application calls the stubs, while the DRI driver does
> > not provide them.
>
> As I clarified at the start, I like the segfault.
>
> [snip]
>
> > @@ -413,6 +419,8 @@ static void
> > _glapi_update_dynamic_entry(struct _glapi_function *entry, GLuint offset,
> > const char *parameter_signature)
> > {
> > + if (offset >= _GLAPI_LAST_DYNAMIC_OFFSET)
> > + return;
> > if (entry->parameter_signature)
> > free((char *) entry->parameter_signature);
>
> Not something in your code, but again: if we're always casting away the
> const, why const it in the first place?
>
> > Subject: [PATCH 4/4] glapi: Use binary search in _glapi_find_static_entry.
> >
> > This cuts down the extension initialization time of a DRI driver
> > dramatically when there are lots of functions.
> [snip]
>
> > + GLuint lo, hi;
> > +
> > + /* binary search */
> > + lo = 0;
> > + hi = ARRAY_SIZE(sorted_static_function_offsets);
> > + while (lo < hi) {
> > + const glprocs_table_t *entry;
> > + GLuint mid = (lo + hi) / 2;
> > + int res;
> > +
> > + entry = &static_functions[sorted_static_function_offsets[mid]];
> > + res = strcmp(_glapi_static_entry_name(entry), funcName);
>
> This might be okay, but I don't want to scroll all the way up to check,
> especially since I might have snipped it ;) Anyway, either this strcmp
> should have a MANGLE case or _glapi_static_entry_name should always
> return "glWhatever" even when the symbol is "mglWhatever".
That would be the latter case.
> Secondly, any chance of using stdlib's bsearch here?
I just had a look at _mesa_bsearch. It seems bsearch is not universally
available? I think the inline version is also slightly faster.
> All in all, great work IMHO. Thanks!
--
Regards,
olv
------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Mesa3d-dev mailing list
Mesa3d-dev@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev
