[PATCH] Use a zero-size array in the struct devres

[PATCH] Use a zero-size array in the struct devres

[Catalin Marinas]

Commit 9ac7849e35f705830f7b016ff272b0ff1f7ff759 adds the devres
structure containing a flexible unsigned long long array, with a
comment about the guaranteed alignment. According to the gcc manual,
flexible arrays are considered incomplete types and it is an error to
ask for their alignment. This patch makes data[] a zero-size array.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---

I came across this when trying to build kmemleak as the modified
container_of macro tries to get the size of the devres.data member and
sizeof cannot be applied to incomplete types.

 drivers/base/devres.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index e177c95..0d6c067 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -22,7 +22,7 @@ struct devres_node {
 struct devres {
 	struct devres_node		node;
 	/* -- 3 pointers */
-	unsigned long long		data[];	/* guarantee ull alignment */
+	unsigned long long		data[0];	/* guarantee ull alignment */
 };
 
 struct devres_group {

Re: [PATCH] Use a zero-size array in the struct devres

[Tejun Heo]

Hello, Catalin.

Catalin Marinas wrote:
> Commit 9ac7849e35f705830f7b016ff272b0ff1f7ff759 adds the devres
> structure containing a flexible unsigned long long array, with a
> comment about the guaranteed alignment. According to the gcc manual,
> flexible arrays are considered incomplete types and it is an error to
> ask for their alignment. This patch makes data[] a zero-size array.

The gcc manual says nothing about alignment.  What it says is sizeof()
doesn't work.  If flexible array doesn't honor the alignment of the
declared type, it would be useless for its designed purpose of
supporting *array* of dynamically-determined length.

> I came across this when trying to build kmemleak as the modified
> container_of macro tries to get the size of the devres.data member and
> sizeof cannot be applied to incomplete types.

If kmemleak's container_of cannot deal with the current struct devres,
it means it can't deal with flexible arrays at all.  Flexible array
being the standard as opposed to 0 size array gcc extension, I guess a
lot of people would prefer flexible array.

It would be best if kmemleak's container_of can be modified to work with
flexible arrays.  If that's not possible, I guess it needs wider
discussion to determine whether to ditch flexible array for kmemleak.

Thanks.

-- 
tejun

Re: [PATCH] Use a zero-size array in the struct devres

[Catalin Marinas]

Tejun,

Tejun Heo <htejun@gmail.com> wrote:
> Catalin Marinas wrote:
>> Commit 9ac7849e35f705830f7b016ff272b0ff1f7ff759 adds the devres
>> structure containing a flexible unsigned long long array, with a
>> comment about the guaranteed alignment. According to the gcc manual,
>> flexible arrays are considered incomplete types and it is an error to
>> ask for their alignment. This patch makes data[] a zero-size array.
>
> The gcc manual says nothing about alignment.  What it says is sizeof()
> doesn't work.  If flexible array doesn't honor the alignment of the
> declared type, it would be useless for its designed purpose of
> supporting *array* of dynamically-determined length.

Chapter 5.31 (http://gcc.gnu.org/onlinedocs/gcc/Alignment.html) states
that "it is an error to ask for the alignment of an incomplete type"
and flexible array members have incomplete type (according to ISO C99
as described in http://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html).

It sounds to me like the alignment of an incomplete type is not
guaranteed (as you can't even enquire about it, though I might be
wrong). This is probably dependent on the compiler as well - with
gcc-3.3 on x86, __alignof__(unsigned long long) is 8 but the
"offsetof(struct test, data)" below is 12 (and it doesn't make any
difference whether it is a 0-size array or not):

struct test {
    unsigned long a;
    unsigned long b;
    unsigned long c;
    unsigned long long data[];
};

>> I came across this when trying to build kmemleak as the modified
>> container_of macro tries to get the size of the devres.data member and
>> sizeof cannot be applied to incomplete types.
>
> If kmemleak's container_of cannot deal with the current struct devres,
> it means it can't deal with flexible arrays at all.  Flexible array
> being the standard as opposed to 0 size array gcc extension, I guess a
> lot of people would prefer flexible array.

I'm OK with flexible arrays (kmemleak uses them as well) but not as a
member of structure getting passed to the container_of macro.

I did a quick grep through the kernel and it looks like Linux mainly
uses 0-size rather than flexible arrays at the end of a structure
(>500 vs ~14). This might be for historical reasons but it's not a big
issue in modifying them.

> It would be best if kmemleak's container_of can be modified to work with
> flexible arrays.  If that's not possible, I guess it needs wider
> discussion to determine whether to ditch flexible array for
> kmemleak.

Kmemleak needs the sizeof the member passed to container_of and I'm
not aware of any __builtin_* predicate that would detect incomplete
types.

-- 
Catalin

Re: [PATCH] Use a zero-size array in the struct devres

[Tejun Heo]

Catalin Marinas wrote:
> Chapter 5.31 (http://gcc.gnu.org/onlinedocs/gcc/Alignment.html) states
> that "it is an error to ask for the alignment of an incomplete type"
> and flexible array members have incomplete type (according to ISO C99
> as described in http://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html).
> 
> It sounds to me like the alignment of an incomplete type is not
> guaranteed (as you can't even enquire about it, though I might be
> wrong). This is probably dependent on the compiler as well - with
> gcc-3.3 on x86, __alignof__(unsigned long long) is 8 but the
> "offsetof(struct test, data)" below is 12 (and it doesn't make any
> difference whether it is a 0-size array or not):
> 
> struct test {
>     unsigned long a;
>     unsigned long b;
>     unsigned long c;
>     unsigned long long data[];
> };

data[0] and data[1] or whatever will also give you offset of 12.  Dunno
why, but it is.  Anyways, whatever the wording in the manual is,
flexible arrays just have to have the required alignment to do its job
as advertised.  :-)

>>> I came across this when trying to build kmemleak as the modified
>>> container_of macro tries to get the size of the devres.data member and
>>> sizeof cannot be applied to incomplete types.
>> If kmemleak's container_of cannot deal with the current struct devres,
>> it means it can't deal with flexible arrays at all.  Flexible array
>> being the standard as opposed to 0 size array gcc extension, I guess a
>> lot of people would prefer flexible array.
> 
> I'm OK with flexible arrays (kmemleak uses them as well) but not as a
> member of structure getting passed to the container_of macro.
> 
> I did a quick grep through the kernel and it looks like Linux mainly
> uses 0-size rather than flexible arrays at the end of a structure
> (>500 vs ~14). This might be for historical reasons but it's not a big
> issue in modifying them.

I think it's mostly historical.  Flexible array is still a relatively
new thing.  I don't mind changing devres to zero sized array, but please
explain in the commit message and as a comment that the choice is for
kmemleak's container_of(), and cc Greg K-H as the change should probably
go through his tree.

Thanks.

-- 
tejun

Re: [PATCH] Use a zero-size array in the struct devres

[Catalin Marinas]

Tejun Heo <htejun@gmail.com> wrote:
> Catalin Marinas wrote:
>> It sounds to me like the alignment of an incomplete type is not
>> guaranteed (as you can't even enquire about it, though I might be
>> wrong). This is probably dependent on the compiler as well - with
>> gcc-3.3 on x86, __alignof__(unsigned long long) is 8 but the
>> "offsetof(struct test, data)" below is 12 (and it doesn't make any
>> difference whether it is a 0-size array or not):
>> 
>> struct test {
>>     unsigned long a;
>>     unsigned long b;
>>     unsigned long c;
>>     unsigned long long data[];
>> };
>
> data[0] and data[1] or whatever will also give you offset of 12.  Dunno
> why, but it is.  Anyways, whatever the wording in the manual is,
> flexible arrays just have to have the required alignment to do its job
> as advertised.  :-)

On ARM (with gcc-4) it gives 16 for both offsetof and sizeof.

>> I did a quick grep through the kernel and it looks like Linux mainly
>> uses 0-size rather than flexible arrays at the end of a structure
>> (>500 vs ~14). This might be for historical reasons but it's not a big
>> issue in modifying them.
>
> I think it's mostly historical.  Flexible array is still a relatively
> new thing.  I don't mind changing devres to zero sized array, but please
> explain in the commit message and as a comment that the choice is for
> kmemleak's container_of(), and cc Greg K-H as the change should probably
> go through his tree.

OK. I'll probably wait until I post a new version of kmemleak against
2.6.21-rcX.

Thanks.

-- 
Catalin