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