Re: [Mesa-dev] question about container_of

[Nicolai Hähnle]

On 20.04.2017 18:32, Emil Velikov wrote:

On 18 April 2017 at 13:55, Pekka Paalanen  wrote:

On Mon, 27 Feb 2017 13:26:11 +
Emil Velikov  wrote:


Hi Julien,

On 27 February 2017 at 12:08, Julien Isorce  wrote:

Hi,

Since 2012 commit ccff74971203b533bf16b46b49a9e61753f75e6c it is said:
"sample must be initialized, or else the result is undefined" in the
description of mesa/src/util/list.h::container_of .

But I can find a few places where it is used without initializing that
second parameter, i.e. like:

struct A a;
container_of(ptr, a, member);

Then I can add the "= NULL" but should it be just
container_of(ptr, struct A, member);
like in the kernel and some other places in mesa ?


Strictly peaking these are toolchain (ASAN iirc) bugs, since there is
no pointer deref, as we're doing pointer arithmetic.


Hi Emil,

that's what people would usually think. It used to work with GCC. Then
came Clang.


Afaict the general decision was to to merge the patch(es) since they
will make actual bugs stand out amongst the noise. In the long run,
it's better to fix the tool (ASAN/other) than trying to "fix" all the
cases in mesa and dozens of other projects. But until then patches are
safe enough ;-)

That's my take on it, at least.


It depends on how container_of() has been defined. For more details,
see e.g.:
https://cgit.freedesktop.org/wayland/wayland/commit/?id=a18e34417ba3fefeb81d891235e8ebf394a20a74

The comment in the code in Mesa is correct for the fallback
implementation it has. Maybe things rely on the fallback implementation
never being used when compiling with Clang.

Julien, some alternatives for container_of use a pointer, others expect
a type instead. I believe one must use the correct form.


Thanks for the correction Pekka - yes, the issue (to deref or not) is
implementation specific.
A 'minor' detail that I've missed.

Seems like we use the more prone one - having the second argument
being a pointer as, opposed to a type.
Perhaps we should reconsider and update mesa, sooner rather than later.


Can we use typeof in Mesa? It's not standard, but I think both GCC and 
Clang have it, and it makes for a more convenient version of 
container_of that is still correct. The big question is whether it works 
on other compilers.


If it doesn't work on all compilers we need to support, I agree 
switching to the macro with the type argument makes sense.


Cheers,
Nicolai




Thanks again!
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev




--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] question about container_of

[Kristian H. Kristensen]

Emil Velikov  writes:

> On 18 April 2017 at 13:55, Pekka Paalanen  wrote:
>> On Mon, 27 Feb 2017 13:26:11 +
>> Emil Velikov  wrote:
>>
>>> Hi Julien,
>>>
>>> On 27 February 2017 at 12:08, Julien Isorce  wrote:
>>> > Hi,
>>> >
>>> > Since 2012 commit ccff74971203b533bf16b46b49a9e61753f75e6c it is said:
>>> > "sample must be initialized, or else the result is undefined" in the
>>> > description of mesa/src/util/list.h::container_of .
>>> >
>>> > But I can find a few places where it is used without initializing that
>>> > second parameter, i.e. like:
>>> >
>>> > struct A a;
>>> > container_of(ptr, a, member);
>>> >
>>> > Then I can add the "= NULL" but should it be just
>>> > container_of(ptr, struct A, member);
>>> > like in the kernel and some other places in mesa ?
>>> >
>>> Strictly peaking these are toolchain (ASAN iirc) bugs, since there is
>>> no pointer deref, as we're doing pointer arithmetic.
>>
>> Hi Emil,
>>
>> that's what people would usually think. It used to work with GCC. Then
>> came Clang.
>>
>>> Afaict the general decision was to to merge the patch(es) since they
>>> will make actual bugs stand out amongst the noise. In the long run,
>>> it's better to fix the tool (ASAN/other) than trying to "fix" all the
>>> cases in mesa and dozens of other projects. But until then patches are
>>> safe enough ;-)
>>>
>>> That's my take on it, at least.
>>
>> It depends on how container_of() has been defined. For more details,
>> see e.g.:
>> https://cgit.freedesktop.org/wayland/wayland/commit/?id=a18e34417ba3fefeb81d891235e8ebf394a20a74
>>
>> The comment in the code in Mesa is correct for the fallback
>> implementation it has. Maybe things rely on the fallback implementation
>> never being used when compiling with Clang.
>>
>> Julien, some alternatives for container_of use a pointer, others expect
>> a type instead. I believe one must use the correct form.
>>
> Thanks for the correction Pekka - yes, the issue (to deref or not) is
> implementation specific.
> A 'minor' detail that I've missed.

The issue isn't whether it derefs the pointer or not. The undefined
version looks like this:

 #define wl_container_of(ptr, sample, member)
(__typeof__(sample))((char *)(ptr)  -
 ((char *)&(sample)->member - (char *)(sample)))

and the problem is that it expects an uninitialized variable (sample) to
have consistent values across two references ( (char *)&(sample)->member and
(char *)sample ), but referencing an uninitialized variable even once is
undefined behavior.

Kristian

> Seems like we use the more prone one - having the second argument
> being a pointer as, opposed to a type.
> Perhaps we should reconsider and update mesa, sooner rather than later.
>
> Thanks again!
> Emil
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] question about container_of

[Emil Velikov]

On 18 April 2017 at 13:55, Pekka Paalanen  wrote:
> On Mon, 27 Feb 2017 13:26:11 +
> Emil Velikov  wrote:
>
>> Hi Julien,
>>
>> On 27 February 2017 at 12:08, Julien Isorce  wrote:
>> > Hi,
>> >
>> > Since 2012 commit ccff74971203b533bf16b46b49a9e61753f75e6c it is said:
>> > "sample must be initialized, or else the result is undefined" in the
>> > description of mesa/src/util/list.h::container_of .
>> >
>> > But I can find a few places where it is used without initializing that
>> > second parameter, i.e. like:
>> >
>> > struct A a;
>> > container_of(ptr, a, member);
>> >
>> > Then I can add the "= NULL" but should it be just
>> > container_of(ptr, struct A, member);
>> > like in the kernel and some other places in mesa ?
>> >
>> Strictly peaking these are toolchain (ASAN iirc) bugs, since there is
>> no pointer deref, as we're doing pointer arithmetic.
>
> Hi Emil,
>
> that's what people would usually think. It used to work with GCC. Then
> came Clang.
>
>> Afaict the general decision was to to merge the patch(es) since they
>> will make actual bugs stand out amongst the noise. In the long run,
>> it's better to fix the tool (ASAN/other) than trying to "fix" all the
>> cases in mesa and dozens of other projects. But until then patches are
>> safe enough ;-)
>>
>> That's my take on it, at least.
>
> It depends on how container_of() has been defined. For more details,
> see e.g.:
> https://cgit.freedesktop.org/wayland/wayland/commit/?id=a18e34417ba3fefeb81d891235e8ebf394a20a74
>
> The comment in the code in Mesa is correct for the fallback
> implementation it has. Maybe things rely on the fallback implementation
> never being used when compiling with Clang.
>
> Julien, some alternatives for container_of use a pointer, others expect
> a type instead. I believe one must use the correct form.
>
Thanks for the correction Pekka - yes, the issue (to deref or not) is
implementation specific.
A 'minor' detail that I've missed.

Seems like we use the more prone one - having the second argument
being a pointer as, opposed to a type.
Perhaps we should reconsider and update mesa, sooner rather than later.

Thanks again!
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] question about container_of

[Pekka Paalanen]

On Mon, 27 Feb 2017 13:26:11 +
Emil Velikov  wrote:

> Hi Julien,
> 
> On 27 February 2017 at 12:08, Julien Isorce  wrote:
> > Hi,
> >
> > Since 2012 commit ccff74971203b533bf16b46b49a9e61753f75e6c it is said:
> > "sample must be initialized, or else the result is undefined" in the
> > description of mesa/src/util/list.h::container_of .
> >
> > But I can find a few places where it is used without initializing that
> > second parameter, i.e. like:
> >
> > struct A a;
> > container_of(ptr, a, member);
> >
> > Then I can add the "= NULL" but should it be just
> > container_of(ptr, struct A, member);
> > like in the kernel and some other places in mesa ?
> >  
> Strictly peaking these are toolchain (ASAN iirc) bugs, since there is
> no pointer deref, as we're doing pointer arithmetic.

Hi Emil,

that's what people would usually think. It used to work with GCC. Then
came Clang.

> Afaict the general decision was to to merge the patch(es) since they
> will make actual bugs stand out amongst the noise. In the long run,
> it's better to fix the tool (ASAN/other) than trying to "fix" all the
> cases in mesa and dozens of other projects. But until then patches are
> safe enough ;-)
> 
> That's my take on it, at least.

It depends on how container_of() has been defined. For more details,
see e.g.:
https://cgit.freedesktop.org/wayland/wayland/commit/?id=a18e34417ba3fefeb81d891235e8ebf394a20a74

The comment in the code in Mesa is correct for the fallback
implementation it has. Maybe things rely on the fallback implementation
never being used when compiling with Clang.

Julien, some alternatives for container_of use a pointer, others expect
a type instead. I believe one must use the correct form.


Thanks,
pq


pgphcLK2HUoB3.pgp
Description: OpenPGP digital signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] question about container_of

[Emil Velikov]

Hi Julien,

On 27 February 2017 at 12:08, Julien Isorce  wrote:
> Hi,
>
> Since 2012 commit ccff74971203b533bf16b46b49a9e61753f75e6c it is said:
> "sample must be initialized, or else the result is undefined" in the
> description of mesa/src/util/list.h::container_of .
>
> But I can find a few places where it is used without initializing that
> second parameter, i.e. like:
>
> struct A a;
> container_of(ptr, a, member);
>
> Then I can add the "= NULL" but should it be just
> container_of(ptr, struct A, member);
> like in the kernel and some other places in mesa ?
>
Strictly peaking these are toolchain (ASAN iirc) bugs, since there is
no pointer deref, as we're doing pointer arithmetic.

Afaict the general decision was to to merge the patch(es) since they
will make actual bugs stand out amongst the noise. In the long run,
it's better to fix the tool (ASAN/other) than trying to "fix" all the
cases in mesa and dozens of other projects. But until then patches are
safe enough ;-)

That's my take on it, at least.

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] question about container_of

[Julien Isorce]

Hi,

Since 2012 commit ccff74971203b533bf16b46b49a9e61753f75e6c it is said:
"sample must be initialized, or else the result is undefined" in the
description of mesa/src/util/list.h::container_of .

But I can find a few places where it is used without initializing that
second parameter, i.e. like:

struct A a;
container_of(ptr, a, member);

Then I can add the "= NULL" but should it be just
container_of(ptr, struct A, member);
like in the kernel and some other places in mesa ?

Thx
Julien
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev