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
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.."
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?
-
>>>>> "Andrew" == Andrew Morton <[email protected]> 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
Momchil Velikov wrote:
>
> >>>>> "Andrew" == Andrew Morton <[email protected]> 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?
-
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.
"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.
-
On Wed, 02 Jan 2002 23:56:25 -0800,
Andrew Morton <[email protected]> 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.
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 ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
> 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
"[email protected]" <[email protected]> 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.