So, I'd be happy with either changing the code to check for null pointers or using no_sanitize attribute. But it looks like others might not agree...
Roland Am 08.02.2017 um 19:21 schrieb Bartosz Tomczyk: > 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. > > As Brian mention, first argument is never NULL. It can simplify changes > while still make UBSAN happy. > > Roland, yes compiler will optimize it anyway, and will generate same code. > Please take a look at https://godbolt.org/g/JUzWyv > <https://urldefense.proofpoint.com/v2/url?u=https-3A__godbolt.org_g_JUzWyv&d=DwMFaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=ojbENEzoHm_vr0NBczGfaefYjlXRWgjyEeA5P2Fad20&s=nJUAuTvZ9Uyp_5WTx9Fk7bdIyABDssOiC5qcPZTy07w&e=> > just uncomment //#define USE_SAFE and observe final asm output. > > > On Wed, Feb 8, 2017 at 6:21 PM, Marek Olšák <[email protected] > <mailto:[email protected]>> wrote: > > 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 > > On Wed, Feb 8, 2017 at 6:12 PM, Roland Scheidegger > <[email protected] <mailto:[email protected]>> 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 > <[email protected] <mailto:[email protected]> > >> <mailto:[email protected] <mailto:[email protected]>>> 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: > >> > > https://software.intel.com/en-us/blogs/2015/04/20/null-pointer-dereferencing-causes-undefined-behavior > > <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=ojbENEzoHm_vr0NBczGfaefYjlXRWgjyEeA5P2Fad20&s=zlOURfZFxmK2zVgrGtSzGuJuJTAuBy0QpU4hEYil8YA&e=> > >> > > <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= > > <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=>> > >> 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 > >> >>> [email protected] > <mailto:[email protected]> > >> <mailto:[email protected] > <mailto:[email protected]>> > >> >>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > <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=ojbENEzoHm_vr0NBczGfaefYjlXRWgjyEeA5P2Fad20&s=p-f7QQ_QHP5vyuF_HWZNYncb0xgyi-KX0R_PmH3z-Iw&e=> > >> > > <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= > > <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=>> > >> >>> > >> >> > >> >> _______________________________________________ > >> >> mesa-dev mailing list > >> >> [email protected] > <mailto:[email protected]> > >> <mailto:[email protected] > <mailto:[email protected]>> > >> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > <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=ojbENEzoHm_vr0NBczGfaefYjlXRWgjyEeA5P2Fad20&s=p-f7QQ_QHP5vyuF_HWZNYncb0xgyi-KX0R_PmH3z-Iw&e=> > >> > > <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= > > <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=>> > >> > > >> > _______________________________________________ > >> > mesa-dev mailing list > >> > [email protected] > <mailto:[email protected]> > <mailto:[email protected] > <mailto:[email protected]>> > >> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > <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=ojbENEzoHm_vr0NBczGfaefYjlXRWgjyEeA5P2Fad20&s=p-f7QQ_QHP5vyuF_HWZNYncb0xgyi-KX0R_PmH3z-Iw&e=> > >> > > <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= > > <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=>> > >> > >> > > > > _______________________________________________ > > mesa-dev mailing list > > [email protected] <mailto:[email protected]> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > <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=ojbENEzoHm_vr0NBczGfaefYjlXRWgjyEeA5P2Fad20&s=p-f7QQ_QHP5vyuF_HWZNYncb0xgyi-KX0R_PmH3z-Iw&e=> > > _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
