On Fri, 2022-09-09 at 13:45 -0700, Brian Norris wrote: > > I think gcc also has one with W=1 now? > > > > But yes, it's similar to that one. I was looking at Jakub's netdev test > > builds here: > > > > https://patchwork.hopto.org/static/nipa/673828/12964988/build_32bit/stderr > > > > ../drivers/net/wireless/marvell/mwifiex/fw.h:2107:46: warning: array of flexible structures > > ../drivers/net/wireless/marvell/mwifiex/fw.h:2257:47: warning: array of flexible structures > > I think you're actually looking at a sparse warning: > > https://lore.kernel.org/linux-sparse/20200930231828.66751-12-luc.vanoostenryck@xxxxxxxxx/ > > I can convince clang to enable the warning I mentioned, but it's not on > by default, even with W=1. When enabled, it complains similarly, as well > as about a ton of other code too. Hm. I _think_ I saw it locally with just W=1, not C=1, but then that would've been with gcc, probably 12.2 from Fedora. But I didn't try again now, don't have much time. > I suppose this warning makes sense when you're truly treating these as > arrays (of size more than 1). And in this case (mwifiex_cmd_mef_cfg() > and mwifiex_cmd_append_rpn_expression()), the code to (mostly?) properly > handle the flexible array is pretty ugly anyway, and doesn't really use > the type safety much (lots of casting through u8* and similar). So this > isn't really the best example to try to retain. Heh, fair point. > (mwifiex_cmd_coalesce_cfg() has some similarly ugly casting and math.) Yeah it kind of has to though, given that it's a variable-sized buffer with variable-sized entries... > But if the "array" is just an optional extension to a struct, and we > expect at most a single element, then I don't think the construct is > fundamentally wrong. It might still be hard to get correct in all cases > (e.g., ensuring the right buffer size), but it still feels like a decent > way to describe things. Fair enough. It's like 0 or 1, maybe, but of course there's no way to describe that to the compiler and say "I promise to never access [1] (or higher)" :) I guess the only real alternative would be to leave it as a _single_ entry, and then fix up all the sizeof() of the containing struct, if any, to be offsetof(..., first_entry). But that sort of describes a single entry should be present, which it might not be. > After a little more look, I'm fine with this patch. I'd probably > appreciate it better if there was a comment of some kind in the struct > definition, and perhaps mention sparse's -Wflexible-array-array. > Sure, I guess it makes sense to also figure out more precisely where the warning appeared. I'm running off for a trip for two weeks now-ish, so not going to send it in the short term. If you wanted to do it instead, no objections, or if Kalle wants to just add a comment while applying, or whatever. In any case there are so many warnings in the drivers that still ought to _have_ a maintainer (and I'm squinting at you for ath too, Kalle) that it's pretty much irrelevant to fix this one quickly ;-) johannes
