<div dir="ltr">I find ASAN and UBSAN extremely useful while debugging, but currently there is hundreds issues reported while running simple test. That make it very hard to use it. <div> </div><div>As Brian mention, first argument is never NULL. It can simplify changes while still make UBSAN happy. </div><div> </div><div>Roland, yes compiler will optimize it anyway, and will generate same code. </div><div>Please take a look at <a href="https://godbolt.org/g/JUzWyv">https://godbolt.org/g/JUzWyv</a></div><div>just uncomment //#define USE_SAFE and observe final asm output. </div><div> </div></div><div class="gmail_extra"> <div class="gmail_quote">On Wed, Feb 8, 2017 at 6:21 PM, Marek Olšák <<a href="mailto:maraeo@gmail.com" target="_blank">maraeo@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">FWIW, I'd like us to focus on real issues instead of trying to please static analyzers. If one of the reference functions stops working, we'll know that pretty quickly. Until then, there is nothing to do. Marek <div><div class="h5"> On Wed, Feb 8, 2017 at 6:12 PM, Roland Scheidegger <<a href="mailto:sroland@vmware.com">sroland@vmware.com</a>> wrote: > no_sanitize attribute looks like a reasonable solution to me (as long as > it still compiles everywhere, of course). > (But as said, since this is iffy, I'd be ok with changing the code too, > iff you can prove that compilers optimize this away.) > > Roland > > Am 08.02.2017 um 16:54 schrieb Bartosz Tomczyk: >> 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"))) ? >> >> On Wed, Feb 8, 2017 at 3:48 PM, Roland Scheidegger <<a href="mailto:sroland@vmware.com">sroland@vmware.com</a> >> <mailto:<a href="mailto:sroland@vmware.com">sroland@vmware.com</a>>> wrote: >> >> 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> >> <<a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__software.intel.com_en-2Dus_blogs_2015_04_20_null-2Dpointer-2Ddereferencing-2Dcauses-2Dundefined-2Dbehavior&d=DwMFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=OqTHFAhQukkS53Uth7VKFGc56MVU2St8mJ8J0uLq1Y8&s=ZWiQfyS52fxj0JPR5C-cJ5wN4H2ko91jFdFqgEdWlfE&e=" rel="noreferrer" target="_blank">https://urldefense.proofpoint.com/v2/url?u=https-3A__software.intel.com_en-2Dus_blogs_2015_04_20_null-2Dpointer-2Ddereferencing-2Dcauses-2Dundefined-2Dbehavior&d=DwMFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=OqTHFAhQukkS53Uth7VKFGc56MVU2St8mJ8J0uLq1Y8&s=ZWiQfyS52fxj0JPR5C-cJ5wN4H2ko91jFdFqgEdWlfE&e=</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 >> >> >> > >> > 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> >> <mailto:<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> >> <<a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=DwMFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=OqTHFAhQukkS53Uth7VKFGc56MVU2St8mJ8J0uLq1Y8&s=iDKajAoZZpzT_GhxToITEOrNT-iE1tTVWXaWKCNeTTo&e=" rel="noreferrer" target="_blank">https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=DwMFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=OqTHFAhQukkS53Uth7VKFGc56MVU2St8mJ8J0uLq1Y8&s=iDKajAoZZpzT_GhxToITEOrNT-iE1tTVWXaWKCNeTTo&e=</a>> >> >>> >> >> >> >> _______________________________________________ >> >> mesa-dev mailing list >> >> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> >> <mailto:<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> >> <<a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=DwMFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=OqTHFAhQukkS53Uth7VKFGc56MVU2St8mJ8J0uLq1Y8&s=iDKajAoZZpzT_GhxToITEOrNT-iE1tTVWXaWKCNeTTo&e=" rel="noreferrer" target="_blank">https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=DwMFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=OqTHFAhQukkS53Uth7VKFGc56MVU2St8mJ8J0uLq1Y8&s=iDKajAoZZpzT_GhxToITEOrNT-iE1tTVWXaWKCNeTTo&e=</a>> >> > >> > _______________________________________________ >> > mesa-dev mailing list >> > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <mailto:<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> >> <<a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=DwMFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=OqTHFAhQukkS53Uth7VKFGc56MVU2St8mJ8J0uLq1Y8&s=iDKajAoZZpzT_GhxToITEOrNT-iE1tTVWXaWKCNeTTo&e=" rel="noreferrer" target="_blank">https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=DwMFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=OqTHFAhQukkS53Uth7VKFGc56MVU2St8mJ8J0uLq1Y8&s=iDKajAoZZpzT_GhxToITEOrNT-iE1tTVWXaWKCNeTTo&e=</a>> >> >> > > _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> </div></div>> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div>