unhappy with current.h

unhappy with current.h

[Daniele Lugli]

I recently wrote a kernel module which gave me some mysterious problems.
After too many days spent in blood, sweat and tears, I found the cause:

*** one of my data structures has a field named 'current'. ***

Pretty common word, isn't it? Would you think it can cause such a
trouble? But in some of my files I happen to indirectly include
<asm/current.h> (kernel 2.4.18 for i386), containing the following line:

#define current get_current()

so that my structure becomes the owner of a function it has never asked
for, while it looses a data member. gcc has nothing to complain about
that.

In some other files I don't happen to include <asm/current.h>, so that
there my structure is sound - but alas! has different size and different
composition. Again, gcc has nothing to complain.

Moral of the story: in my opinion kernel developers should reduce to a
minimum the use of #define, and preferably use words in uppercase and/or
with underscores, in any case not commonly used words.
In the specific case the said #define just looks to save 6 keystrokes
and I think it could have been completely avoided.

Regards, Daniele Lugli

Re: unhappy with current.h

[David S. Miller]

   From: Daniele Lugli <genlogic@inrete.it>
   Date: Mon, 14 Oct 2002 21:46:08 +0200
   
   Moral of the story: in my opinion kernel developers should reduce to a
   minimum the use of #define, and preferably use words in uppercase and/or
   with underscores, in any case not commonly used words.

Or maybe you should change your datastructure to not have member names
the conflict with 7 year old well defined global symbols in the Linux
kernel?

Re: unhappy with current.h

[Daniele Lugli]

"David S. Miller" wrote:
> 
>    From: Daniele Lugli <genlogic@inrete.it>
>    Date: Mon, 14 Oct 2002 21:46:08 +0200
> 
>    Moral of the story: in my opinion kernel developers should reduce to a
>    minimum the use of #define, and preferably use words in uppercase and/or
>    with underscores, in any case not commonly used words.
> 
> Or maybe you should change your datastructure to not have member names
> the conflict with 7 year old well defined global symbols in the Linux
> kernel?

That's what I did.

What about, next time,

#define i j

??

Regards, DL

Re: unhappy with current.h

[Chris Wedgwood]

On Mon, Oct 14, 2002 at 09:46:08PM +0200, Daniele Lugli wrote:

> I recently wrote a kernel module which gave me some mysterious
> problems. After too many days spent in blood, sweat and tears, I found the cause:

> *** one of my data structures has a field named 'current'. ***

gcc -Wshadow



  --cw

Re: unhappy with current.h

[Rik van Riel]

On Mon, 14 Oct 2002, Chris Wedgwood wrote:
> On Mon, Oct 14, 2002 at 09:46:08PM +0200, Daniele Lugli wrote:
>
> > I recently wrote a kernel module which gave me some mysterious
> > problems. After too many days spent in blood, sweat and tears, I found the cause:
>
> > *** one of my data structures has a field named 'current'. ***
>
> gcc -Wshadow

Would it be a good idea to add -Wshadow to the kernel
compile options by default ?

Rik
-- 
Bravely reimplemented by the knights who say "NIH".
http://www.surriel.com/		http://distro.conectiva.com/
Current spamtrap:  <a href=mailto:"october@surriel.com">october@surriel.com</a>

Re: unhappy with current.h

[Chris Wedgwood]

On Mon, Oct 14, 2002 at 10:00:26PM -0200, Rik van Riel wrote:

> Would it be a good idea to add -Wshadow to the kernel compile
> options by default ?

Yes, I think so.  Last time I tried this it produced quite a bit of
output.


  --cw

Re: unhappy with current.h

[Murray J. Root]

On Mon, Oct 14, 2002 at 10:00:26PM -0200, Rik van Riel wrote:
> On Mon, 14 Oct 2002, Chris Wedgwood wrote:
> > On Mon, Oct 14, 2002 at 09:46:08PM +0200, Daniele Lugli wrote:
> >
> > > I recently wrote a kernel module which gave me some mysterious
> > > problems. After too many days spent in blood, sweat and tears, I found the cause:
> >
> > > *** one of my data structures has a field named 'current'. ***
> >
> > gcc -Wshadow
> 
> Would it be a good idea to add -Wshadow to the kernel
> compile options by default ?
> 

Yes. 

-- 
Murray J. Root
------------------------------------------------
DISCLAIMER: http://www.goldmark.org/jeff/stupid-disclaimers/
------------------------------------------------
Mandrake on irc.freenode.net:
  #mandrake & #mandrake-linux = help for newbies 
  #mdk-cooker = Mandrake Cooker 

Re: unhappy with current.h

[Mikael Pettersson]

Rik van Riel writes:
 > On Mon, 14 Oct 2002, Chris Wedgwood wrote:
 > > On Mon, Oct 14, 2002 at 09:46:08PM +0200, Daniele Lugli wrote:
 > >
 > > > I recently wrote a kernel module which gave me some mysterious
 > > > problems. After too many days spent in blood, sweat and tears, I found the cause:
 > >
 > > > *** one of my data structures has a field named 'current'. ***
 > >
 > > gcc -Wshadow
 > 
 > Would it be a good idea to add -Wshadow to the kernel
 > compile options by default ?

While I'm not defending macro abuse, please note that Daniele's problem
appears to have been caused by using g++ instead of gcc or gcc -x c to
compile a kernel module. Daniele's later example throws a syntax error
in gcc, since the cpp output isn't legal C ...

Hence I fail to see the utility of hacking in kludges for something
that's not supposed to work anyway.

Re: unhappy with current.h

[Daniele Lugli]

Mikael Pettersson wrote:
> 
> Rik van Riel writes:
>  > On Mon, 14 Oct 2002, Chris Wedgwood wrote:
>  > > On Mon, Oct 14, 2002 at 09:46:08PM +0200, Daniele Lugli wrote:
>  > >
>  > > > I recently wrote a kernel module which gave me some mysterious
>  > > > problems. After too many days spent in blood, sweat and tears, I found the cause:
>  > >
>  > > > *** one of my data structures has a field named 'current'. ***
>  > >
>  > > gcc -Wshadow
>  >
>  > Would it be a good idea to add -Wshadow to the kernel
>  > compile options by default ?
> 
> While I'm not defending macro abuse, please note that Daniele's problem
> appears to have been caused by using g++ instead of gcc or gcc -x c to
> compile a kernel module. Daniele's later example throws a syntax error
> in gcc, since the cpp output isn't legal C ...
> 
> Hence I fail to see the utility of hacking in kludges for something
> that's not supposed to work anyway.

Yes i confess, i'm writing a kernel module in c++ (and i'm not the only
one).
Anyway my consideration was general and IMHO applies to C too. What is
the benefit of redefining commonly used words? I would say nothing
against eg #define _I386_current get_current(), but just #define current
get_current() seems to me a little bit dangerous. What is the limit?
What do you consider a bad practice? Would #define i j be tolerated?

I wouldn't like to open a discussion about using c++ for kernel modules
because i know that the large majority of kernel developers is against.
And in fact i had to make my own version of the kernel sources turning
all 'new' into 'nEw', 'class' into 'klass' and so on, but this last
problem was more subtle.

But let me at least summarize my poor-programmer-not-kernel-developer
point of view: at present the kernel if a mined field for c++ and i
understand it is not viable nor interesting for the majority to rewrite
it in a more c++-friendly way. But why not at least keep in mind, while
writing new stuff (not the case of current.h i see), that kernel headers
could be included by c++?

Regards, Daniele

Re: unhappy with current.h

[Alexander Viro]

On Tue, 15 Oct 2002, Daniele Lugli wrote:

> But let me at least summarize my poor-programmer-not-kernel-developer
> point of view: at present the kernel if a mined field for c++ and i
> understand it is not viable nor interesting for the majority to rewrite
> it in a more c++-friendly way. But why not at least keep in mind, while
> writing new stuff (not the case of current.h i see), that kernel headers
> could be included by c++?

current.h is, indeed, a special case.  #define i j would not be tolerated
simply because it's stupid.  Abuses of preprocessor are generally frowned
upon, but there are passionate wa^H^Hpersons who just can't help themselves
and use e.g. ## for no good reason.

But as for C++... frankly, for all I care it doesn't exist.  As long as
requirements of style happen to reduce problems of C++ programmers -
they are lucky.  But other than that... watch me not care.  In the linux
kernel context C++ is obscure language and it will stay that way.  Ergo,
no reasons to spend any mental efforts on being nice to it.  Deal.

Re: unhappy with current.h

[Mikael Pettersson]

Daniele Lugli writes:
 > Mikael Pettersson wrote:
 > > 
 > > Rik van Riel writes:
 > >  > On Mon, 14 Oct 2002, Chris Wedgwood wrote:
 > >  > > On Mon, Oct 14, 2002 at 09:46:08PM +0200, Daniele Lugli wrote:
 > >  > >
 > >  > > > I recently wrote a kernel module which gave me some mysterious
 > >  > > > problems. After too many days spent in blood, sweat and tears, I found the cause:
 > >  > >
 > >  > > > *** one of my data structures has a field named 'current'. ***
 > >  > >
 > >  > > gcc -Wshadow
 > >  >
 > >  > Would it be a good idea to add -Wshadow to the kernel
 > >  > compile options by default ?
 > > 
 > > While I'm not defending macro abuse, please note that Daniele's problem
 > > appears to have been caused by using g++ instead of gcc or gcc -x c to
 > > compile a kernel module. Daniele's later example throws a syntax error
 > > in gcc, since the cpp output isn't legal C ...
 > > 
 > > Hence I fail to see the utility of hacking in kludges for something
 > > that's not supposed to work anyway.
 > 
 > Yes i confess, i'm writing a kernel module in c++ (and i'm not the only
 > one).
 > Anyway my consideration was general and IMHO applies to C too. What is
 > the benefit of redefining commonly used words? I would say nothing
 > against eg #define _I386_current get_current(), but just #define current
 > get_current() seems to me a little bit dangerous. What is the limit?
 > What do you consider a bad practice? Would #define i j be tolerated?

As I wrote above: "While I'm not defending macro abuse". #define i j is
definitely macro abuse, and no sane programmer should do it.

Any programming language has a set of reserved words, and any large piece
of software has its own reserved words. Think of C++ "this" or C typedef names,
for example. You don't expect new code to work if it violates the syntax of
the system in which it is compiled, do you?

"current" is just that: one of the Linux kernel's reserved words, one
that kernel programmers are supposed to know about.

 > But let me at least summarize my poor-programmer-not-kernel-developer
 > point of view: at present the kernel if a mined field for c++ and i
 > understand it is not viable nor interesting for the majority to rewrite
 > it in a more c++-friendly way. But why not at least keep in mind, while
 > writing new stuff (not the case of current.h i see), that kernel headers
 > could be included by c++?

1. The kernel is not written in C++.
2. C++ is not C, and a C++ compiler is not a substitute for a C compiler.
3. User-space should't include raw kernel headers but "sanitized" ones,
   provided e.g. by the C library.

Ergo, the kernel headers should never be processed by a C++ compiler, and
anyone doing it anyway is on their own.

/Mikael

Re: unhappy with current.h

[Richard B. Johnson]

On Mon, 14 Oct 2002, Daniele Lugli wrote:

> I recently wrote a kernel module which gave me some mysterious problems.
> After too many days spent in blood, sweat and tears, I found the cause:
> 
> *** one of my data structures has a field named 'current'. ***
> 
> Pretty common word, isn't it? Would you think it can cause such a
> trouble? But in some of my files I happen to indirectly include
> <asm/current.h> (kernel 2.4.18 for i386), containing the following line:
> 
> #define current get_current()
> 
> so that my structure becomes the owner of a function it has never asked
> for, while it looses a data member. gcc has nothing to complain about
> that.
>

This cannot be the reason for your problem. The name of a structure
member has no connection whatsoever with the name of any function or
definition.

The following code will correctly write "Hello world!" to the screen
even though the text initializes a member of a structure called "current"
while "current" has been defined to be a function called puts.


#include <stdio.h>
#define current puts
struct foo {
    char *current;
    int foo;
    } bar; 
main()
{
    bar.current = "Hello world!";
    current(bar.current);
    return 0;
}


For your code to get "confused", you really have something else
wrong. That said, some name-space polution may make it difficult
to find the problem. For instance, a structure member is expected
to have a ";" after it. It's possible for some previous definition
to make a syntax error invisible.


Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
The US military has given us many words, FUBAR, SNAFU, now ENRON.
Yes, top management were graduates of West Point and Annapolis.

Re: unhappy with current.h

[David S. Miller]

   From: "Richard B. Johnson" <root@chaos.analogic.com>
   Date: Mon, 14 Oct 2002 16:33:50 -0400 (EDT)
   
   This cannot be the reason for your problem. The name of a structure
   member has no connection whatsoever with the name of any function or
   definition.

try instead

#define current foo()

Re: unhappy with current.h

[Olivier Galibert]

On Mon, Oct 14, 2002 at 04:33:50PM -0400, Richard B. Johnson wrote:
> > #define current get_current()

> This cannot be the reason for your problem. The name of a structure
> member has no connection whatsoever with the name of any function or
> definition.

You forgot the effect of the parenthesis.  This isn't a name-changing
macro, this is a variable-to-function-call changing macro.

  OG.

Re: unhappy with current.h

[Richard B. Johnson]

On Mon, 14 Oct 2002, Olivier Galibert wrote:

> On Mon, Oct 14, 2002 at 04:33:50PM -0400, Richard B. Johnson wrote:
> > > #define current get_current()
> 
> > This cannot be the reason for your problem. The name of a structure
> > member has no connection whatsoever with the name of any function or
> > definition.
> 
> You forgot the effect of the parenthesis.  This isn't a name-changing
> macro, this is a variable-to-function-call changing macro.
> 
>   OG.
> 

Yep. But if the originator can not tried to compile kernel headers
with g++, an appropriate diagnostic would have been caught as shown.


Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
The US military has given us many words, FUBAR, SNAFU, now ENRON.
Yes, top management were graduates of West Point and Annapolis.

Re: unhappy with current.h

[Daniele Lugli]

"Richard B. Johnson" wrote:
> 
> On Mon, 14 Oct 2002, Daniele Lugli wrote:
> 
> > I recently wrote a kernel module which gave me some mysterious problems.
> > After too many days spent in blood, sweat and tears, I found the cause:
> >
> > *** one of my data structures has a field named 'current'. ***
> >
> > Pretty common word, isn't it? Would you think it can cause such a
> > trouble? But in some of my files I happen to indirectly include
> > <asm/current.h> (kernel 2.4.18 for i386), containing the following line:
> >
> > #define current get_current()
> >
> > so that my structure becomes the owner of a function it has never asked
> > for, while it looses a data member. gcc has nothing to complain about
> > that.
> >
> 
> This cannot be the reason for your problem. The name of a structure
> member has no connection whatsoever with the name of any function or
> definition.
> 
> The following code will correctly write "Hello world!" to the screen
> even though the text initializes a member of a structure called "current"
> while "current" has been defined to be a function called puts.
> 
> #include <stdio.h>
> #define current puts
> struct foo {
>     char *current;
>     int foo;
>     } bar;
> main()
> {
>     bar.current = "Hello world!";
>     current(bar.current);
>     return 0;
> }
> 
> For your code to get "confused", you really have something else
> wrong. That said, some name-space polution may make it difficult
> to find the problem. For instance, a structure member is expected
> to have a ";" after it. It's possible for some previous definition
> to make a syntax error invisible.
> 
> Cheers,
> Dick Johnson

Try the following instead:

// file shade1.cpp

// excerpt from common.h

#define current get_current()

// my stuff

struct {
  int any;
  int current[1000];
} mine;


// file shade2.cpp

#include <stdio.h>

// my stuff

extern struct {
  int any;
  int current[1000];
} mine;

int main () {
  mine.current[999] = 1;
  printf ("%d\n", mine.current[999]);
}

g++ shade1.cpp shade2.cpp

./a.out => segmentation fault

Regards, Daniele

Re: unhappy with current.h

[Richard B. Johnson]

On Mon, 14 Oct 2002, Daniele Lugli wrote:

> 
> Try the following instead:
> 
> // file shade1.cpp
> 
> // excerpt from common.h
> 
> #define current get_current()
> 
> // my stuff
> 
> struct {
>   int any;
>   int current[1000];
> } mine;
> 
> 
> // file shade2.cpp
> 
> #include <stdio.h>
> 
> // my stuff
> 
> extern struct {
>   int any;
>   int current[1000];
> } mine;
> 
> int main () {
>   mine.current[999] = 1;
>   printf ("%d\n", mine.current[999]);
> }
> 
> g++ shade1.cpp shade2.cpp
> 
> ./a.out => segmentation fault
> 
> Regards, Daniele
> 

It should have written a diagnostic which it does in C, not C++.

xxx.c:6: `get_current' declared as function returning an array
xxx.c:6: field `get_current' declared as a function

This () make all the difference. If you are trying to write kernel
drivers in C++, you will have more suprises. If not, you should
not be including kernel headers.


Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
The US military has given us many words, FUBAR, SNAFU, now ENRON.
Yes, top management were graduates of West Point and Annapolis.

Re: unhappy with current.h

[Andi Kleen]

Daniele Lugli <genlogic@inrete.it> writes:

> *** one of my data structures has a field named 'current'. ***
> 
> Pretty common word, isn't it? Would you think it can cause such a
> trouble? But in some of my files I happen to indirectly include
> <asm/current.h> (kernel 2.4.18 for i386), containing the following line:
> 
> #define current get_current()

How about changing the definition to: 

#define current ((struct task_struct *)get_current()) 

That should get the same effect as currently for kernel code, but 
will guarantee a syntax error if it's used in a structure declaration.

-Andi

Re: unhappy with current.h

[Chris Wedgwood]

On Mon, Oct 14, 2002 at 10:45:30PM +0200, Andi Kleen wrote:

> How about changing the definition to:

> #define current ((struct task_struct *)get_current())

Is there some magic way to make it an inline function?  That way
collisions would be at the compiler level as opposed to the
preprocessor level (pretending there is a strong separation here).


  --cw

Re: unhappy with current.h

[Daniele Lugli]

Frank Davis wrote:
> 
> Daniele,
>   Its easier for you to modify your code than to ask the kernel developers to modify a commonly used, intangled macro within the Linux kernel. :)
> 
> Regards,
> Frank

Thank you for your calm reply. You're right, and in fact that's what I
did before sending my mail. I'm not asking to modify an existing and
(unhappily) well-established kernel file; I am only giving a suggestion
for the future - if a common mortal human being can give suggestions.

Regards, Daniele Lugli

Re: unhappy with current.h

[Jeffrey Lim]

On Mon, 14 Oct 2002 12:52:34 -0700 (PDT), "David S. Miller"
<davem@redhat.com> said:
>
>        From: Daniele Lugli <genlogic@inrete.it>
>        Date: Mon, 14 Oct 2002 21:46:08 +0200
>    
>        Moral of the story: in my opinion kernel developers should reduce to a
>        minimum the use of #define, and preferably use words in uppercase
>        and/or with underscores, in any case not commonly used words.
> 
> Or maybe you should change your datastructure to not have member names
> the conflict with 7 year old well defined global symbols in the Linux
> kernel?
>

I guess tradition may have been well established. Is there an faq for
this somewhere (for these "well defined global symbols", that is) that we
could perhaps refer to?

I do believe he has a point though. It would be good to watch the
#defines in the future, so as to make it easier for aspiring coders.

-jf
 
#!/bin/signoff
# ~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~
echo The reasonable man adapts himself to the world; the unreasonable one
echo persists in trying to adapt the world to himself. Therefore, all
echo progress depends on the unreasonable.
echo                                              — George Bernard Shaw
# ~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~-~