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

Commit Message

Linus Torvalds March 30, 2014, 6:38 p.m. UTC

On Sun, Mar 30, 2014 at 10:22 AM, Linus Torvalds
<[e-mail]> 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
symbol.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Comments

Christopher Li March 30, 2014, 7:22 p.m. UTC | #1

On Sun, Mar 30, 2014 at 11:38 AM, Linus Torvalds
<[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Linus Torvalds March 30, 2014, 8:21 p.m. UTC | #2

On Sun, Mar 30, 2014 at 12:22 PM, Christopher Li <[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christopher Li March 31, 2014, 5:59 a.m. UTC | #3

On Sun, Mar 30, 2014 at 1:21 PM, Linus Torvalds
<[e-mail]> wrote:
> On Sun, Mar 30, 2014 at 12:22 PM, Christopher Li <[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christopher Li April 18, 2014, 6:29 a.m. UTC | #4

On Sun, Mar 30, 2014 at 1:21 PM, Linus Torvalds
<[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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)