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.
> 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.
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...
> +#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.
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?
> -#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'.
> @@ -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.
> /**
> + * 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.
> @@ -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.
[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.
> + 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).
> 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".
Secondly, any chance of using stdlib's bsearch here?
All in all, great work IMHO. Thanks!
-tom
------------------------------------------------------------------------------
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
