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=>
