[PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Gustavo A. R. Silva]

The current codebase makes use of the zero-length array language
extension to the C90 standard, but the preferred mechanism to declare
variable-length types such as these ones is a flexible array member[1][2],
introduced in C99:

struct foo {
        int stuff;
        struct boo array[];
};

By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on.

Also, notice that, dynamic memory allocations won't be affected by
this change:

"Flexible array members have incomplete type, and so the sizeof operator
may not be applied. As a quirk of the original implementation of
zero-length arrays, sizeof evaluates to zero."[1]

This issue was found with the help of Coccinelle.

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")

Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/net/wireless/zydas/zd1211rw/zd_usb.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/zydas/zd1211rw/zd_usb.h b/drivers/net/wireless/zydas/zd1211rw/zd_usb.h
index a52ee323a142..8f03b09a602c 100644
--- a/drivers/net/wireless/zydas/zd1211rw/zd_usb.h
+++ b/drivers/net/wireless/zydas/zd1211rw/zd_usb.h
@@ -69,7 +69,7 @@ enum control_requests {
 
 struct usb_req_read_regs {
 	__le16 id;
-	__le16 addr[0];
+	__le16 addr[];
 } __packed;
 
 struct reg_data {
@@ -79,7 +79,7 @@ struct reg_data {
 
 struct usb_req_write_regs {
 	__le16 id;
-	struct reg_data reg_writes[0];
+	struct reg_data reg_writes[];
 } __packed;
 
 enum {
@@ -95,7 +95,7 @@ struct usb_req_rfwrite {
 	/* 2: other (default) */
 	__le16 bits;
 	/* RF2595: 24 */
-	__le16 bit_values[0];
+	__le16 bit_values[];
 	/* (ZD_CR203 & ~(RF_IF_LE | RF_CLK | RF_DATA)) | (bit ? RF_DATA : 0) */
 } __packed;
 
@@ -118,7 +118,7 @@ struct usb_int_header {
 
 struct usb_int_regs {
 	struct usb_int_header hdr;
-	struct reg_data regs[0];
+	struct reg_data regs[];
 } __packed;
 
 struct usb_int_retry_fail {
-- 
2.25.0

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Kalle Valo]

"Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:

> The current codebase makes use of the zero-length array language
> extension to the C90 standard, but the preferred mechanism to declare
> variable-length types such as these ones is a flexible array member[1][2],
> introduced in C99:
>
> struct foo {
>         int stuff;
>         struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on.
>
> Also, notice that, dynamic memory allocations won't be affected by
> this change:
>
> "Flexible array members have incomplete type, and so the sizeof operator
> may not be applied. As a quirk of the original implementation of
> zero-length arrays, sizeof evaluates to zero."[1]
>
> This issue was found with the help of Coccinelle.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/net/wireless/zydas/zd1211rw/zd_usb.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

"zd1211rw: " is enough, no need to have the filename in the title.

But I asked this already in an earlier patch, who prefers this format?
It already got opposition so I'm not sure what to do.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Joe Perches]

On Thu, 2020-03-05 at 16:50 +0200, Kalle Valo wrote:
> "Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:
[]
> >  drivers/net/wireless/zydas/zd1211rw/zd_usb.h | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> "zd1211rw: " is enough, no need to have the filename in the title.
> 
> But I asked this already in an earlier patch, who prefers this format?
> It already got opposition so I'm not sure what to do.

I think it doesn't matter.

Trivial inconsistencies in patch subject and word choice
don't have much overall impact.

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Kalle Valo]

Joe Perches <joe@perches.com> writes:

> On Thu, 2020-03-05 at 16:50 +0200, Kalle Valo wrote:
>> "Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:
> []
>> >  drivers/net/wireless/zydas/zd1211rw/zd_usb.h | 8 ++++----
>> >  1 file changed, 4 insertions(+), 4 deletions(-)
>> 
>> "zd1211rw: " is enough, no need to have the filename in the title.

>> But I asked this already in an earlier patch, who prefers this format?
>> It already got opposition so I'm not sure what to do.
>
> I think it doesn't matter.
>
> Trivial inconsistencies in patch subject and word choice
> don't have much overall impact.

I wrote in a confusing way, my question above was about the actual patch
and not the the title. For example, Jes didn't like this style change:

https://patchwork.kernel.org/patch/11402315/

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Gustavo A. R. Silva]

Hi,

On 3/5/20 10:10, Kalle Valo wrote:
> Joe Perches <joe@perches.com> writes:
> 
>> On Thu, 2020-03-05 at 16:50 +0200, Kalle Valo wrote:
>>> "Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:
>> []
>>>>  drivers/net/wireless/zydas/zd1211rw/zd_usb.h | 8 ++++----
>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> "zd1211rw: " is enough, no need to have the filename in the title.
> 
>>> But I asked this already in an earlier patch, who prefers this format?
>>> It already got opposition so I'm not sure what to do.
>>
>> I think it doesn't matter.
>>
>> Trivial inconsistencies in patch subject and word choice
>> don't have much overall impact.
> 
> I wrote in a confusing way, my question above was about the actual patch
> and not the the title. For example, Jes didn't like this style change:
> 
> https://patchwork.kernel.org/patch/11402315/
> 

It doesn't seem that that comment adds a lot to the conversation. The only
thing that it says is literally "fix the compiler". By the way, more than
a hundred patches have already been applied to linux-next[1] and he seems
to be the only person that has commented such a thing. Qemu guys are adopting
this format, too[2][3].

On the other hand, the changelog text explains the reasons why we are
implementing this change all across the kernel tree. :)

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/?qt=grep&q=flexible-array
[2] https://lists.nongnu.org/archive/html/qemu-s390x/2020-03/msg00019.html
[3] https://lists.nongnu.org/archive/html/qemu-s390x/2020-03/msg00020.html

Thanks
--
Gustavo

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Kalle Valo]

+ jes

"Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:

> Hi,
>
> On 3/5/20 10:10, Kalle Valo wrote:
>> Joe Perches <joe@perches.com> writes:
>> 
>>> On Thu, 2020-03-05 at 16:50 +0200, Kalle Valo wrote:
>>>> "Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:
>>> []
>>>>>  drivers/net/wireless/zydas/zd1211rw/zd_usb.h | 8 ++++----
>>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> "zd1211rw: " is enough, no need to have the filename in the title.
>> 
>>>> But I asked this already in an earlier patch, who prefers this format?
>>>> It already got opposition so I'm not sure what to do.
>>>
>>> I think it doesn't matter.
>>>
>>> Trivial inconsistencies in patch subject and word choice
>>> don't have much overall impact.
>> 
>> I wrote in a confusing way, my question above was about the actual patch
>> and not the the title. For example, Jes didn't like this style change:
>> 
>> https://patchwork.kernel.org/patch/11402315/
>> 
>
> It doesn't seem that that comment adds a lot to the conversation. The only
> thing that it says is literally "fix the compiler". By the way, more than
> a hundred patches have already been applied to linux-next[1] and he seems
> to be the only person that has commented such a thing.

But I also asked who prefers this format in that thread, you should not
ignore questions from two maintainers (me and Jes).

> Qemu guys are adopting this format, too[2][3].
>
> On the other hand, the changelog text explains the reasons why we are
> implementing this change all across the kernel tree. :)
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/?qt=grep&q=flexible-array
> [2] https://lists.nongnu.org/archive/html/qemu-s390x/2020-03/msg00019.html
> [3] https://lists.nongnu.org/archive/html/qemu-s390x/2020-03/msg00020.html

TBH I was leaning more on Jes side on this, but I guess these patches
are ok if they are so widely accepted. Unless anyone objects?

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Gustavo A. R. Silva]

On 3/10/20 8:56 AM, Kalle Valo wrote:
> + jes
> 
> "Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:
> 
>> Hi,
>>
>> On 3/5/20 10:10, Kalle Valo wrote:
>>> Joe Perches <joe@perches.com> writes:
>>>
>>>> On Thu, 2020-03-05 at 16:50 +0200, Kalle Valo wrote:
>>>>> "Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:
>>>> []
>>>>>>  drivers/net/wireless/zydas/zd1211rw/zd_usb.h | 8 ++++----
>>>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> "zd1211rw: " is enough, no need to have the filename in the title.
>>>
>>>>> But I asked this already in an earlier patch, who prefers this format?
>>>>> It already got opposition so I'm not sure what to do.
>>>>
>>>> I think it doesn't matter.
>>>>
>>>> Trivial inconsistencies in patch subject and word choice
>>>> don't have much overall impact.
>>>
>>> I wrote in a confusing way, my question above was about the actual patch
>>> and not the the title. For example, Jes didn't like this style change:
>>>
>>> https://patchwork.kernel.org/patch/11402315/
>>>
>>
>> It doesn't seem that that comment adds a lot to the conversation. The only
>> thing that it says is literally "fix the compiler". By the way, more than
>> a hundred patches have already been applied to linux-next[1] and he seems
>> to be the only person that has commented such a thing.
> 
> But I also asked who prefers this format in that thread, you should not
> ignore questions from two maintainers (me and Jes).
> 

I'm sorry. I thought the changelog text had already the proper information.
In the changelog text I'm quoting the GCC documentation below:

"The preferred mechanism to declare variable-length types like struct line
above is the ISO C99 flexible array member..." [1]

I'm also including a link to the following KSPP open issue:

https://github.com/KSPP/linux/issues/21

The issue above mentions the following:

"Both cases (0-byte and 1-byte arrays) pose confusion for things like sizeof(),
CONFIG_FORTIFY_SOURCE."

sizeof(flexible-array-member) triggers a warning because flexible array members have
incomplete type[1]. There are some instances of code in which the sizeof operator
is being incorrectly/erroneously applied to zero-length arrays and the result is zero.
Such instances may be hiding some bugs. So, the idea is also to get completely rid
of those sorts of issues.

Should I update the changelog in some way so it is a bit more informative?

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html

Thanks
--
Gustavo

>> Qemu guys are adopting this format, too[2][3].
>>
>> On the other hand, the changelog text explains the reasons why we are
>> implementing this change all across the kernel tree. :)
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/?qt=grep&q=flexible-array
>> [2] https://lists.nongnu.org/archive/html/qemu-s390x/2020-03/msg00019.html
>> [3] https://lists.nongnu.org/archive/html/qemu-s390x/2020-03/msg00020.html
> 
> TBH I was leaning more on Jes side on this, but I guess these patches
> are ok if they are so widely accepted. Unless anyone objects?
> 

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Jes Sorensen]

On 3/10/20 5:52 PM, Gustavo A. R. Silva wrote:
> 
> 
> On 3/10/20 8:56 AM, Kalle Valo wrote:
>> + jes
>>
>> "Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:
>>>> I wrote in a confusing way, my question above was about the actual patch
>>>> and not the the title. For example, Jes didn't like this style change:
>>>>
>>>> https://patchwork.kernel.org/patch/11402315/
>>>>
>>>
>>> It doesn't seem that that comment adds a lot to the conversation. The only
>>> thing that it says is literally "fix the compiler". By the way, more than
>>> a hundred patches have already been applied to linux-next[1] and he seems
>>> to be the only person that has commented such a thing.
>>
>> But I also asked who prefers this format in that thread, you should not
>> ignore questions from two maintainers (me and Jes).
>>
> 
> I'm sorry. I thought the changelog text had already the proper information.
> In the changelog text I'm quoting the GCC documentation below:
> 
> "The preferred mechanism to declare variable-length types like struct line
> above is the ISO C99 flexible array member..." [1]
> 
> I'm also including a link to the following KSPP open issue:
> 
> https://github.com/KSPP/linux/issues/21
> 
> The issue above mentions the following:
> 
> "Both cases (0-byte and 1-byte arrays) pose confusion for things like sizeof(),
> CONFIG_FORTIFY_SOURCE."
> 
> sizeof(flexible-array-member) triggers a warning because flexible array members have
> incomplete type[1]. There are some instances of code in which the sizeof operator
> is being incorrectly/erroneously applied to zero-length arrays and the result is zero.
> Such instances may be hiding some bugs. So, the idea is also to get completely rid
> of those sorts of issues.

As I stated in my previous answer, this seems more code churn than an
actual fix. If this is a real problem, shouldn't the work be put into
fixing the compiler to handle foo[0] instead? It seems that is where the
real value would be.

Thanks,
Jes

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Gustavo A. R. Silva]

On 3/10/20 5:07 PM, Jes Sorensen wrote:
> On 3/10/20 5:52 PM, Gustavo A. R. Silva wrote:
>>
>>
>> On 3/10/20 8:56 AM, Kalle Valo wrote:
>>> + jes
>>>
>>> "Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:
>>>>> I wrote in a confusing way, my question above was about the actual patch
>>>>> and not the the title. For example, Jes didn't like this style change:
>>>>>
>>>>> https://patchwork.kernel.org/patch/11402315/
>>>>>
>>>>
>>>> It doesn't seem that that comment adds a lot to the conversation. The only
>>>> thing that it says is literally "fix the compiler". By the way, more than
>>>> a hundred patches have already been applied to linux-next[1] and he seems
>>>> to be the only person that has commented such a thing.
>>>
>>> But I also asked who prefers this format in that thread, you should not
>>> ignore questions from two maintainers (me and Jes).
>>>
>>
>> I'm sorry. I thought the changelog text had already the proper information.
>> In the changelog text I'm quoting the GCC documentation below:
>>
>> "The preferred mechanism to declare variable-length types like struct line
>> above is the ISO C99 flexible array member..." [1]
>>
>> I'm also including a link to the following KSPP open issue:
>>
>> https://github.com/KSPP/linux/issues/21
>>
>> The issue above mentions the following:
>>
>> "Both cases (0-byte and 1-byte arrays) pose confusion for things like sizeof(),
>> CONFIG_FORTIFY_SOURCE."
>>
>> sizeof(flexible-array-member) triggers a warning because flexible array members have
>> incomplete type[1]. There are some instances of code in which the sizeof operator
>> is being incorrectly/erroneously applied to zero-length arrays and the result is zero.
>> Such instances may be hiding some bugs. So, the idea is also to get completely rid
>> of those sorts of issues.
> 
> As I stated in my previous answer, this seems more code churn than an
> actual fix. If this is a real problem, shouldn't the work be put into
> fixing the compiler to handle foo[0] instead? It seems that is where the
> real value would be.
> 

Yeah. But, unfortunately, I'm not a compiler guy, so I'm not able to fix the
compiler as you suggest. And I honestly don't see what is so annoying/disturbing
about applying a patch that removes the 0 from foo[0] when it brings benefit
to the whole codebase.

Thanks
--
Gustavo

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Joe Perches]

On Tue, 2020-03-10 at 17:13 -0500, Gustavo A. R. Silva wrote:
> 
> On 3/10/20 5:07 PM, Jes Sorensen wrote:
> > On 3/10/20 5:52 PM, Gustavo A. R. Silva wrote:
> > > 
> > > On 3/10/20 8:56 AM, Kalle Valo wrote:
> > > > + jes
> > > > 
> > > > "Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:
> > > > > > I wrote in a confusing way, my question above was about the actual patch
> > > > > > and not the the title. For example, Jes didn't like this style change:
> > > > > > 
> > > > > > https://patchwork.kernel.org/patch/11402315/
> > > > > > 
> > > > > 
> > > > > It doesn't seem that that comment adds a lot to the conversation. The only
> > > > > thing that it says is literally "fix the compiler". By the way, more than
> > > > > a hundred patches have already been applied to linux-next[1] and he seems
> > > > > to be the only person that has commented such a thing.
> > > > 
> > > > But I also asked who prefers this format in that thread, you should not
> > > > ignore questions from two maintainers (me and Jes).
> > > > 
> > > 
> > > I'm sorry. I thought the changelog text had already the proper information.
> > > In the changelog text I'm quoting the GCC documentation below:
> > > 
> > > "The preferred mechanism to declare variable-length types like struct line
> > > above is the ISO C99 flexible array member..." [1]
> > > 
> > > I'm also including a link to the following KSPP open issue:
> > > 
> > > https://github.com/KSPP/linux/issues/21
> > > 
> > > The issue above mentions the following:
> > > 
> > > "Both cases (0-byte and 1-byte arrays) pose confusion for things like sizeof(),
> > > CONFIG_FORTIFY_SOURCE."
> > > 
> > > sizeof(flexible-array-member) triggers a warning because flexible array members have
> > > incomplete type[1]. There are some instances of code in which the sizeof operator
> > > is being incorrectly/erroneously applied to zero-length arrays and the result is zero.
> > > Such instances may be hiding some bugs. So, the idea is also to get completely rid
> > > of those sorts of issues.
> > 
> > As I stated in my previous answer, this seems more code churn than an
> > actual fix. If this is a real problem, shouldn't the work be put into
> > fixing the compiler to handle foo[0] instead? It seems that is where the
> > real value would be.
> > 
> 
> Yeah. But, unfortunately, I'm not a compiler guy, so I'm not able to fix the
> compiler as you suggest. And I honestly don't see what is so annoying/disturbing
> about applying a patch that removes the 0 from foo[0] when it brings benefit
> to the whole codebase.

As far as I can tell, it doesn't actually make a difference as
all the compilers produce the same object code with either form.

There may be some compiler warning by clang through.

Does any version of gcc produce a warning on 

	struct foo {
		...
		type bar[0];
	};

but not

	struct foo {
		...
		type bar[];
	};

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Gustavo A. R. Silva]

On 3/10/20 5:15 PM, Joe Perches wrote:
> On Tue, 2020-03-10 at 17:13 -0500, Gustavo A. R. Silva wrote:
>>
>> On 3/10/20 5:07 PM, Jes Sorensen wrote:
>>> On 3/10/20 5:52 PM, Gustavo A. R. Silva wrote:
>>>>
>>>> On 3/10/20 8:56 AM, Kalle Valo wrote:
>>>>> + jes
>>>>>
>>>>> "Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:
>>>>>>> I wrote in a confusing way, my question above was about the actual patch
>>>>>>> and not the the title. For example, Jes didn't like this style change:
>>>>>>>
>>>>>>> https://patchwork.kernel.org/patch/11402315/
>>>>>>>
>>>>>>
>>>>>> It doesn't seem that that comment adds a lot to the conversation. The only
>>>>>> thing that it says is literally "fix the compiler". By the way, more than
>>>>>> a hundred patches have already been applied to linux-next[1] and he seems
>>>>>> to be the only person that has commented such a thing.
>>>>>
>>>>> But I also asked who prefers this format in that thread, you should not
>>>>> ignore questions from two maintainers (me and Jes).
>>>>>
>>>>
>>>> I'm sorry. I thought the changelog text had already the proper information.
>>>> In the changelog text I'm quoting the GCC documentation below:
>>>>
>>>> "The preferred mechanism to declare variable-length types like struct line
>>>> above is the ISO C99 flexible array member..." [1]
>>>>
>>>> I'm also including a link to the following KSPP open issue:
>>>>
>>>> https://github.com/KSPP/linux/issues/21
>>>>
>>>> The issue above mentions the following:
>>>>
>>>> "Both cases (0-byte and 1-byte arrays) pose confusion for things like sizeof(),
>>>> CONFIG_FORTIFY_SOURCE."
>>>>
>>>> sizeof(flexible-array-member) triggers a warning because flexible array members have
>>>> incomplete type[1]. There are some instances of code in which the sizeof operator
>>>> is being incorrectly/erroneously applied to zero-length arrays and the result is zero.
>>>> Such instances may be hiding some bugs. So, the idea is also to get completely rid
>>>> of those sorts of issues.
>>>
>>> As I stated in my previous answer, this seems more code churn than an
>>> actual fix. If this is a real problem, shouldn't the work be put into
>>> fixing the compiler to handle foo[0] instead? It seems that is where the
>>> real value would be.
>>>
>>
>> Yeah. But, unfortunately, I'm not a compiler guy, so I'm not able to fix the
>> compiler as you suggest. And I honestly don't see what is so annoying/disturbing
>> about applying a patch that removes the 0 from foo[0] when it brings benefit
>> to the whole codebase.
> 
> As far as I can tell, it doesn't actually make a difference as
> all the compilers produce the same object code with either form.
> 

That's precisely why we can implement these changes, cleanly(the fact
that the compiler produces the same object code). So, the resulting
object code is not the point here.

--
Gustavo

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Joe Perches]

On Tue, 2020-03-10 at 17:21 -0500, Gustavo A. R. Silva wrote:
> On 3/10/20 5:15 PM, Joe Perches wrote:
> > As far as I can tell, it doesn't actually make a difference as
> > all the compilers produce the same object code with either form.
> > 
> 
> That's precisely why we can implement these changes, cleanly(the fact
> that the compiler produces the same object code). So, the resulting
> object code is not the point here.

You are making Jes' point.

There's nothing wrong with making changes just for consistent
style across the kernel.

This change is exactly that.

I have no objection to this patch.

Jes does, though Jes is not a maintainer of this file.

I think "churn" arguments are overstated.

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Jes Sorensen]

On 3/10/20 6:28 PM, Joe Perches wrote:
> On Tue, 2020-03-10 at 17:21 -0500, Gustavo A. R. Silva wrote:
>> On 3/10/20 5:15 PM, Joe Perches wrote:
>>> As far as I can tell, it doesn't actually make a difference as
>>> all the compilers produce the same object code with either form.
>>>
>>
>> That's precisely why we can implement these changes, cleanly(the fact
>> that the compiler produces the same object code). So, the resulting
>> object code is not the point here.
> 
> You are making Jes' point.
> 
> There's nothing wrong with making changes just for consistent
> style across the kernel.
> 
> This change is exactly that.
> 
> I have no objection to this patch.
> 
> Jes does, though Jes is not a maintainer of this file.

I responded to this thread because my previous comments to files I
maintain were ignored.

This is a bulk change across the tree, so it affects a lot of places.

> I think "churn" arguments are overstated.

Again, the changes are not harmful to the code, but add no value. So far
I haven't seen any good arguments for making these changes, and having
this kind of churn is a nuisance for anyone hitting patch conflicts due
to them.

Jes

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Joe Perches]

On Tue, 2020-03-10 at 18:33 -0400, Jes Sorensen wrote:
> Again, the changes are not harmful to the code, but add no value.

That argument asserts that style consistency is value-free.
I generally disagree with that argument.

But then again, it's likely we will have to agree to disagree.

cheers, Joe

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Gustavo A. R. Silva]

On 3/10/20 5:41 PM, Joe Perches wrote:
> On Tue, 2020-03-10 at 18:33 -0400, Jes Sorensen wrote:
>> Again, the changes are not harmful to the code, but add no value.
> 
> That argument asserts that style consistency is value-free.
> I generally disagree with that argument.
> 

If this is a matter of style, then I have already proved (this is in
the changelog text) that the [] style is better than the [0] style:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76497732932f15e7323dc805e8ea8dc11bb587cf

> But then again, it's likely we will have to agree to disagree.
> 

That's a healthy thing. :)

Thanks
--
Gustavo

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Gustavo A. R. Silva]

On 3/10/20 5:28 PM, Joe Perches wrote:
> On Tue, 2020-03-10 at 17:21 -0500, Gustavo A. R. Silva wrote:
>> On 3/10/20 5:15 PM, Joe Perches wrote:
>>> As far as I can tell, it doesn't actually make a difference as
>>> all the compilers produce the same object code with either form.
>>>
>>
>> That's precisely why we can implement these changes, cleanly(the fact
>> that the compiler produces the same object code). So, the resulting
>> object code is not the point here.
> 
> You are making Jes' point.
> 

Please, see my other reply.

> There's nothing wrong with making changes just for consistent
> style across the kernel.
> 
> This change is exactly that.
> 
> I have no objection to this patch.
> 
> Jes does, though Jes is not a maintainer of this file.
> 
> I think "churn" arguments are overstated.
> 

I agree.

Thanks
--
Gustavo

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Jes Sorensen]

On 3/10/20 6:13 PM, Gustavo A. R. Silva wrote:
> 
> 
> On 3/10/20 5:07 PM, Jes Sorensen wrote:
>> As I stated in my previous answer, this seems more code churn than an
>> actual fix. If this is a real problem, shouldn't the work be put into
>> fixing the compiler to handle foo[0] instead? It seems that is where the
>> real value would be.
> 
> Yeah. But, unfortunately, I'm not a compiler guy, so I'm not able to fix the
> compiler as you suggest. And I honestly don't see what is so annoying/disturbing
> about applying a patch that removes the 0 from foo[0] when it brings benefit
> to the whole codebase.

My point is that it adds what seems like unnecessary churn, which is not
a benefit, and it doesn't improve the generated code.

Best regards,
Jes

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Gustavo A. R. Silva]

On 3/10/20 5:20 PM, Jes Sorensen wrote:
> On 3/10/20 6:13 PM, Gustavo A. R. Silva wrote:
>>
>>
>> On 3/10/20 5:07 PM, Jes Sorensen wrote:
>>> As I stated in my previous answer, this seems more code churn than an
>>> actual fix. If this is a real problem, shouldn't the work be put into
>>> fixing the compiler to handle foo[0] instead? It seems that is where the
>>> real value would be.
>>
>> Yeah. But, unfortunately, I'm not a compiler guy, so I'm not able to fix the
>> compiler as you suggest. And I honestly don't see what is so annoying/disturbing
>> about applying a patch that removes the 0 from foo[0] when it brings benefit
>> to the whole codebase.
> 
> My point is that it adds what seems like unnecessary churn, which is not
> a benefit, and it doesn't improve the generated code.
> 

As an example of one of the benefits of this is that the compiler won't trigger
a warning in the following case:

struct boo {
	int stuff;
	struct foo array[0];
	int morestuff;
};

The result of the code above is an undefined behavior.

On the other hand in the case below, the compiles does trigger a warning:

struct boo {
	int stuff;
	struct foo array[];
	int morestuff;
};

See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76497732932f15e7323dc805e8ea8dc11bb587cf

Thanks
--
Gustavo

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Jes Sorensen]

On 3/10/20 6:31 PM, Gustavo A. R. Silva wrote:
> 
> 
> On 3/10/20 5:20 PM, Jes Sorensen wrote:
>> On 3/10/20 6:13 PM, Gustavo A. R. Silva wrote:
>>>
>>>
>>> On 3/10/20 5:07 PM, Jes Sorensen wrote:
>>>> As I stated in my previous answer, this seems more code churn than an
>>>> actual fix. If this is a real problem, shouldn't the work be put into
>>>> fixing the compiler to handle foo[0] instead? It seems that is where the
>>>> real value would be.
>>>
>>> Yeah. But, unfortunately, I'm not a compiler guy, so I'm not able to fix the
>>> compiler as you suggest. And I honestly don't see what is so annoying/disturbing
>>> about applying a patch that removes the 0 from foo[0] when it brings benefit
>>> to the whole codebase.
>>
>> My point is that it adds what seems like unnecessary churn, which is not
>> a benefit, and it doesn't improve the generated code.
>>
> 
> As an example of one of the benefits of this is that the compiler won't trigger
> a warning in the following case:
> 
> struct boo {
> 	int stuff;
> 	struct foo array[0];
> 	int morestuff;
> };
> 
> The result of the code above is an undefined behavior.
> 
> On the other hand in the case below, the compiles does trigger a warning:
> 
> struct boo {
> 	int stuff;
> 	struct foo array[];
> 	int morestuff;
> };

Right, this just underlines my prior argument, that this should be fixed
in the compiler.

Jes

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Gustavo A. R. Silva]

On 3/10/20 5:34 PM, Jes Sorensen wrote:
> On 3/10/20 6:31 PM, Gustavo A. R. Silva wrote:
>>
>>
>> On 3/10/20 5:20 PM, Jes Sorensen wrote:
>>> On 3/10/20 6:13 PM, Gustavo A. R. Silva wrote:
>>>>
>>>>
>>>> On 3/10/20 5:07 PM, Jes Sorensen wrote:
>>>>> As I stated in my previous answer, this seems more code churn than an
>>>>> actual fix. If this is a real problem, shouldn't the work be put into
>>>>> fixing the compiler to handle foo[0] instead? It seems that is where the
>>>>> real value would be.
>>>>
>>>> Yeah. But, unfortunately, I'm not a compiler guy, so I'm not able to fix the
>>>> compiler as you suggest. And I honestly don't see what is so annoying/disturbing
>>>> about applying a patch that removes the 0 from foo[0] when it brings benefit
>>>> to the whole codebase.
>>>
>>> My point is that it adds what seems like unnecessary churn, which is not
>>> a benefit, and it doesn't improve the generated code.
>>>
>>
>> As an example of one of the benefits of this is that the compiler won't trigger
>> a warning in the following case:
>>
>> struct boo {
>> 	int stuff;
>> 	struct foo array[0];
>> 	int morestuff;
>> };
>>
>> The result of the code above is an undefined behavior.
>>
>> On the other hand in the case below, the compiles does trigger a warning:
>>
>> struct boo {
>> 	int stuff;
>> 	struct foo array[];
>> 	int morestuff;
>> };
> 
> Right, this just underlines my prior argument, that this should be fixed
> in the compiler.
> 

In the meantime it's not at all harmful to do something about it in the codebase.

Thanks
--
Gustavo

Re: [PATCH][next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member

[Kalle Valo]

"Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:

> On 3/10/20 5:34 PM, Jes Sorensen wrote:
>> On 3/10/20 6:31 PM, Gustavo A. R. Silva wrote:
>>>
>>>
>>> On 3/10/20 5:20 PM, Jes Sorensen wrote:
>>>> On 3/10/20 6:13 PM, Gustavo A. R. Silva wrote:
>>>>>
>>>>>
>>>>> On 3/10/20 5:07 PM, Jes Sorensen wrote:
>>>>>> As I stated in my previous answer, this seems more code churn than an
>>>>>> actual fix. If this is a real problem, shouldn't the work be put into
>>>>>> fixing the compiler to handle foo[0] instead? It seems that is where the
>>>>>> real value would be.
>>>>>
>>>>> Yeah. But, unfortunately, I'm not a compiler guy, so I'm not able to fix the
>>>>> compiler as you suggest. And I honestly don't see what is so annoying/disturbing
>>>>> about applying a patch that removes the 0 from foo[0] when it brings benefit
>>>>> to the whole codebase.
>>>>
>>>> My point is that it adds what seems like unnecessary churn, which is not
>>>> a benefit, and it doesn't improve the generated code.
>>>>
>>>
>>> As an example of one of the benefits of this is that the compiler won't trigger
>>> a warning in the following case:
>>>
>>> struct boo {
>>> 	int stuff;
>>> 	struct foo array[0];
>>> 	int morestuff;
>>> };
>>>
>>> The result of the code above is an undefined behavior.
>>>
>>> On the other hand in the case below, the compiles does trigger a warning:
>>>
>>> struct boo {
>>> 	int stuff;
>>> 	struct foo array[];
>>> 	int morestuff;
>>> };
>> 
>> Right, this just underlines my prior argument, that this should be fixed
>> in the compiler.
>> 
>
> In the meantime it's not at all harmful to do something about it in the codebase.

Cleanup patches are not always harmful, at least they can create bugs
and conflicts. But I think in this case there are clear benefits for the
churn so I'm going to apply these.

Sorry Jes :)

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH][next] zd1211rw: Replace zero-length array with flexible-array member

[Kalle Valo]

"Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:

> The current codebase makes use of the zero-length array language
> extension to the C90 standard, but the preferred mechanism to declare
> variable-length types such as these ones is a flexible array member[1][2],
> introduced in C99:
> 
> struct foo {
>         int stuff;
>         struct boo array[];
> };
> 
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on.
> 
> Also, notice that, dynamic memory allocations won't be affected by
> this change:
> 
> "Flexible array members have incomplete type, and so the sizeof operator
> may not be applied. As a quirk of the original implementation of
> zero-length arrays, sizeof evaluates to zero."[1]
> 
> This issue was found with the help of Coccinelle.
> 
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
> 
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>

Patch applied to wireless-drivers-next.git, thanks.

8622a0e5a499 zd1211rw: Replace zero-length array with flexible-array member

-- 
https://patchwork.kernel.org/patch/11421571/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches