Extern variables in *.c files

Extern variables in *.c files

[vda]

I grepped kernel *.c (not *.h!) files for extern variable definitions.
Much to my surprize, I found ~1500 such defs.

Isn't that bad C code style? What will happen if/when type of variable gets 
changed? (int->long).

I thought external definitions ought to be in *.h files.
Maybe I'm missing something here...
--
vda

Re: Extern variables in *.c files

[Oliver Xymoron]

On Wed, 2 Jan 2002, vda wrote:

> I grepped kernel *.c (not *.h!) files for extern variable definitions.
> Much to my surprize, I found ~1500 such defs.
>
> Isn't that bad C code style? What will happen if/when type of variable gets
> changed? (int->long).

Yes; Int->long won't change anything on 32-bit machines and will break
silently on 64-bit ones. The trick is finding appropriate places to put
such definitions so that all the things that need them can include them
without circular dependencies.

-- 
 "Love the dolphins," she advised him. "Write by W.A.S.T.E.."

Re: Extern variables in *.c files

[Andrew Morton]

Oliver Xymoron wrote:
> 
> On Wed, 2 Jan 2002, vda wrote:
> 
> > I grepped kernel *.c (not *.h!) files for extern variable definitions.
> > Much to my surprize, I found ~1500 such defs.
> >
> > Isn't that bad C code style? What will happen if/when type of variable gets
> > changed? (int->long).
> 
> Yes; Int->long won't change anything on 32-bit machines and will break
> silently on 64-bit ones. The trick is finding appropriate places to put
> such definitions so that all the things that need them can include them
> without circular dependencies.
> 

Isn't there some way to get the linker to detect the differing
sizes?

-

Re: Extern variables in *.c files

[Momchil Velikov]

>>>>> "Andrew" == Andrew Morton <akpm@zip.com.au> writes:

Andrew> Oliver Xymoron wrote:
>> 
>> On Wed, 2 Jan 2002, vda wrote:
>> 
>> > I grepped kernel *.c (not *.h!) files for extern variable definitions.
>> > Much to my surprize, I found ~1500 such defs.
>> >
>> > Isn't that bad C code style? What will happen if/when type of variable gets
>> > changed? (int->long).
>> 
>> Yes; Int->long won't change anything on 32-bit machines and will break
>> silently on 64-bit ones. The trick is finding appropriate places to put
>> such definitions so that all the things that need them can include them
>> without circular dependencies.
>> 

Andrew> Isn't there some way to get the linker to detect the differing
Andrew> sizes?
`--warn-common'
     Warn when a common symbol is combined with another common symbol
     or with a symbol definition.  Unix linkers allow this somewhat
     sloppy practice, but linkers on some other operating systems do
     not.  This option allows you to find potential problems from
     combining global symbols.  Unfortunately, some C libraries use
     this practice, so you may get some warnings about symbols in the
     libraries as well as in your programs.

Regards,
-velco

Re: Extern variables in *.c files

[Andrew Morton]

Momchil Velikov wrote:
> 
> >>>>> "Andrew" == Andrew Morton <akpm@zip.com.au> writes:
> 
> Andrew> Oliver Xymoron wrote:
> >>
> >> On Wed, 2 Jan 2002, vda wrote:
> >>
> >> > I grepped kernel *.c (not *.h!) files for extern variable definitions.
> >> > Much to my surprize, I found ~1500 such defs.
> >> >
> >> > Isn't that bad C code style? What will happen if/when type of variable gets
> >> > changed? (int->long).
> >>
> >> Yes; Int->long won't change anything on 32-bit machines and will break
> >> silently on 64-bit ones. The trick is finding appropriate places to put
> >> such definitions so that all the things that need them can include them
> >> without circular dependencies.
> >>
> 
> Andrew> Isn't there some way to get the linker to detect the differing
> Andrew> sizes?
> `--warn-common'
>      Warn when a common symbol is combined with another common symbol
>      or with a symbol definition.  Unix linkers allow this somewhat
>      sloppy practice, but linkers on some other operating systems do
>      not.  This option allows you to find potential problems from
>      combining global symbols.  Unfortunately, some C libraries use
>      this practice, so you may get some warnings about symbols in the
>      libraries as well as in your programs.
> 

Alas, it doesn't quite work as we'd like:

akpm-1:/home/akpm> cat a.c
int a;

main()
{}
akpm-1:/home/akpm> cat b.c
extern char a;

int b()
{return a;}
akpm-1:/home/akpm> gcc -fno-common -Wl,--warn-common a.c b.c
akpm-1:/home/akpm> 

No warnings emitted.  If b.c doesn't use `extern' it will fail
to link because of -fno-common.

What we'd *like* to happen is for the linker to determine that
the `extern char' and the `int' declarations are a mismatch.
Maybe the object file doesn't have the size info for `extern'
variables.   The compiler emits it though.

We can actually get what we want by disabling `-fno-common' and enabling
`--warn-common', but that's rather awkward. 

Perhaps HJ can suggest a solution?

-

Re: Extern variables in *.c files

[H . J . Lu]

On Wed, Jan 02, 2002 at 11:19:29PM -0800, Andrew Morton wrote:
> 
> Alas, it doesn't quite work as we'd like:
> 
> akpm-1:/home/akpm> cat a.c
> int a;
> 
> main()
> {}
> akpm-1:/home/akpm> cat b.c
> extern char a;
> 
> int b()
> {return a;}
> akpm-1:/home/akpm> gcc -fno-common -Wl,--warn-common a.c b.c
> akpm-1:/home/akpm> 
> 
> No warnings emitted.  If b.c doesn't use `extern' it will fail
> to link because of -fno-common.
> 
> What we'd *like* to happen is for the linker to determine that
> the `extern char' and the `int' declarations are a mismatch.
> Maybe the object file doesn't have the size info for `extern'
> variables.   The compiler emits it though.
> 

Compile doesn't emit the size info for

extern char a;

One way to fix it is to remove

extern char a;

and put

extern int a;

in a header file which is included by everyone.


H.J.

Re: Extern variables in *.c files

[Andrew Morton]

"H . J . Lu" wrote:
> 
> ...
> Compile doesn't emit the size info for
> 
> extern char a;

You're right.  I goofed.
 
> One way to fix it is to remove
> 
> extern char a;
> 
> and put
> 
> extern int a;
> 
> in a header file which is included by everyone.
> 

Yup.  Problem is, we have about 1500 instances in the kernel :(

(Wouldn't it be nice if `int a;' generated a compiler error
if a declaration `extern int a;' was not in scope?)

Oh well.  Seems that disabling -fno-common and enabling
--warn-common is the only way to autodetect bugs such as this.

-

Re: Extern variables in *.c files

[Keith Owens]

On Wed, 02 Jan 2002 23:56:25 -0800, 
Andrew Morton <akpm@zip.com.au> wrote:
>Yup.  Problem is, we have about 1500 instances in the kernel :(

You can ignore the ~250 entries in *syms.c files.  EXPORT_SYMBOL only
needs to know if a symbol is a function or anything else, it does not
care about types at all.  You can define variables and functions with
invalid types in *syms.c without doing any damage.

Re: Extern variables in *.c files

[Russell King]

On Wed, Jan 02, 2002 at 11:56:25PM -0800, Andrew Morton wrote:
> Oh well.  Seems that disabling -fno-common and enabling
> --warn-common is the only way to autodetect bugs such as this.

You open another can of worms with variables in drivers that should be
static that aren't - you end up with no way to detect these without
-fno-common.

So, you have a choice:
1. Enable -fno-common
   - detect variables that should be marked static which aren't
   - don't detect size differences
2. Disable -fno-common
   - don't detect variables that should be marked static
   - detect size differences as long as the variables aren't marked extern

As soon as someone has int foo in one file, and extern char foo in another,
you've lost no matter which option you take.

The header file approach is the most reliable (and imho correct) method to
solve this problem.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: Extern variables in *.c files (maintainers pls read this)

[vda]

> So, you have a choice:
> 1. Enable -fno-common
>    - detect variables that should be marked static which aren't
>    - don't detect size differences
> 2. Disable -fno-common
>    - don't detect variables that should be marked static
>    - detect size differences as long as the variables aren't marked extern
>
> As soon as someone has int foo in one file, and extern char foo in another,
> you've lost no matter which option you take.
>
> The header file approach is the most reliable (and imho correct) method to
> solve this problem.

And this method is traditional for C. We have struct declarations and fn 
propotypes in *.h, we should place extern vars there too. Always.

If you are a kernel subsystem or driver maintainer, you may wish to check 
whether *your* part of kernel has any extern variable defs. Just run this
hunter script in top dir of kernel source:
-----------------------
#!/bin/sh

function do_grep() {
    pattern="$1"
    dir="$2"
    shift;shift
    
    for i in $dir/$*; do
        if ! test -d "$i"; then
            if test -e "$i"; then
		grep -E "$pattern" "$i" /dev/null
	    fi
        fi
    done
    for i in $dir/*; do
        if test -d "$i"; then
	    do_grep "$pattern" "$i" $*
	fi
    done
}

do_grep 'extern [^()]*;' . "*.c" 2>&1 | tee ../extern.log
---------------------------------

Output is not attached here, it's too big: ~100 KB, ~1500 lines for 2.5.1-pre8
--
vda

Re: Extern variables in *.c files (maintainers pls read this)

[Olaf Dietsche]

"vda@port.imtp.ilyichevsk.odessa.ua"  <vda@port.imtp.ilyichevsk.odessa.ua> writes:

> And this method is traditional for C. We have struct declarations and fn 
> propotypes in *.h, we should place extern vars there too. Always.

Agreed.

> If you are a kernel subsystem or driver maintainer, you may wish to check 
> whether *your* part of kernel has any extern variable defs. Just run this
> hunter script in top dir of kernel source:
> -----------------------
> #!/bin/sh
> 
> function do_grep() {
>     pattern="$1"
>     dir="$2"
>     shift;shift
>     
>     for i in $dir/$*; do
>         if ! test -d "$i"; then
>             if test -e "$i"; then
> 		grep -E "$pattern" "$i" /dev/null
> 	    fi
>         fi
>     done
>     for i in $dir/*; do
>         if test -d "$i"; then
> 	    do_grep "$pattern" "$i" $*
> 	fi
>     done
> }
> 
> do_grep 'extern [^()]*;' . "*.c" 2>&1 | tee ../extern.log
> ---------------------------------

FWIW, I suppose you meant:
$ find . -name '*.c' | xargs grep -E 'extern [^()]*;' 2>&1 | tee extern.log

Regards, Olaf.