Chia-I Wu <olva...@gmail.com> writes:
> On Mon, Oct 05, 2009 at 11:16:17AM -0600, tom fogal wrote:
> > Chia-I Wu <olva...@gmail.com> writes:
> > > 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.
While I agree MESA_DEBUG is useful to me, a user emailing me saying
that "your application crashed" gets me to the problem a lot quicker
than "the window stays white; nothing ever renders into it".
Seems like this is a matter of opinion at this point, so maybe we
should just let Brian / whoever will commit it decide.
> Also, MANGLE was not and is not supported here on non-x86 platforms
> because of glprocs.h.
Hrm? I might have recently broken our AIX support, then..
> > > +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.
Again, sounds like a decision not to be made by either of us.
Regardless, I think you're right that it belongs in a separate patch.
> > > - 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.
Sorry, I didn't mean to imply that you should fix the leak. I did mean
to imply that it would be nice if you could update the comment from
`possible memory leak' to `we might be leaking any dispatches that were
generated above by returning here w/o cleaning them up' (or similar).
> > 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.
I would bet a custom binary search would outperform any bsearch call,
because there's always going to be a need for a function pointer in the
latter. My preference is still on reusing the existing infrastructure,
until a demonstrated performance benefit justifies not doing so.
Anyway, regardless of which way some of these decisions go, this looks
good to me / your responses have convinced me. I've also applied it
and got one of my dynamic loading test programs to work.
Signed-off-by: Tom Fogal <tfo...@alumni.unh.edu>
FWIW.
-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
