| Message ID | 20210122162625.73007-1-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | handle qualified anonymous structures | expand |
On Fri, Jan 22, 2021 at 8:26 AM Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > > So, the solution chosen here is to handle this during type examination, > more precisely, inside examine_struct_union_type(), where things are > a bit more complicated Well, doesn't look all that complicated to me. The only thing I would do is to just at the head of that function do: unsigned long mod = sym->ctype.modifiers & MOD_QUALIFIER; if (!mod) return; and that also means that you can avoid the "parent-vs-sym" thing, because the symbol is never used after that, so you don't need to create a new one. The other thing that might be worth doing is to just make sure that the "sub" whose modifier you change is always a SYM_NODE. We never want to touch an actual type, only the node. I don't think it _can_ be anything else (that's how the struct/union symbol_list should be set up), but since this is a very unusual case of going back and modifying a symbol after the fact, I think I'd be a bit more comfortable with that kind of sanity check. Hmm? Anyway, looks good, and obviously passes my trivial test-case. Linus
On Fri, Jan 22, 2021 at 9:35 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Anyway, looks good, and obviously passes my trivial test-case. Oh, and then I tried a slightly more complex test-case, and didn't get the result I expected. In this: struct dummy { const struct { int a; volatile struct { int b; }; }; int c; }; int *test(struct dummy *a) { a->a = a->c; return &a->b; } I expected to also get a warning about how we return the wrong type (ie "&a->b" is of type "const volatile *int", but "test()" returns an "int *"). That seems to have nothing to do with the anonymous struct type, though. It is just because we never warn about that pointer conversion at all. Interestingly, It does show up as a "ptrcast" instruction in the linearization, so I can tell that yes, sparse saw that it was a different pointer type. It just didn't care to warn. Not a huge deal, but I thought I'd mention it since it showed up in my test. Linus
On Fri, Jan 22, 2021 at 09:35:43AM -0800, Linus Torvalds wrote: > On Fri, Jan 22, 2021 at 8:26 AM Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > > > > So, the solution chosen here is to handle this during type examination, > > more precisely, inside examine_struct_union_type(), where things are > > a bit more complicated > > Well, doesn't look all that complicated to me. > > The only thing I would do is to just at the head of that function do: > > unsigned long mod = sym->ctype.modifiers & MOD_QUALIFIER; > > if (!mod) > return; > > and that also means that you can avoid the "parent-vs-sym" thing, > because the symbol is never used after that, so you don't need to > create a new one. Yes, certainly since the vast majority will have a null mod. > The other thing that might be worth doing is to just make sure that > the "sub" whose modifier you change is always a SYM_NODE. We never > want to touch an actual type, only the node. > > I don't think it _can_ be anything else (that's how the struct/union > symbol_list should be set up), but since this is a very unusual case > of going back and modifying a symbol after the fact, I think I'd be a > bit more comfortable with that kind of sanity check. Yes, I agree. I fact I almost did this but hen I said "why bother? it's always a NODE anyway". But sure, it's cheap and better be safe than sorry. > Anyway, looks good, and obviously passes my trivial test-case. Thank you. -- Luc
On Fri, Jan 22, 2021 at 10:01:40AM -0800, Linus Torvalds wrote: > On Fri, Jan 22, 2021 at 9:35 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > Anyway, looks good, and obviously passes my trivial test-case. > > Oh, and then I tried a slightly more complex test-case, and didn't get > the result I expected. > > In this: > > struct dummy { > const struct { > int a; > volatile struct { > int b; > }; > }; > int c; > }; > > int *test(struct dummy *a) > { > a->a = a->c; > return &a->b; > } > > I expected to also get a warning about how we return the wrong type > (ie "&a->b" is of type "const volatile *int", but "test()" returns an > "int *"). > > That seems to have nothing to do with the anonymous struct type, > though. It is just because we never warn about that pointer conversion > at all. > > Interestingly, It does show up as a "ptrcast" instruction in the > linearization, so I can tell that yes, sparse saw that it was a > different pointer type. It just didn't care to warn. > > Not a huge deal, but I thought I'd mention it since it showed up in my test. Thank you to mention this. It's because once an error has been issued, warnings are not displayed anymore. For example, the following will return the expected warning: int *test(struct dummy *a) { return &a->b; } I think it's something that made a lot of sense before but which is more and more annoying because it hides legitimate warnings, like here (of course, it also hides silly/'second order' warnings). -- Luc
diff --git a/symbol.c b/symbol.c index aa02c8c5ad80..0030afb5f3f5 100644 --- a/symbol.c +++ b/symbol.c @@ -175,6 +175,30 @@ static void lay_out_struct(struct symbol *sym, struct struct_union_info *info) // warning (sym->pos, "regular: offset=%d", sym->offset); } +/// +// propagate properties of anonymous structs or unions into their members. +// +// :note: GCC seems to only propagate the qualifiers. +// :note: clang doesn't propagate anything at all. +static void examine_anonymous_member(struct symbol *sym) +{ + struct symbol *parent = sym; + struct symbol *sub; + + if (parent->type == SYM_NODE) + parent = sym->ctype.base_type; + if (parent->type != SYM_STRUCT && parent->type != SYM_UNION) + return; + + FOR_EACH_PTR(parent->symbol_list, sub) { + sub->ctype.modifiers |= sym->ctype.modifiers & MOD_QUALIFIER; + + // if nested, propagate all the way down + if (!sub->ident) + examine_anonymous_member(sub); + } END_FOR_EACH_PTR(sub); +} + static struct symbol * examine_struct_union_type(struct symbol *sym, int advance) { struct struct_union_info info = { @@ -196,6 +220,8 @@ static struct symbol * examine_struct_union_type(struct symbol *sym, int advance if (info.flex_array) sparse_error(info.flex_array->pos, "flexible array member '%s' is not last", show_ident(info.flex_array->ident)); examine_symbol_type(member); + if (!member->ident) + examine_anonymous_member(member); if (member->ctype.alignment > info.max_align && !sym->packed) { // Unnamed bitfields do not affect alignment.
The kernel is trying to use a unnamed struct to mark a group of fields as being 'const' and get a warning if one of its member is assigned. GCC gives indeed a warning but Sparse and clang don't. So, the type qualifiers of the anonymous struct need to be propagate to the members. An easy solution is to handle this inside find_identifier(), where access to are recursively searched inside sub-structures. It's very easy but it feels wrong to do semantics inside this function. Worse, it's only working for fields that are effectively accessed, doing a type evaluation on the anonymous struct (or its parent) would not by itself handle this. So, the solution chosen here is to handle this during type examination, more precisely, inside examine_struct_union_type(), where things are a bit more complicated (it can't be done when examining the members themselves because only the parent SYM_STRUCT is accessible and the qualifiers are in the SYM_NODE, so it needs to be done when examining the anonymous struct itself) but can be done for all members. Note: It would seems to logical to also handle at least all qualifier-like attributes but GCC seems to only bother with the true qualifiers and ignore things like attributes and alignment specifiers. Link: lore.kernel.org/r/CAHk-=wj4Kviw8q2Sx9vrrvyn3uWK-zNi4uGRx=5bzde0Cio8uQ@mail.gmail.com Link: lore.kernel.org/r/CAHk-=wjdJmL22+zk3_rWAfEJJCf=oDxiJ530qk-WNk_Ji0qhxw@mail.gmail.com Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> --- symbol.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)