Linux Kernel Mailing List wrote:
> --- a/include/linux/compiler.h 2004-09-11 00:26:40 -07:00
> +++ b/include/linux/compiler.h 2004-09-11 00:26:40 -07:00
> @@ -6,13 +6,17 @@
> # define __kernel /* default address space */
> # define __safe __attribute__((safe))
> # define __force __attribute__((force))
> +# define __iomem __attribute__((noderef, address_space(2)))
Dumb gcc attribute questions:
1) what does force do? it doesn't appear to be in gcc 3.3.3 docs.
2) is "volatile ... __force" redundant?
3) can we use 'malloc' attribute on kmalloc?
On Sun, 12 Sep 2004 20:26:38 -0400
Jeff Garzik <[email protected]> wrote:
> Dumb gcc attribute questions:
>
> 1) what does force do? it doesn't appear to be in gcc 3.3.3 docs.
>
> 2) is "volatile ... __force" redundant?
>
> 3) can we use 'malloc' attribute on kmalloc?
Jeff, this code you quoted is in the sparse ifdef block.
On Sun, 12 Sep 2004, Jeff Garzik wrote:
>
> Linux Kernel Mailing List wrote:
> > --- a/include/linux/compiler.h 2004-09-11 00:26:40 -07:00
> > +++ b/include/linux/compiler.h 2004-09-11 00:26:40 -07:00
> > @@ -6,13 +6,17 @@
> > # define __kernel /* default address space */
> > # define __safe __attribute__((safe))
> > # define __force __attribute__((force))
> > +# define __iomem __attribute__((noderef, address_space(2)))
>
> Dumb gcc attribute questions:
>
> 1) what does force do? it doesn't appear to be in gcc 3.3.3 docs.
It doesn't do anything for gcc. You're looking at the sparse-only code.
What "attribute((force))" does for sparse is to mark a type to be
"forced", ie a cast to a forced type will not complain even if the cast
otherwise would be invalid.
For example, "sparse" will warn about explicit casts that drop address
space information:
void __user *userptr;
...
memset((void *)userptr, 0, ...)
will cause a
warning: cast removes address space of expression
complaint from sparse. But some _internal_ functions want to force the
cast because they know it's safe. For example, you'll find
#define __addr_ok(addr) ((unsigned long __force)(addr) < (current_thread_info()->addr_limit.seg))
because this internal x86 implementation detail knows that in that
particular case it's safe to remove the address space information (it's
just checking the user pointer against the address limit).
For gcc, none of this means anything, so all the #define's just become
empty.
> 2) is "volatile ... __force" redundant?
No, although it's likely to be a strange combination. If you want to force
a static address space conversion to a volatile pointer, you can do so. I
don't see _why_ you'd want to do it ;)
> 3) can we use 'malloc' attribute on kmalloc?
Since we can't use the gcc alias analysis anyway (it's too broken until
very late gcc versions), the gcc 'malloc' attribute shouldn't make any
difference that I can tell.
But there wouldn't be anything _wrong_ in adding it to kmalloc(), if
that's what you're asking.
Linus
Linus Torvalds wrote:
> On Sun, 12 Sep 2004, Jeff Garzik wrote:
>>1) what does force do? it doesn't appear to be in gcc 3.3.3 docs.
> It doesn't do anything for gcc. You're looking at the sparse-only code.
doh, and thanks for the info.
>>2) is "volatile ... __force" redundant?
> No, although it's likely to be a strange combination. If you want to force
> a static address space conversion to a volatile pointer, you can do so. I
> don't see _why_ you'd want to do it ;)
Well the reason I ask....
static inline void writeb(unsigned char b, volatile void __iomem *addr)
{
*(volatile unsigned char __force *) addr = b;
}
static inline void writew(unsigned short b, volatile void __iomem *addr)
{
*(volatile unsigned short __force *) addr = b;
}
static inline void writel(unsigned int b, volatile void __iomem *addr)
{
*(volatile unsigned int __force *) addr = b;
}
>>3) can we use 'malloc' attribute on kmalloc?
> Since we can't use the gcc alias analysis anyway (it's too broken until
> very late gcc versions), the gcc 'malloc' attribute shouldn't make any
> difference that I can tell.
>
> But there wouldn't be anything _wrong_ in adding it to kmalloc(), if
> that's what you're asking.
That's what I'm asking.
Thanks,
Jeff
On Sun, 12 Sep 2004, Jeff Garzik wrote:
>
> > No, although it's likely to be a strange combination. If you want to force
> > a static address space conversion to a volatile pointer, you can do so. I
> > don't see _why_ you'd want to do it ;)
>
> Well the reason I ask....
>
> static inline void writeb(unsigned char b, volatile void __iomem *addr)
> {
> *(volatile unsigned char __force *) addr = b;
> }
Right. Let's look a bit closer (more of an explanation than you need, but
hey, maybe somebody else is also wondering):
- for gcc, none of this matters one whit. We're just passing in a
"volatile void *", and we're storing the value "b" to the byte pointed
by it. Which is correct on x86, since memory-mapped PCI-space just
looks like memory on x86.
This is important to remember: for gcc, the sparse annotations are
meaningless. They can still be useful just to tell the _programmer_
that "hey, that pointer you got wasn't a normal pointer" in a fairly
readable manner, but in the end, unless you use sparse, they don't
actually _do_ anything.
HOWEVER. When you _do_ use parse, it is another matter entirely. For
"sparse", that "__iomem" has lots of meaning:
# define __iomem __attribute__((noderef, address_space(2)))
ie "iomem" means two separate things: it means that sparse should complain
if the pointer is ever dereferenced (it's a "noderef" pointer) directly,
and it's in "address space 2" as opposed to the normal address space (0).
Now, that means that _sparse_ will complain if such a pointer is ever
passed into a function that wants a regular pointer (because it is _not_ a
normal pointer, and you obviously shouldn't do things like "strcmp()" etc
on it), and sparse will also complain if you try to cast it to another
pointer in another address space.
So if you compile and install sparse, and build with "make C=1", you'll
get warnings like
drivers/video/aty/radeon_base.c:1725:42: warning: incorrect type in argument 2 (different address spaces)
drivers/video/aty/radeon_base.c:1725:42: expected void const *from
drivers/video/aty/radeon_base.c:1725:42: got void [noderef] *[assigned] base_addr<asn:2>
which is just another way sparse tells you that there is a bug in the
source code (in this case, we try to copy from PCI memory-mapped space
directly to user space using "copy_to_user()", which is a _bad_ idea).
So not only can you not dereference it by mistake, you can't even _turn_
it into a pointer that you could dereference by mistake. Sparse will
complain. Sparse will complain even if you use an explicit cast to make it
a normal "(void *)".
And that's good, because on some other architectures, if you try to
dereference the pointer, the machine just oopses. You need to do all the
special magic to actually read from memory-mapped PCI space.
HOWEVER. On x86, it just so happens that dereferencing the pointer _is_
actually the right thing to do, as long as you only do it with the proper
interfaces (ie readb/writeb and friends). And so that sparse won't be
upset, we use the "__force" directive - we're telling sparse that "I know
what I'm doing". And so sparse will quietly allow us to dereference that
pointer that was originally not dereferencable.
Generally, you shouldn't ever use __force in a driver or anything like
that. It's usually a valid thing to do only in the code that is defined
for that particular type. By definition, a driver isn't an entity that
should understand how architecture-specific data structures work, so it
shouldn't try to force things.
So a good use of "__force" is exactly the usage you quote: the
arch-specific code that actually really _knows_ what the magic address
space means, and knows what to do about it.
Linus
While resuming adding __user annotations to the m68k-specific parts of the
code, I stumbled on
struct task_struct {
...
unsigned long sas_ss_sp;
...
}
If I'm not mistaken, sas_ss_sp is always a pointer to user stack space.
Shouldn't it be changed to `void __user *sas_ss_sp', or is an
unsigned long/void * change in generic code a too controversial change for
making sparse happy?
And I guess I can find a few more of these...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Mon, 13 Sep 2004, Geert Uytterhoeven wrote:
>
> While resuming adding __user annotations to the m68k-specific parts of the
> code, I stumbled on
>
> struct task_struct {
> ...
> unsigned long sas_ss_sp;
> ...
> }
>
> If I'm not mistaken, sas_ss_sp is always a pointer to user stack space.
> Shouldn't it be changed to `void __user *sas_ss_sp', or is an
> unsigned long/void * change in generic code a too controversial change for
> making sparse happy?
I don't think it's too controversial per se, and it would certainly remove
at least two casts from kernel/signal.c. Are you ready to fix up some
other architectures from the fall-out (x86 at the least..)
Linus
Salut,
On Sun, Sep 12, 2004 at 08:00:48PM -0700, Linus Torvalds wrote:
> Generally, you shouldn't ever use __force in a driver or anything like
> that.
Why don't we send the __force attribute into some #ifdef that is never
defined unless you're in arch specific code? This way we'd prevent
stupid people from doing stupid things.
Tonnerre
On Mon, 13 Sep 2004, Tonnerre wrote:
>
> On Sun, Sep 12, 2004 at 08:00:48PM -0700, Linus Torvalds wrote:
> > Generally, you shouldn't ever use __force in a driver or anything like
> > that.
>
> Why don't we send the __force attribute into some #ifdef that is never
> defined unless you're in arch specific code? This way we'd prevent
> stupid people from doing stupid things.
Hey, the thing is, that may well prevent smart people from doing smart
things too. Give 'em rope, within reason.
The point behind __force is not so much that you should never use it, but
that you should never use it by _mistake_. It's very easy (and often
_required_) to have a regular typecast in C, and it can often hide bugs
when you cast something in a way that you didn't really think through.
For example, we often have generic "void *" or "unsigned long" things that
are used for passing opaque data around, and they need casts when working
with them. It is quite conceivable that you need an address space cast
too, and that you need to use "__force" to do so. It might be ok, even in
a driver. But hopefully it's something where the action of having to force
the cast makes people think about it more. And the fact that there
probably never will be very _man_ casts like that means that they'll stand
out.
If people start using "__force" to hide their own bugs, we'll just have to
start stringing them up. Hang'em high, I say. But maybe somebody has a
valid reason at times.
Linus
On Mon, Sep 13, 2004 at 08:31:26PM +0200, Tonnerre wrote:
> Salut,
>
> On Sun, Sep 12, 2004 at 08:00:48PM -0700, Linus Torvalds wrote:
> > Generally, you shouldn't ever use __force in a driver or anything like
> > that.
>
> Why don't we send the __force attribute into some #ifdef that is never
> defined unless you're in arch specific code? This way we'd prevent
> stupid people from doing stupid things.
man grep