'const' unnamed structures

'const' unnamed structures

[Linus Torvalds]

So the kernel was trying to use a unnamed struct to mark a group of
fields as being 'const', and while gcc is perfectly happy with that,
it turns out clang is not.

And while testing, I notice that sparse is also not happy with it.

Stupid test-case:

    struct dummy {
        const struct {
                int a, b;
        };
        int c;
    };

    void test(struct dummy *a)
    {
        a->a = a->c;
    }

and gcc will report

    t.c: In function ‘test’:
    t.c:10:7: error: assignment of member ‘a’ in read-only object
       10 |  a->a = a->c;
          |       ^

which is the helpful message we're looking for in the kernel, but
clang and sparse both silently accept this. Presumably for very
similar reasons.

(And when I talk about 'const', obviously all the same issues hold wrt
'volatile' too)

Basically, I see two possibilities

 (a) since the const is there in the unnamed sub-struct, we can "fix"
it at evaluate lo time, in find_identifier().

     We'd have to add a "unsigned int qual" as an argument - initially
zero - to "find_identifier()", and then in the recursive unnamed
union/struct case we'd or in the qualifiers for this one.

     And then we'd modify the symbol result as per the qualifier when
we return it.

Honestly, (a) strikes me as ugly and wrong, but it might be simpler
than what I think might be the right model:

 (b) examine_struct_union_type() knows when it is traversing an
unnamed union,  and when it does that

                examine_symbol_type(member);

     it would then add the modifiers after-the-fact.

Obviously, the third possibility is to say "ok, clang also gets this
wrong, the clang people are trying to argue that the standard is not
clear about it, and sparse might as well ignore this until it's a
bigger problem".

           Linus

Re: 'const' unnamed structures

[Luc Van Oostenryck]

On Wed, Jan 20, 2021 at 11:21:46AM -0800, Linus Torvalds wrote:
> So the kernel was trying to use a unnamed struct to mark a group of
> fields as being 'const', and while gcc is perfectly happy with that,
> it turns out clang is not.

...
 
> Basically, I see two possibilities
> 
>  (a) since the const is there in the unnamed sub-struct, we can "fix"
> it at evaluate lo time, in find_identifier().
> 
>      We'd have to add a "unsigned int qual" as an argument - initially
> zero - to "find_identifier()", and then in the recursive unnamed
> union/struct case we'd or in the qualifiers for this one.
> 
>      And then we'd modify the symbol result as per the qualifier when
> we return it.
> 
> Honestly, (a) strikes me as ugly and wrong, but it might be simpler
> than what I think might be the right model:

Yes, and it would anyway need more than a simple "unsigned int qual"
because attributes (and alignment via _Alignas()) are also concerned.
But I don't think an additional argument is needed: merging the
modifiers, alignment and AS can be done inside find_identifier()
itself, in the recursive part. This would make it a little less ugly.

> Obviously, the third possibility is to say "ok, clang also gets this
> wrong, the clang people are trying to argue that the standard is not
> clear about it, and sparse might as well ignore this until it's a
> bigger problem".

Hehe, this third possibility is kinda tempting but for once the
standard seems quite clear to me (the grammar clearly allow it,
why it should then be ignored at the semantic level?).

I'll look at this in the next days.

-- Luc

[PATCH] handle qualified anonymous structures

[Luc Van Oostenryck]

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(+)

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.
-- 
2.30.0

Re: [PATCH] handle qualified anonymous structures

[Linus Torvalds]

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

Re: [PATCH] handle qualified anonymous structures

[Linus Torvalds]

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

Re: [PATCH] handle qualified anonymous structures

[Luc Van Oostenryck]

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

Re: [PATCH] handle qualified anonymous structures

[Luc Van Oostenryck]

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