The no_sanitize attribute seems to be the most acceptable approach. Marek
On Wed, Feb 8, 2017 at 8:14 PM, Roland Scheidegger <srol...@vmware.com> wrote: > 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 <mar...@gmail.com >> <mailto:mar...@gmail.com>> 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 >> <srol...@vmware.com <mailto:srol...@vmware.com>> 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 >> <srol...@vmware.com <mailto:srol...@vmware.com> >> >> <mailto:srol...@vmware.com <mailto:srol...@vmware.com>>> 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 >> >> >>> mesa-dev@lists.freedesktop.org >> <mailto:mesa-dev@lists.freedesktop.org> >> >> <mailto:mesa-dev@lists.freedesktop.org >> <mailto:mesa-dev@lists.freedesktop.org>> >> >> >>> 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 >> >> >> mesa-dev@lists.freedesktop.org >> <mailto:mesa-dev@lists.freedesktop.org> >> >> <mailto:mesa-dev@lists.freedesktop.org >> <mailto:mesa-dev@lists.freedesktop.org>> >> >> >> 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 >> >> > mesa-dev@lists.freedesktop.org >> <mailto:mesa-dev@lists.freedesktop.org> >> <mailto:mesa-dev@lists.freedesktop.org >> <mailto:mesa-dev@lists.freedesktop.org>> >> >> > 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 >> > mesa-dev@lists.freedesktop.org <mailto:mesa-dev@lists.freedesktop.org> >> > 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 mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
