<div dir="ltr">I'm not insist to push it, although it looks like undefined behavior by C standard, all known compilers generates perfectly legal code for those construction. If there is strong objections about the patches, how about disabling UBSAN for those function with __attribute__((no_sanitize("undefined"))) ? </div><div class="gmail_extra"> <div class="gmail_quote">On Wed, Feb 8, 2017 at 3:48 PM, Roland Scheidegger <<a href="mailto:sroland@vmware.com" target="_blank">sroland@vmware.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Am 08.02.2017 um 10:21 schrieb Nicolai Hähnle: > On 07.02.2017 23:00, Brian Paul wrote: >> Yeah, it would never make sense to pass NULL as the first argument to >> any of the _reference() functions. >> >> Would putting an assert(ptr) at the start of pipe_surface_reference(), >> for example, silence the UBSAN warning? > > I think the point is that *ptr is NULL, and so (*ptr)->reference > "accesses a member within a NULL pointer". > > Yes, it's only for taking the address, but perhaps UBSAN doesn't care > about that? > > I'm not enough of a C language lawyer to know whether that's genuinely > undefined behavior or if this is an UBSAN false positive. Actually that seems to be a complicated question. This article here has a nice summary why it is undefined behavior: <a href="https://software.intel.com/en-us/blogs/2015/04/20/null-pointer-dereferencing-causes-undefined-behavior" rel="noreferrer" target="_blank">https://software.intel.com/en-us/blogs/2015/04/20/null-pointer-dereferencing-causes-undefined-behavior</a> Plus the comments why the article is wrong and it's perfectly defined... > > The patch as-is is overzealous (the pptr ? *pptr : NULL part is > unnecessary), but the part of replacing &(*ptr)->reference by *ptr ? > &(*ptr)->reference : NULL and similarly for surf may be warranted, and > is likely to be optimized away (though somebody should verify that...). So, if the compiler does optimize it away (with ordinary optimization switched on), I'm open to changes there. Otherwise I still don't think it's a good idea - basically the idea is that reference counting is used everywhere, so it should be as cheap as possible. (Note that this is used in a lot of places with an explicit NULL pointer as the second argument, and noone has ever claimed it caused actual issues.) Roland <div class="HOEnZb"><div class="h5"> > > Cheers, > Nicolai > >> -Brian >> >> >> On 02/07/2017 02:45 PM, Roland Scheidegger wrote: >>> I'm not quite sure there's really a bug here? >>> As far as I can tell, these functions are architected specifically to >>> look like that - they do not actually dereference potential null >>> pointers, but only take the address in the end. This change seems to add >>> some overhead. >>> >>> >>> Roland >>> >>> >>> Am 07.02.2017 um 19:34 schrieb Bartosz Tomczyk: >>>> --- >>>> src/gallium/auxiliary/util/u_inlines.h | 65 >>>> ++++++++++++++++++++-------------- >>>> 1 file changed, 39 insertions(+), 26 deletions(-) >>>> >>>> diff --git a/src/gallium/auxiliary/util/u_inlines.h >>>> b/src/gallium/auxiliary/util/u_inlines.h >>>> index b7b8313583..3bb3bcd6e0 100644 >>>> --- a/src/gallium/auxiliary/util/u_inlines.h >>>> +++ b/src/gallium/auxiliary/util/u_inlines.h >>>> @@ -104,14 +104,17 @@ pipe_reference(struct pipe_reference *ptr, >>>> struct pipe_reference *reference) >>>> } >>>> >>>> static inline void >>>> -pipe_surface_reference(struct pipe_surface **ptr, struct >>>> pipe_surface *surf) >>>> +pipe_surface_reference(struct pipe_surface **pptr, struct >>>> pipe_surface *surf) >>>> { >>>> - struct pipe_surface *old_surf = *ptr; >>>> + struct pipe_surface *ptr = pptr ? *pptr : NULL; >>>> + struct pipe_surface *old_surf = ptr; >>>> >>>> - if (pipe_reference_described(&(*ptr)->reference, &surf->reference, >>>> + if (pipe_reference_described(ptr ? &ptr->reference : NULL, >>>> + surf ? &surf->reference : NULL, >>>> >>>> (debug_reference_descriptor)debug_describe_surface)) >>>> old_surf->context->surface_destroy(old_surf->context, >>>> old_surf); >>>> - *ptr = surf; >>>> + >>>> + if (pptr) *pptr = surf; >>>> } >>>> >>>> /** >>>> @@ -121,37 +124,43 @@ pipe_surface_reference(struct pipe_surface >>>> **ptr, struct pipe_surface *surf) >>>> * that's shared by multiple contexts. >>>> */ >>>> static inline void >>>> -pipe_surface_release(struct pipe_context *pipe, struct pipe_surface >>>> **ptr) >>>> +pipe_surface_release(struct pipe_context *pipe, struct pipe_surface >>>> **pptr) >>>> { >>>> - if (pipe_reference_described(&(*ptr)->reference, NULL, >>>> + struct pipe_surface *ptr = pptr ? *pptr : NULL; >>>> + if (pipe_reference_described(ptr ? &ptr->reference : NULL, NULL, >>>> >>>> (debug_reference_descriptor)debug_describe_surface)) >>>> - pipe->surface_destroy(pipe, *ptr); >>>> - *ptr = NULL; >>>> + pipe->surface_destroy(pipe, ptr); >>>> + if (pptr) *pptr = NULL; >>>> } >>>> >>>> >>>> static inline void >>>> -pipe_resource_reference(struct pipe_resource **ptr, struct >>>> pipe_resource *tex) >>>> +pipe_resource_reference(struct pipe_resource **pptr, struct >>>> pipe_resource *tex) >>>> { >>>> - struct pipe_resource *old_tex = *ptr; >>>> + struct pipe_resource *ptr = pptr ? *pptr : NULL; >>>> + struct pipe_resource *old_tex = ptr; >>>> >>>> - if (pipe_reference_described(&(*ptr)->reference, &tex->reference, >>>> + if (pipe_reference_described(ptr ? &ptr->reference : NULL, >>>> + tex ? &tex->reference : NULL, >>>> >>>> (debug_reference_descriptor)debug_describe_resource)) { >>>> pipe_resource_reference(&old_tex->next, NULL); >>>> old_tex->screen->resource_destroy(old_tex->screen, old_tex); >>>> } >>>> - *ptr = tex; >>>> + >>>> + if (pptr) *pptr = tex; >>>> } >>>> >>>> static inline void >>>> -pipe_sampler_view_reference(struct pipe_sampler_view **ptr, struct >>>> pipe_sampler_view *view) >>>> +pipe_sampler_view_reference(struct pipe_sampler_view **pptr, struct >>>> pipe_sampler_view *view) >>>> { >>>> - struct pipe_sampler_view *old_view = *ptr; >>>> + struct pipe_sampler_view *ptr = pptr ? *pptr : NULL; >>>> + struct pipe_sampler_view *old_view = ptr; >>>> >>>> - if (pipe_reference_described(&(*ptr)->reference, &view->reference, >>>> + if (pipe_reference_described(ptr ? &ptr->reference : NULL, >>>> + view ? &view->reference : NULL, >>>> >>>> (debug_reference_descriptor)debug_describe_sampler_view)) >>>> old_view->context->sampler_view_destroy(old_view->context, >>>> old_view); >>>> - *ptr = view; >>>> + if (pptr) *pptr = view; >>>> } >>>> >>>> /** >>>> @@ -162,29 +171,33 @@ pipe_sampler_view_reference(struct >>>> pipe_sampler_view **ptr, struct pipe_sampler_ >>>> */ >>>> static inline void >>>> pipe_sampler_view_release(struct pipe_context *ctx, >>>> - struct pipe_sampler_view **ptr) >>>> + struct pipe_sampler_view **pptr) >>>> { >>>> - struct pipe_sampler_view *old_view = *ptr; >>>> - if (*ptr && (*ptr)->context != ctx) { >>>> + struct pipe_sampler_view *ptr = pptr ? *pptr : NULL; >>>> + struct pipe_sampler_view *old_view = ptr; >>>> + >>>> + if (ptr && ptr->context != ctx) { >>>> debug_printf_once(("context mis-match in >>>> pipe_sampler_view_release()\n")); >>>> } >>>> - if (pipe_reference_described(&(*ptr)->reference, NULL, >>>> + if (pipe_reference_described(ptr ? &ptr->reference : NULL, NULL, >>>> >>>> (debug_reference_descriptor)debug_describe_sampler_view)) { >>>> ctx->sampler_view_destroy(ctx, old_view); >>>> } >>>> - *ptr = NULL; >>>> + if(pptr) *pptr = NULL; >>>> } >>>> >>>> static inline void >>>> -pipe_so_target_reference(struct pipe_stream_output_target **ptr, >>>> +pipe_so_target_reference(struct pipe_stream_output_target **pptr, >>>> struct pipe_stream_output_target *target) >>>> { >>>> - struct pipe_stream_output_target *old = *ptr; >>>> + struct pipe_stream_output_target *ptr = pptr ? *pptr : NULL; >>>> + struct pipe_stream_output_target *old = ptr; >>>> >>>> - if (pipe_reference_described(&(*ptr)->reference, >>>> &target->reference, >>>> - >>>> (debug_reference_descriptor)debug_describe_so_target)) >>>> + if (pipe_reference_described(ptr ? &ptr->reference : NULL, >>>> + target ? &target->reference : NULL, >>>> + >>>> (debug_reference_descriptor)debug_describe_so_target)) >>>> old->context->stream_output_target_destroy(old->context, old); >>>> - *ptr = target; >>>> + if (pptr) *pptr = target; >>>> } >>>> >>>> static inline void >>>> >>> >>> _______________________________________________ >>> mesa-dev mailing list >>> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> >>> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> >>> >> >> _______________________________________________ >> mesa-dev mailing list >> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> >> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> </div>