| Message ID | 20200305111216.GA24982@embeddedor (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8622a0e5a499e99aeee4df66eacfb25830c57388 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [next] zd1211rw/zd_usb.h: Replace zero-length array with flexible-array member | expand |
"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.
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.
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/
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
+ 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?
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? >
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
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
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[]; };
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
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
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.
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
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
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
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
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
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
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
"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 :)
"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
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 {
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(-)