[PATCH 2/2] Use any previous initializer to size a symbol

[PATCH 2/2] Use any previous initializer to size a symbol

[Linus Torvalds]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sun, 30 Mar 2014 10:13:54 -0700
Subject: [PATCH 2/2] Use any previous initializer to size a symbol

When we size a symbol, we only have one initializer per symbol, but we
may have multiple symbol declarations for the same symbol.  So make sure
to walk the "same_symbol" chain to find the initializer, rather than
assuming it is attached to the current declaration.

Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

This seems to be the simplest approach. Chris, what was your concern about 
scoping? The "same_symbol" list should already take scoping into account.

 symbol.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/symbol.c b/symbol.c
index eb6e1215ee87..4b91abd8021e 100644
--- a/symbol.c
+++ b/symbol.c
@@ -354,6 +354,15 @@ static int count_array_initializer(struct symbol *t, struct expression *expr)
 	return nr;
 }
 
+static struct expression *get_symbol_initializer(struct symbol *sym)
+{
+	do {
+		if (sym->initializer)
+			return sym->initializer;
+	} while ((sym = sym->same_symbol) != NULL);
+	return NULL;
+}
+
 static struct symbol * examine_node_type(struct symbol *sym)
 {
 	struct symbol *base_type = examine_base_type(sym);
@@ -376,12 +385,15 @@ static struct symbol * examine_node_type(struct symbol *sym)
 		sym->ctype.alignment = alignment;
 
 	/* Unsized array? The size might come from the initializer.. */
-	if (bit_size < 0 && base_type->type == SYM_ARRAY && sym->initializer) {
-		struct symbol *node_type = base_type->ctype.base_type;
-		int count = count_array_initializer(node_type, sym->initializer);

Re: [PATCH 2/2] Use any previous initializer to size a symbol

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1514 bytes --]

On Sun, Mar 30, 2014 at 10:22 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> This seems to be the simplest approach. Chris, what was your concern about
> scoping? The "same_symbol" list should already take scoping into account.

Ho humm. I found a test-case that still messes up:

  extern const char *v4l2_type_names[];
  const char *v4l2_type_names[] = {
    "test", "hello"
  };

  static unsigned test(void)
  {
    extern const char *v4l2_type_names[];
    return sizeof(v4l2_type_names);
  }

and the reason seems to be that the 'same_symbol' list is incomplete.

What happen sis that we tie the two "extern" v4l2_type_names[] things
together, and the declaration of v4l2_type_names[] with the
initializer is also tied to the first 'extern' declaration, but we do
*not* tie the second 'extern' in block scope to the one with the
initializer.

The reason seems to be that our logic in check_declaration[] is
broken: it only ties things together based on both symbols having
'extern'. So the actual declaration with an initializer, that lacks
the extern (but is in global scope) is missed.

The attached patch would seem to fix it. Chris, what is the reason for
that MOD_INLINE check? You added it in commit 8cf99394ee4c ("move
extern inline function to file scope") and I suspect it messes up the
same_symbol logic. But I left it alone, and added a comment. If you
can resolve that comment, please apply this patch with my sign-off and
some random commit message..

                      Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 913 bytes --]

 symbol.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/symbol.c b/symbol.c
index 4b91abd8021e..c5e4784734a8 100644
--- a/symbol.c
+++ b/symbol.c
@@ -576,11 +576,15 @@ void check_declaration(struct symbol *sym)
 			sym->same_symbol = next;
 			return;
 		}
-		if (sym->ctype.modifiers & next->ctype.modifiers & MOD_EXTERN) {
-			if ((sym->ctype.modifiers ^ next->ctype.modifiers) & MOD_INLINE)
-				continue;
-			sym->same_symbol = next;
-			return;
+		/* Extern in block level matches a TOPLEVEL non-static symbol */
+		if (sym->ctype.modifiers & MOD_EXTERN) {
+			if ((next->ctype.modifiers & (MOD_TOPLEVEL|MOD_STATIC)) == MOD_TOPLEVEL) {
+				/* FIXME? Differs in 'inline' only? Why does that matter? */
+				if ((sym->ctype.modifiers ^ next->ctype.modifiers) & MOD_INLINE)
+					continue;
+				sym->same_symbol = next;
+				return;
+			}
 		}
 
 		if (!Wshadow || warned)

Re: [PATCH 2/2] Use any previous initializer to size a symbol

[Christopher Li]

On Sun, Mar 30, 2014 at 11:38 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> The reason seems to be that our logic in check_declaration[] is
> broken: it only ties things together based on both symbols having
> 'extern'. So the actual declaration with an initializer, that lacks
> the extern (but is in global scope) is missed.
>
> The attached patch would seem to fix it. Chris, what is the reason for
> that MOD_INLINE check? You added it in commit 8cf99394ee4c ("move
> extern inline function to file scope") and I suspect it messes up the
> same_symbol logic. But I left it alone, and added a comment. If you

Possible. If I comment out that check, I will get this error in the
extern-inline.c
test case:

$ ./sparse validation/extern-inline.c validation/extern-inline.c
validation/extern-inline.c:12:1: warning: multiple definitions for function 'g'
validation/extern-inline.c:12:1:  the previous one is here
$

Unfortunately I have to run now, I will take a closer look when I am back
tonight. I verify that applying your second patch did not trigger the error
in extern-inline.c.

I think it is possible the fix for the extern-inline.c is wrong regarding the
same_symbol chain, there should be better way to fix it. I need to spend
more time on it though.

> can resolve that comment, please apply this patch with my sign-off and
> some random commit message..

Sure

Chris

Re: [PATCH 2/2] Use any previous initializer to size a symbol

[Linus Torvalds]

On Sun, Mar 30, 2014 at 12:22 PM, Christopher Li <sparse@chrisli.org> wrote:
>
> Possible. If I comment out that check, I will get this error in the
> extern-inline.c
> test case:
>
> $ ./sparse validation/extern-inline.c validation/extern-inline.c
> validation/extern-inline.c:12:1: warning: multiple definitions for function 'g'
> validation/extern-inline.c:12:1:  the previous one is here

Ok, I think that is just us being too anal. We should allow multiple
definitions for functions that are identical otherwise, but differ in
the "inline".

So I think that MOD_INLINE check in check_declaration() is wrong, and
makes us consider the definitions to be for different symbols. Yes,
that ignores the warning, but they really *aren't* different symbols.

So the correct thing to do is (I think) to remove that check, and then
make the parse_function_body() warning go away by accepting the fact
that we can have both an inline version and an out-of-line version of
the same function.

Hmm? I'm on my way out, so I'm not going to write that patch..

              Linus

Re: [PATCH 2/2] Use any previous initializer to size a symbol

[Christopher Li]

On Sun, Mar 30, 2014 at 10:22 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Sun, 30 Mar 2014 10:13:54 -0700
> Subject: [PATCH 2/2] Use any previous initializer to size a symbol
>
> When we size a symbol, we only have one initializer per symbol, but we
> may have multiple symbol declarations for the same symbol.  So make sure
> to walk the "same_symbol" chain to find the initializer, rather than
> assuming it is attached to the current declaration.
>
> Reported-by: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

This patch looks good to me. Will apply.


> ---
>
> This seems to be the simplest approach. Chris, what was your concern about
> scoping? The "same_symbol" list should already take scoping into account.


This bug is a special case of the more general sparse bug in merging
declaration types.

The scoping I am concern about is some thing like this:
(BTW, your patch does not have the scoping issue).

extern void foo(void) __attribute__((X));
for (;;) {
    extern void foo(void) __attribute__((Y));
    /* function foo()  should have both attribute X and Y here */

}

/* foo() should have only attribute X here */

Merging the initializer is a special case of this more general
bug. Obviously, this patch fix the merging of initialization,  should
be applied.

Chris

Re: [PATCH 2/2] Use any previous initializer to size a symbol

[Christopher Li]

On Sun, Mar 30, 2014 at 1:21 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sun, Mar 30, 2014 at 12:22 PM, Christopher Li <sparse@chrisli.org> wrote:
>>
>> Possible. If I comment out that check, I will get this error in the
>> extern-inline.c
>> test case:
>>
>> $ ./sparse validation/extern-inline.c validation/extern-inline.c
>> validation/extern-inline.c:12:1: warning: multiple definitions for function 'g'
>> validation/extern-inline.c:12:1:  the previous one is here
>
> Ok, I think that is just us being too anal. We should allow multiple
> definitions for functions that are identical otherwise, but differ in
> the "inline".

The difference is not only in "inline", but also the different function body
declared in two different files. The better example should be
./sparse validations/extern-inlineA.c validations/extern-inlineB.c

where inlineA.c and inlineB.c has different implementation of function 'g'.
Same function prototype of 'g', just different function body in different files.

In other words, we should treat extern inline has the same file visibility
as static inline. The only difference is that, when compiler can't inline
the function, extern inline assume the function has external copy and
don't generate the local copy. Static inline turn the function into
a static function.


>
> So the correct thing to do is (I think) to remove that check, and then
> make the parse_function_body() warning go away by accepting the fact
> that we can have both an inline version and an out-of-line version of
> the same function.

I agree we should remove that MOD_INLINE check and fix the warning else
where. The warning is actually not about inline version vs out-of-line version.
It is about two files have two different implementation of the inline version.

Let me take a stab on that. My guess is that, I can fix it by treating
extern inline
like static inline, remove the symbol when the file scope ends. Then the
second file will not see it and complain the duplicate symbol. Haven't verify
that works or not yet.

Chris

Re: [PATCH 2/2] Use any previous initializer to size a symbol

[Christopher Li]

On Sun, Mar 30, 2014 at 1:21 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Ok, I think that is just us being too anal. We should allow multiple
> definitions for functions that are identical otherwise, but differ in
> the "inline".
>
> So I think that MOD_INLINE check in check_declaration() is wrong, and
> makes us consider the definitions to be for different symbols. Yes,
> that ignores the warning, but they really *aren't* different symbols.

I take a closer look to this MOD_INLINE check. That check is wrong
and should be removed.

However, still don't have a good way to fix duplicate extern inline check yet.
Here is what goes wrong when I remove the check

./sparse validation/extern-inline.c validation/extern-inline.c
validation/extern-inline.c:12:1: warning: multiple definitions for function 'g'
validation/extern-inline.c:12:1:  the previous one is here

The offending source code are:
extern int g(int);

extern __inline__ int
g(int x)
{
        return x;
}

Notice that the first extern int g(int) does not have inline? It get the
global scope. The second g has inline, it get the local scope.

However, the first global version of g() gets the declaration from
parse_function_body()

        while (prev) {
            prev->definition = decl;
            prev = prev->same_symbol;
        }

The extern-inline version of g() actually get removed when the file scope
ends. The extern version stays and it has the definition from the
extern-inline version. (BTW, there is a minor bug there the user of
the first global symbol will know nothing about inline, they are using
the global version on the extern inline version. I can give a test case
if needed)

So when we compile the extern-inline as the second file, sparse complain
that there are two g() has definition, the conflicting one is the first global
version of g(), the function body borrow from the inline version.

> So the correct thing to do is (I think) to remove that check, and then
> make the parse_function_body() warning go away by accepting the fact
> that we can have both an inline version and an out-of-line version of
> the same function.

Unfortunately, disable the warning is not the right fix. Because it will
allow extern-inline fuction to have two function body in the same file.
e.g.

extern __inline__ int g(int x) { return x; }
extern __inline__ int g(int x) { return x + 1; }

If we just silence the warning, sparse will not able to warn about this.

So it come back to the question:
When the file scope ends, why does the global symbol
still stays? end_file_scope() only removes the local file symbol,
not the global symbol define by the previous file.

When sparse compile the second file, it can still see the global
symbol left by the previous file. If the global symbol was removed,
this duplicate definition warning should go way as well.

Suggestion?

I am tempting to just revert the MOD_INLINE check and fix the
extern inline breakage in a different patch.

Chris