void * vs. int

[Mark]

Hi

I encountered a piece of code in one of the linux kernel module, which I'm
to maintain. Briefly it is doing something like this:

....
typedef struct _msg_queue {
int priority;
int msg;
int arg1;
int arg2;
int arg3;
struct _msg_queue *prev;
struct _msg_queue *next;
} msg_queue_t;

struct sk_buff {
struct sk_buff *next;
struct sk_buff *prev;
/*...*/
};

struct sk_buff *sk;
msg_queue_t igmp_Q_msg;
....
sk = (struct skb *)igmp_Q_msg.arg1; /* xxx */

'xxx' isn't complaining. Is this normal?
I would declare 'argX' members of the structure _msg_queue as 'void *'.

 

[Seebs]

I encountered a piece of code in one of the linux kernel module, which I'm
to maintain.

Welcome to the land of deeply system-specific magic.

typedef struct _msg_queue { ....
int arg1; ....
} msg_queue_t;

struct sk_buff {
/*...*/
};

struct sk_buff *sk;
msg_queue_t igmp_Q_msg;
...
sk = (struct skb *)igmp_Q_msg.arg1; /* xxx */

'xxx' isn't complaining. Is this normal?

With a cast in place, it is pretty normal, unless you're compiling
on a target where int and pointer are not the same width, in which case
I think at least one common compiler (hint: the only one most people
will ever use to compile the Linux kernel...) would be giving you a
warning even WITH the cast.

I would declare 'argX' members of the structure _msg_queue as 'void *'.

I probably would too. I would guess, just off the top of my head, that
there probably exist systems on which this code does not work as desired.
However, you're in deep magic land; kernel code is one of the places where
it may in fact be reasonable to figure out what the hardware you're
supporting looks like and rely on it.

So, for instance, if that module is part of a driver for a piece of hardware
which is physically incapable of being used except on a 32-bit machine (one
where both int and pointers are 32 bits, and pointers are just plain 32-bit
values with no particular magic), that may actually be a reasonable choice.

It's not how I'd do it. Being a curious sort, I've dropped a line to a
kernel expert I know who might be able to tell me whether there's some
reason I don't know of for which this should be trustworthy.

-s

 

[Stephen Sprunk]

I encountered a piece of code in one of the linux kernel module, which
I'm to maintain. Briefly it is doing something like this:

typedef struct _msg_queue {
int priority;
int msg;
int arg1;
...
sk = (struct skb *)igmp_Q_msg.arg1; /* xxx */

'xxx' isn't complaining. Is this normal?

The cast silences the warning, at least with GCC* on platforms where int
and (void *) are the same size.

Normally this would be a bad idea because it's unportable, but such
rules often go out the window when you're writing kernels, device
drivers, or other parts of "the implementation" because they are
inherently unportable anyway.

I would declare 'argX' members of the structure _msg_queue as 'void *'.

So would I. I have no problem with unportable code when necessary, but
given the limited context above, this case appears to be gratuitous.

S

(* And presumably ICC; one of Intel's main release criteria is being
able to compile the Linux/x86 kernel correctly, which means duplicating
the various GCC extensions and other oddities the kernel depends on.)

 

[Michael Tsang]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi

I encountered a piece of code in one of the linux kernel module, which I'm
to maintain. Briefly it is doing something like this:

...
typedef struct _msg_queue {
int priority;
int msg;
int arg1;
int arg2;
int arg3;
struct _msg_queue *prev;
struct _msg_queue *next;
} msg_queue_t;

struct sk_buff {
struct sk_buff *next;
struct sk_buff *prev;
/*...*/
};

struct sk_buff *sk;
msg_queue_t igmp_Q_msg;
...
sk = (struct skb *)igmp_Q_msg.arg1; /* xxx */

'xxx' isn't complaining. Is this normal?
I would declare 'argX' members of the structure _msg_queue as 'void *'.

Does the code work on 64-bit systems where int is 32-bit but pointers are
64-bit?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iEYEARECAAYFAkr9YEgACgkQG6NzcAXitM/PxgCgjVv3MeHeHG5wsfi/+6OzI9WY
wdEAoIntKn+5VEonKNcOeCtYDkFcjgXP
=7VRN
-----END PGP SIGNATURE-----

 

[Seebs]

Design a better data structure and design a proper interface to it.

One of the reasons this may be impossible is that in many cases, the
kernel has to implement a very specific interface -- one where the exact
arrangement of bits has been Handed Down From On High, and may not be altered.

At which point, the ability of C to specify what you mean to do, despite
the fact that it's obviously wrong, can be a considerable asset.

-s

 

[Seebs]

Then you need another level of indirection - to fix the On High.

Maybe. The problem, of course, is that if that level of indirection
adds a noticeable cost to a core execution path, it may not be acceptable.

There are places where Crazy Stuff like this is done, and done on purpose.
I don't know whether this is one.

I can see two ways of taking that: (1) it's inherently
self-contradictory because it claims that getting something wrong is
an asset, or (2) it's a rather odd re-statement of Doug Gwyn's (to
me, self-evidently true) statement that when a system (he was talking
about Unix, I think, but it applies to C too) makes it impossible to
do stupid things, it also makes it impossible to do clever things.

I hope you mean (2), but I have an uneasy feeling that you don't.

More like two. The key is an (intentional, mostly) equivocation on "wrong".
Casting an int to a pointer is "obviously wrong". However, there are several
million pieces of hardware out there, for which at least one major ABI is
such that, in fact, it is precisely what you ought to do in at least one
circumstance.

In which case, having the option of doing something because you know something
about the target that's not intrinsic to the language spec might be
reasonable.

I have a hunk of code which defines a function called fopen(). It is quite
obvious to even the most casual observer that this is necessarily incorrect,
but it happens that I really do need to do that, and it really does do
what it's supposed to do. It is obviously wrong; it is non-obviously right.

-s

 

[Phil Carmody]

Presumably you mean:

sk = (struct sk_buff *)igmp_Q_msg.arg1; /* xxx */


The compiler would be required to issue a diagnosis, but the cast
means it doesn't have to. The code is still broken, though. It's like
finding a penny in the fusebox instead of a fuse. It "works" -
electricity flows - but you know it's not right.


So would I. And I would follow the consequences of that decision all
the way to a complete rewrite of the kernel if necessary. Hopefully,
this time round it'll be done right.

So how do I convert a physical address into a page number using
portable C? Either I shift the pointer by PAGE_SHIFT bits, which
is impossible, or I deal with addresses as integers instead, which
is possible. Your recommendation between those two would be?

(This is not knowing or caring which module he's looking at, but
I know that, as I've been working on the DSP bridge today, I've
been passing around 32-bit unsigned integers in order to identify
pages of RAM, and I certainly wouldn't want to swap them for
void*.)

And whilst you're filling me in on how to best deal with these matters,
how does C distinguish between logical addresses, linear addresses,
and physical addresses, again? A quick grep doesn't find anything in
either standards document I have access to.

Phil

 

[Seebs]

So how do I convert a physical address into a page number using
portable C? Either I shift the pointer by PAGE_SHIFT bits, which
is impossible, or I deal with addresses as integers instead, which
is possible. Your recommendation between those two would be?

You don't. You use a predefined macro or function to do it.

That said, I checked with one of our kernel people, and he confirms that
this is almost certainly just asking for trouble in a case like this -- it
won't work on most targets.

.... On the other hand, what if it's the driver for the
physically-part-of-the-CPU ethernet on a 32-bit SoC? Then it's probably
harmless.

-s

 

[Eric Sosman]

[...]
At which point, the ability of C to specify what you mean to do,
despite the fact that it's obviously wrong, can be a considerable
asset.

I can see two ways of taking that: (1) it's inherently
self-contradictory because it claims that getting something wrong is
an asset, or (2) it's a rather odd re-statement of Doug Gwyn's (to
me, self-evidently true) statement that when a system (he was talking
about Unix, I think, but it applies to C too) makes it impossible to
do stupid things, it also makes it impossible to do clever things.

I hope you mean (2), but I have an uneasy feeling that you don't.

He seems closer to (1), but that doesn't bother me much.
As the Rationale says, "the ability to write machine-specific
code is one of the strengths of C." Obviously -- trivially! --
machine-specific code sacrifices a lot of portability, but an
O/S kernel already makes a few concessions in that area anyhow.

It's been said that "C combines the power of assembler with
the portability of assembler," which needn't always be the case
but is sometimes near the truth. This looks like one of those
"sometimes."

 

[Robert Latest]

So would I. And I would follow the consequences of that decision all
the way to a complete rewrite of the kernel if necessary. Hopefully,
this time round it'll be done right.

Except that, seeing that this comes from a kernel module, there may be
very good (i.e., hardware-specific) reasons to do it the way it's been
done. But then the reasons should be made clear in comments.

robert

 

[Phil Carmody]

You don't. You use a predefined macro or function to do it.

Presumably your smiley it to make pretending that I'm not the one
writing that macro, or function easier?

That said, I checked with one of our kernel people, and he confirms that
this is almost certainly just asking for trouble in a case like this -- it
won't work on most targets.

'This' is singular. I mentioned 2 things (use of pointers, or
use of integers). 2 is plural.

... On the other hand, what if it's the driver for the
physically-part-of-the-CPU ethernet on a 32-bit SoC? Then it's probably
harmless.

Alas that's wrong. The instructions the CPU uses in all cases that I
can think of do not distinguish between memory mapped memory, and
memory mapped peripherals (and yes, they are peripherals, your concept
that they are 'part of the CPU' is a pretty wacky one; clue - the 'C'
in 'SoC' does not stand for 'CPU'), and therefore there must still be
a correctly-arranged logical-address to physical-address mapping.
Just setting a pointer up to the numerical value of the physical
address is quite the opposite of harmless, it's probably fatal.

Phil

 

[Seebs]

Presumably your smiley it to make pretending that I'm not the one
writing that macro, or function easier?

In general, I would expect that either:
1. You are working within an existing body of code, such as a kernel,
that has almost certainly provided a way to perform that calculation
so that you don't need to know how to do it.
2. You are the one writing that, and you can make suitable decisions
for your targets.

In general, that's the kind of thing where the answer is "the portable
thing to do is include a header which is architecture-specific."

'This' is singular. I mentioned 2 things (use of pointers, or
use of integers). 2 is plural.

Sorry, was referring to the "this" as in "the use of ints rather
than some other type as the thing used to stash a pointer value".

Alas that's wrong. The instructions the CPU uses in all cases that I
can think of do not distinguish between memory mapped memory, and
memory mapped peripherals (and yes, they are peripherals, your concept
that they are 'part of the CPU' is a pretty wacky one; clue - the 'C'
in 'SoC' does not stand for 'CPU'), and therefore there must still be
a correctly-arranged logical-address to physical-address mapping.

Oh, certainly.

My point is, if you know exactly what CPU your code will execute on, you
don't have to worry about whether or not it's portable to assume that an
int can contain a pointer -- you know for sure whether it can or can't.

Just setting a pointer up to the numerical value of the physical
address is quite the opposite of harmless, it's probably fatal.

This gets close to one of the reasons people sometimes stash values
in non-pointer objects. There have existed systems on which "pointers"
were loaded into registers such that loading an invalid value caused
a trap. On such a system, if you had magic cookies which might
represent a valid pointer some of the time, and not at other times (say,
depending on what you last programmed the MMU for), the natural idiom
would be to stash those cookies in a suitably-sized int, then, when you
are ready to use them (and unbeknownst to the compiler, you know that
the MMU is in a state where they work), you cast them over to pointers
and they work.

So it is *conceivable* that this code is not stupid. I'm not willing to
say anything more complimentary about it without a lot more information
than I have. I'm not going to argue that it could ever be good style;
even if it's technically correct for all systems it can possibly be
run on, it's still ugly.

-s