Hi,
It looks like a compiler bug to me...
Some users have complained that when the following code is
compiled without the -fno-strict-aliasing, the order of the write and
memcpy is inverted (which mean a bogus len is mem-copied into the
stream).
Code (from linux/include/net/iw_handler.h) :
--------------------------------------------
static inline char *
iwe_stream_add_event(char * stream, /* Stream of events */
char * ends, /* End of stream */
struct iw_event *iwe, /* Payload */
int event_len) /* Real size of payload */
{
/* Check if it's possible */
if((stream + event_len) < ends) {
iwe->len = event_len;
memcpy(stream, (char *) iwe, event_len);
stream += event_len;
}
return stream;
}
--------------------------------------------
IMHO, the compiler should have enough context to know that the
reordering is dangerous. Any suggestion to make this simple code more
bullet proof is welcomed.
Have fun...
Jean
Jean Tourrilhes writes:
> It looks like a compiler bug to me...
> Some users have complained that when the following
> code is compiled without the -fno-strict-aliasing,
> the order of the write and memcpy is inverted (which
> mean a bogus len is mem-copied into the stream).
> Code (from linux/include/net/iw_handler.h) :
> --------------------------------------------
> static inline char *
> iwe_stream_add_event(char * stream, /* Stream of events */
> char * ends, /* End of stream */
> struct iw_event *iwe, /* Payload */
> int event_len) /* Real size of payload */
> {
> /* Check if it's possible */
> if((stream + event_len) < ends) {
> iwe->len = event_len;
> memcpy(stream, (char *) iwe, event_len);
> stream += event_len;
> } return stream;
> }
> --------------------------------------------
> IMHO, the compiler should have enough context to
> know that the reordering is dangerous. Any suggestion
> to make this simple code more bullet proof is welcomed.
>
> Have fun...
Since (char*) is special, I agree that it's a bug.
In any case, a warning sure would be nice!
Now for the fun. Pass iwe->len into this
macro before the memcpy, and all should be well.
#define FORCE_TO_MEM(x) asm volatile(""::"r"(&(x)))
Like this:
iwe->len = event_len;
FORCE_TO_MEM(iwe->len);
memcpy(stream, (char *) iwe, event_len);
Jean Tourrilhes <[email protected]> said:
> It looks like a compiler bug to me...
> Some users have complained that when the following code is
> compiled without the -fno-strict-aliasing, the order of the write and
> memcpy is inverted (which mean a bogus len is mem-copied into the
> stream).
> Code (from linux/include/net/iw_handler.h) :
> --------------------------------------------
> static inline char *
> iwe_stream_add_event(char * stream, /* Stream of events */
> char * ends, /* End of stream */
> struct iw_event *iwe, /* Payload */
> int event_len) /* Real size of payload */
> {
> /* Check if it's possible */
> if((stream + event_len) < ends) {
> iwe->len = event_len;
> memcpy(stream, (char *) iwe, event_len);
> stream += event_len;
> }
> return stream;
> }
> --------------------------------------------
> IMHO, the compiler should have enough context to know that the
> reordering is dangerous. Any suggestion to make this simple code more
> bullet proof is welcomed.
The compiler is free to assume char *stream and struct iw_event *iwe point
to separate areas of memory, due to strict aliasing.
--
Dr. Horst H. von Brand User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria +56 32 654239
Casilla 110-V, Valparaiso, Chile Fax: +56 32 797513
Horst von Brand <[email protected]> writes:
> Jean Tourrilhes <[email protected]> said:
> > if((stream + event_len) < ends) {
> > iwe->len = event_len;
> > memcpy(stream, (char *) iwe, event_len);
> > stream += event_len;
> > }
> > return stream;
> > }
>
> The compiler is free to assume char *stream and struct iw_event *iwe
> point to separate areas of memory, due to strict aliasing.
The relevant paragraph of the C99 standard is:
An object shall have its stored value accessed only by an lvalue
expression that has one of the following types:
-- a type compatible with the effective type of the object,
-- a qualified version of a type compatible with the effective type of
the object,
-- a type that is the signed or unsigned type corresponding to the
effective type of the object,
-- a type that is the signed or unsigned type corresponding to a
qualified version of the effective type of the object,
-- an aggregate or union type that includes one of the aforementioned
types among its members (including, recursively, a member of a
subaggregate or contained union), or
-- a character type.
I can't really spot any lvalue here that might violate this rule. It
would be nice if somebody could report a bug with a testcase.
--
Falk
On Wed, Feb 26, 2003 at 04:38:10PM +0100, Horst von Brand wrote:
> Jean Tourrilhes <[email protected]> said:
> > It looks like a compiler bug to me...
> > Some users have complained that when the following code is
> > compiled without the -fno-strict-aliasing, the order of the write and
> > memcpy is inverted (which mean a bogus len is mem-copied into the
> > stream).
> > Code (from linux/include/net/iw_handler.h) :
> > --------------------------------------------
> > static inline char *
> > iwe_stream_add_event(char * stream, /* Stream of events */
> > char * ends, /* End of stream */
> > struct iw_event *iwe, /* Payload */
> > int event_len) /* Real size of payload */
> > {
> > /* Check if it's possible */
> > if((stream + event_len) < ends) {
> > iwe->len = event_len;
> > memcpy(stream, (char *) iwe, event_len);
> > stream += event_len;
> > }
> > return stream;
> > }
> > --------------------------------------------
> > IMHO, the compiler should have enough context to know that the
> > reordering is dangerous. Any suggestion to make this simple code more
> > bullet proof is welcomed.
>
> The compiler is free to assume char *stream and struct iw_event *iwe point
> to separate areas of memory, due to strict aliasing.
Which is true and which is not the problem I'm complaining about.
Have fun...
Jean
In article <[email protected]>,
Jean Tourrilhes <[email protected]> wrote:
>
> It looks like a compiler bug to me...
Why do you think the kernel uses "-fno-strict-aliasing"?
The gcc people are more interested in trying to find out what can be
allowed by the c99 specs than about making things actually _work_. The
aliasing code in particular is not even worth enabling, it's just not
possible to sanely tell gcc when some things can alias.
> Some users have complained that when the following code is
>compiled without the -fno-strict-aliasing, the order of the write and
>memcpy is inverted (which mean a bogus len is mem-copied into the
>stream).
The "problem" is that we inline the memcpy(), at which point gcc won't
care about the fact that it can alias, so they'll just re-order
everything and claim it's out own fault. Even though there is no sane
way for us to even tell gcc about it.
I tried to get a sane way a few years ago, and the gcc developers really
didn't care about the real world in this area. I'd be surprised if that
had changed, judging by the replies I have already seen.
I'm not going to bother to fight it.
Linus
On Tue, Feb 25, 2003 at 11:33:09PM -0500, Albert Cahalan wrote:
> Jean Tourrilhes writes:
>
> > It looks like a compiler bug to me...
> > Some users have complained that when the following
> > code is compiled without the -fno-strict-aliasing,
> > the order of the write and memcpy is inverted (which
> > mean a bogus len is mem-copied into the stream).
> > Code (from linux/include/net/iw_handler.h) :
> > --------------------------------------------
> > static inline char *
> > iwe_stream_add_event(char * stream, /* Stream of events */
> > char * ends, /* End of stream */
> > struct iw_event *iwe, /* Payload */
> > int event_len) /* Real size of payload */
> > {
> > /* Check if it's possible */
> > if((stream + event_len) < ends) {
> > iwe->len = event_len;
> > memcpy(stream, (char *) iwe, event_len);
> > stream += event_len;
> > } return stream;
> > }
> > --------------------------------------------
> > IMHO, the compiler should have enough context to
> > know that the reordering is dangerous. Any suggestion
> > to make this simple code more bullet proof is welcomed.
> >
> > Have fun...
>
> Since (char*) is special, I agree that it's a bug.
> In any case, a warning sure would be nice!
>
> Now for the fun. Pass iwe->len into this
> macro before the memcpy, and all should be well.
>
> #define FORCE_TO_MEM(x) asm volatile(""::"r"(&(x)))
>
> Like this:
>
> iwe->len = event_len;
> FORCE_TO_MEM(iwe->len);
> memcpy(stream, (char *) iwe, event_len);
I'll try that, that sounds absolutely clever (but I only
understand half of it).
Thanks a lot !
Jean
On Wed, 26 Feb 2003, Jean Tourrilhes wrote:
> On Tue, Feb 25, 2003 at 11:33:09PM -0500, Albert Cahalan wrote:
> > Jean Tourrilhes writes:
> >
> > > It looks like a compiler bug to me...
> > > Some users have complained that when the following
> > > code is compiled without the -fno-strict-aliasing,
> > > the order of the write and memcpy is inverted (which
> > > mean a bogus len is mem-copied into the stream).
> > > Code (from linux/include/net/iw_handler.h) :
> > > --------------------------------------------
> > > static inline char *
> > > iwe_stream_add_event(char * stream, /* Stream of events */
> > > char * ends, /* End of stream */
> > > struct iw_event *iwe, /* Payload */
> > > int event_len) /* Real size of payload */
> > > {
> > > /* Check if it's possible */
> > > if((stream + event_len) < ends) {
> > > iwe->len = event_len;
> > > memcpy(stream, (char *) iwe, event_len);
> > > stream += event_len;
> > > } return stream;
> > > }
> > > --------------------------------------------
> > > IMHO, the compiler should have enough context to
> > > know that the reordering is dangerous. Any suggestion
> > > to make this simple code more bullet proof is welcomed.
> > >
> > > Have fun...
> >
> > Since (char*) is special, I agree that it's a bug.
> > In any case, a warning sure would be nice!
> >
> > Now for the fun. Pass iwe->len into this
> > macro before the memcpy, and all should be well.
> >
> > #define FORCE_TO_MEM(x) asm volatile(""::"r"(&(x)))
> >
> > Like this:
> >
> > iwe->len = event_len;
> > FORCE_TO_MEM(iwe->len);
> > memcpy(stream, (char *) iwe, event_len);
>
> I'll try that, that sounds absolutely clever (but I only
> understand half of it).
> Thanks a lot !
>
> Jean
> -
This does absoultely nothing with egcs-2.91.66. I also modified
#define FORCE_TO_MEM(x) asm volatile(""::"r"(&(x)))
|________ this to "memory"
and it still does nothing. The result of gcc -O2 -S -o xxx xxx.c just
shows:
#AP
#NOAP
With no code in-between.
I also changed it to:
#define FORCE_TO_MEM(x) __asm__ __volatile__(""::"r"(&(x)))
to no avail.
What's up?
Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
Why is the government concerned about the lunatic fringe? Think about it.
On Wed, Feb 26, 2003 at 01:23:19PM -0500, Richard B. Johnson wrote:
> On Wed, 26 Feb 2003, Jean Tourrilhes wrote:
>
> > On Tue, Feb 25, 2003 at 11:33:09PM -0500, Albert Cahalan wrote:
> > > Jean Tourrilhes writes:
> > >
> > > > It looks like a compiler bug to me...
> > > > Some users have complained that when the following
> > > > code is compiled without the -fno-strict-aliasing,
> > > > the order of the write and memcpy is inverted (which
> > > > mean a bogus len is mem-copied into the stream).
> > > > Code (from linux/include/net/iw_handler.h) :
> > > > --------------------------------------------
> > > > static inline char *
> > > > iwe_stream_add_event(char * stream, /* Stream of events */
> > > > char * ends, /* End of stream */
> > > > struct iw_event *iwe, /* Payload */
> > > > int event_len) /* Real size of payload */
> > > > {
> > > > /* Check if it's possible */
> > > > if((stream + event_len) < ends) {
> > > > iwe->len = event_len;
> > > > memcpy(stream, (char *) iwe, event_len);
> > > > stream += event_len;
> > > > } return stream;
> > > > }
> > > > --------------------------------------------
> > > > IMHO, the compiler should have enough context to
> > > > know that the reordering is dangerous. Any suggestion
> > > > to make this simple code more bullet proof is welcomed.
> > > >
> > > > Have fun...
> > >
> > > Since (char*) is special, I agree that it's a bug.
> > > In any case, a warning sure would be nice!
> > >
> > > Now for the fun. Pass iwe->len into this
> > > macro before the memcpy, and all should be well.
> > >
> > > #define FORCE_TO_MEM(x) asm volatile(""::"r"(&(x)))
> > >
> > > Like this:
> > >
> > > iwe->len = event_len;
> > > FORCE_TO_MEM(iwe->len);
> > > memcpy(stream, (char *) iwe, event_len);
> >
> > I'll try that, that sounds absolutely clever (but I only
> > understand half of it).
> > Thanks a lot !
> >
> > Jean
> > -
>
> This does absoultely nothing with egcs-2.91.66. I also modified
>
> #define FORCE_TO_MEM(x) asm volatile(""::"r"(&(x)))
> |________ this to "memory"
>
> and it still does nothing. The result of gcc -O2 -S -o xxx xxx.c just
> shows:
>
> #AP
> #NOAP
>
> With no code in-between.
>
> I also changed it to:
> #define FORCE_TO_MEM(x) __asm__ __volatile__(""::"r"(&(x)))
> to no avail.
>
> What's up?
That's "working". Does it prevent the reordering?
--
Daniel Jacobowitz
MontaVista Software Debian GNU/Linux Developer
On Wed, Feb 26, 2003 at 02:40:27PM -0500, Richard B. Johnson wrote:
> On Wed, 26 Feb 2003, Daniel Jacobowitz wrote:
>
> > On Wed, Feb 26, 2003 at 01:23:19PM -0500, Richard B. Johnson wrote:
> > > On Wed, 26 Feb 2003, Jean Tourrilhes wrote:
> > >
> > > > On Tue, Feb 25, 2003 at 11:33:09PM -0500, Albert Cahalan wrote:
> > > > > Jean Tourrilhes writes:
> > > > >
> > > > > > It looks like a compiler bug to me...
> > > > > > Some users have complained that when the following
> > > > > > code is compiled without the -fno-strict-aliasing,
> > > > > > the order of the write and memcpy is inverted (which
> > > > > > mean a bogus len is mem-copied into the stream).
> > > > > > Code (from linux/include/net/iw_handler.h) :
> > > > > > --------------------------------------------
> > > > > > static inline char *
> > > > > > iwe_stream_add_event(char * stream, /* Stream of events */
> > > > > > char * ends, /* End of stream */
> > > > > > struct iw_event *iwe, /* Payload */
> > > > > > int event_len) /* Real size of payload */
> > > > > > {
> > > > > > /* Check if it's possible */
> > > > > > if((stream + event_len) < ends) {
> > > > > > iwe->len = event_len;
> > > > > > memcpy(stream, (char *) iwe, event_len);
> > > > > > stream += event_len;
> > > > > > } return stream;
> > > > > > }
> > > > > > --------------------------------------------
> > > > > > IMHO, the compiler should have enough context to
> > > > > > know that the reordering is dangerous. Any suggestion
> > > > > > to make this simple code more bullet proof is welcomed.
> > > > > >
> > > > > > Have fun...
> > > > >
> > > > > Since (char*) is special, I agree that it's a bug.
> > > > > In any case, a warning sure would be nice!
> > > > >
> > > > > Now for the fun. Pass iwe->len into this
> > > > > macro before the memcpy, and all should be well.
> > > > >
> > > > > #define FORCE_TO_MEM(x) asm volatile(""::"r"(&(x)))
> > > > >
> > > > > Like this:
> > > > >
> > > > > iwe->len = event_len;
> > > > > FORCE_TO_MEM(iwe->len);
> > > > > memcpy(stream, (char *) iwe, event_len);
> > > >
> > > > I'll try that, that sounds absolutely clever (but I only
> > > > understand half of it).
> > > > Thanks a lot !
> > > >
> > > > Jean
> > > > -
> > >
> > > This does absoultely nothing with egcs-2.91.66. I also modified
> > >
> > > #define FORCE_TO_MEM(x) asm volatile(""::"r"(&(x)))
> > > |________ this to "memory"
> > >
> > > and it still does nothing. The result of gcc -O2 -S -o xxx xxx.c just
> > > shows:
> > >
> > > #AP
> > > #NOAP
> > >
> > > With no code in-between.
> > >
> > > I also changed it to:
> > > #define FORCE_TO_MEM(x) __asm__ __volatile__(""::"r"(&(x)))
> > > to no avail.
> > >
> > > What's up?
> >
> > That's "working". Does it prevent the reordering?
>
> It was supposed to force x, which may be cached in a register,
> to be written to memory __now__. It doesn't seem to do anything.
> I think FORCE_TO_MEM() needs to claim that it uses most all the
> registers. That will make sure that any register values get
> written to their final memory locations.
If so it wouldn't be inside the #APP/#NOAPP markers. You didn't answer
my other question: was X in memory at the time?
You should be using something like __asm__ __volatile__ (""::"m"(x))
anyway.
--
Daniel Jacobowitz
MontaVista Software Debian GNU/Linux Developer
On Wed, 26 Feb 2003, Daniel Jacobowitz wrote:
> On Wed, Feb 26, 2003 at 01:23:19PM -0500, Richard B. Johnson wrote:
> > On Wed, 26 Feb 2003, Jean Tourrilhes wrote:
> >
> > > On Tue, Feb 25, 2003 at 11:33:09PM -0500, Albert Cahalan wrote:
> > > > Jean Tourrilhes writes:
> > > >
> > > > > It looks like a compiler bug to me...
> > > > > Some users have complained that when the following
> > > > > code is compiled without the -fno-strict-aliasing,
> > > > > the order of the write and memcpy is inverted (which
> > > > > mean a bogus len is mem-copied into the stream).
> > > > > Code (from linux/include/net/iw_handler.h) :
> > > > > --------------------------------------------
> > > > > static inline char *
> > > > > iwe_stream_add_event(char * stream, /* Stream of events */
> > > > > char * ends, /* End of stream */
> > > > > struct iw_event *iwe, /* Payload */
> > > > > int event_len) /* Real size of payload */
> > > > > {
> > > > > /* Check if it's possible */
> > > > > if((stream + event_len) < ends) {
> > > > > iwe->len = event_len;
> > > > > memcpy(stream, (char *) iwe, event_len);
> > > > > stream += event_len;
> > > > > } return stream;
> > > > > }
> > > > > --------------------------------------------
> > > > > IMHO, the compiler should have enough context to
> > > > > know that the reordering is dangerous. Any suggestion
> > > > > to make this simple code more bullet proof is welcomed.
> > > > >
> > > > > Have fun...
> > > >
> > > > Since (char*) is special, I agree that it's a bug.
> > > > In any case, a warning sure would be nice!
> > > >
> > > > Now for the fun. Pass iwe->len into this
> > > > macro before the memcpy, and all should be well.
> > > >
> > > > #define FORCE_TO_MEM(x) asm volatile(""::"r"(&(x)))
> > > >
> > > > Like this:
> > > >
> > > > iwe->len = event_len;
> > > > FORCE_TO_MEM(iwe->len);
> > > > memcpy(stream, (char *) iwe, event_len);
> > >
> > > I'll try that, that sounds absolutely clever (but I only
> > > understand half of it).
> > > Thanks a lot !
> > >
> > > Jean
> > > -
> >
> > This does absoultely nothing with egcs-2.91.66. I also modified
> >
> > #define FORCE_TO_MEM(x) asm volatile(""::"r"(&(x)))
> > |________ this to "memory"
> >
> > and it still does nothing. The result of gcc -O2 -S -o xxx xxx.c just
> > shows:
> >
> > #AP
> > #NOAP
> >
> > With no code in-between.
> >
> > I also changed it to:
> > #define FORCE_TO_MEM(x) __asm__ __volatile__(""::"r"(&(x)))
> > to no avail.
> >
> > What's up?
>
> That's "working". Does it prevent the reordering?
It was supposed to force x, which may be cached in a register,
to be written to memory __now__. It doesn't seem to do anything.
I think FORCE_TO_MEM() needs to claim that it uses most all the
registers. That will make sure that any register values get
written to their final memory locations.
Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
Why is the government concerned about the lunatic fringe? Think about it.
On Wed, 26 Feb 2003, Daniel Jacobowitz wrote:
[SNIPPED...]
> > It was supposed to force x, which may be cached in a register,
> > to be written to memory __now__. It doesn't seem to do anything.
> > I think FORCE_TO_MEM() needs to claim that it uses most all the
> > registers. That will make sure that any register values get
> > written to their final memory locations.
>
> If so it wouldn't be inside the #APP/#NOAPP markers. You didn't answer
> my other question: was X in memory at the time?
It was in %ebx register and didn't go back to NNN(%esp) where
it came from. Like I said, it did do anything.
>
> You should be using something like __asm__ __volatile__ (""::"m"(x))
> anyway.
>
Yep. Probably.
Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
Why is the government concerned about the lunatic fringe? Think about it.
Falk Hueffner <[email protected]> said:
> Horst von Brand <[email protected]> writes:
> > Jean Tourrilhes <[email protected]> said:
> > > if((stream + event_len) < ends) {
> > > iwe->len = event_len;
> > > memcpy(stream, (char *) iwe, event_len);
> > > stream += event_len;
> > > }
> > > return stream;
> > > }
> >
> > The compiler is free to assume char *stream and struct iw_event *iwe
> > point to separate areas of memory, due to strict aliasing.
>
> The relevant paragraph of the C99 standard is:
>
> An object shall have its stored value accessed only by an lvalue
> expression that has one of the following types:
[...]
> -- a character type.
(char *) gives you a (pointer to) a character type.
> I can't really spot any lvalue here that might violate this rule. It
> would be nice if somebody could report a bug with a testcase.
stream and (char *) iwe
--
Dr. Horst H. von Brand User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria +56 32 654239
Casilla 110-V, Valparaiso, Chile Fax: +56 32 797513
On Wed, Feb 26, 2003 at 05:47:48PM -0300, Horst von Brand wrote:
> Falk Hueffner <[email protected]> said:
> > Horst von Brand <[email protected]> writes:
> > > Jean Tourrilhes <[email protected]> said:
> > > > if((stream + event_len) < ends) {
> > > > iwe->len = event_len;
> > > > memcpy(stream, (char *) iwe, event_len);
> > > > stream += event_len;
> > > > }
> > > > return stream;
> > > > }
> > >
> > > The compiler is free to assume char *stream and struct iw_event *iwe
> > > point to separate areas of memory, due to strict aliasing.
> >
> > The relevant paragraph of the C99 standard is:
> >
> > An object shall have its stored value accessed only by an lvalue
> > expression that has one of the following types:
> [...]
> > -- a character type.
>
> (char *) gives you a (pointer to) a character type.
>
> > I can't really spot any lvalue here that might violate this rule. It
> > would be nice if somebody could report a bug with a testcase.
>
> stream and (char *) iwe
Stream is not the same storage as iwe, so this is hardly the issue.
Writes to stream don't affect iwe. The problem was the assignment to
iwe->len being moved after the access, according to the report.
--
Daniel Jacobowitz
MontaVista Software Debian GNU/Linux Developer
Jean Tourrilhes <[email protected]> said:
> On Wed, Feb 26, 2003 at 04:38:10PM +0100, Horst von Brand wrote:
> > Jean Tourrilhes <[email protected]> said:
> > > It looks like a compiler bug to me...
> > > Some users have complained that when the following code is
> > > compiled without the -fno-strict-aliasing, the order of the write and
> > > memcpy is inverted (which mean a bogus len is mem-copied into the
> > > stream).
> > > Code (from linux/include/net/iw_handler.h) :
> > > --------------------------------------------
> > > static inline char *
> > > iwe_stream_add_event(char * stream, /* Stream of events */
> > > char * ends, /* End of stream */
> > > struct iw_event *iwe, /* Payload */
> > > int event_len) /* Real size of payload */
> > > {
> > > /* Check if it's possible */
> > > if((stream + event_len) < ends) {
> > > iwe->len = event_len;
> > > memcpy(stream, (char *) iwe, event_len);
> > > stream += event_len;
> > > }
> > > return stream;
> > > }
> > > --------------------------------------------
> > > IMHO, the compiler should have enough context to know that the
> > > reordering is dangerous. Any suggestion to make this simple code more
> > > bullet proof is welcomed.
> >
> > The compiler is free to assume char *stream and struct iw_event *iwe point
> > to separate areas of memory, due to strict aliasing.
>
> Which is true and which is not the problem I'm complaining about.
... the compiler thus goes and reorders the frobbing of the variables, as
they are (assumed) separate.
--
Dr. Horst H. von Brand User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria +56 32 654239
Casilla 110-V, Valparaiso, Chile Fax: +56 32 797513
On Wed, 2003-02-26 at 15:19, Richard B. Johnson wrote:
> On Wed, 26 Feb 2003, Daniel Jacobowitz wrote:
>>> It was supposed to force x, which may be cached in a register,
>>> to be written to memory __now__. It doesn't seem to do anything.
>>> I think FORCE_TO_MEM() needs to claim that it uses most all the
>>> registers. That will make sure that any register values get
>>> written to their final memory locations.
>>
>> If so it wouldn't be inside the #APP/#NOAPP markers. You didn't
>> answer my other question: was X in memory at the time?
>
> It was in %ebx register and didn't go back to NNN(%esp) where
> it came from. Like I said, it did do anything.
>
>> You should be using something like __asm__ __volatile__ (""::"m"(x))
>> anyway.
>
> Yep. Probably.
I wrote that from my memory and gcc documentation. With none
of my 3 compilers did I see the sample code failing, so I
wasn't able to perform a useful test. There may be some little
detail I missed.
I've had the code working, with gcc 3.0 I'd guess, on both
PowerPC and something strange. The code relies on "r" being
a suitable register; this might only be true for common RISC
architectures. So this is semi-portable code, able to be used
where the gcc porter followed the suggestion to use "r" as
the normal sort of register. Since x86 is notably lacking in
the concept of general purpose registers, the above might not
apply.
Add :"memory" (a clobber) on the end if you want to be brutal.
This may produce slower code though.
The basic idea is to convince gcc that some assembly code
will access the data via a pointer. Probably somebody else
has already posted a fixed version.
With a recent gcc, look into the __may_alias__ attribute.
This kind of stuff sure is poorly documented, isn't it?
Has anybody ever seen a reasonable explanation of the new
C99 restrict keyword, including scope interactions? How
about a list of gcc bugs in this area? Even with a recent
gcc, control over aliasing is really crude. I ought to be
able to specify a list of types and objects that do or do
not alias, with each other or with the first one specified.
On Wednesday 26 February 2003 22:07, Horst von Brand wrote:
> Jean Tourrilhes <[email protected]> said:
> > On Wed, Feb 26, 2003 at 04:38:10PM +0100, Horst von Brand wrote:
> > > Jean Tourrilhes <[email protected]> said:
> > > > It looks like a compiler bug to me...
> > > > Some users have complained that when the following code is
> > > > compiled without the -fno-strict-aliasing, the order of the write and
> > > > memcpy is inverted (which mean a bogus len is mem-copied into the
> > > > stream).
> > > > Code (from linux/include/net/iw_handler.h) :
> > > > --------------------------------------------
> > > > static inline char *
> > > > iwe_stream_add_event(char * stream, /* Stream of events */
> > > > char * ends, /* End of stream */
> > > > struct iw_event *iwe, /* Payload */
> > > > int event_len) /* Real size of payload */
> > > > {
> > > > /* Check if it's possible */
> > > > if((stream + event_len) < ends) {
> > > > iwe->len = event_len;
> > > > memcpy(stream, (char *) iwe, event_len);
> > > > stream += event_len;
> > > > }
> > > > return stream;
> > > > }
> > > > --------------------------------------------
> > > > IMHO, the compiler should have enough context to know that the
> > > > reordering is dangerous. Any suggestion to make this simple code more
> > > > bullet proof is welcomed.
> > >
> > > The compiler is free to assume char *stream and struct iw_event *iwe
> > > point to separate areas of memory, due to strict aliasing.
> >
> > Which is true and which is not the problem I'm complaining about.
>
> ... the compiler thus goes and reorders the frobbing of the variables, as
> they are (assumed) separate.
Actually, the compiler appears to view &iwe->len and (char *) iwe as
separate, which I find surprising.
Regards,
Daniel
On Wed, Feb 26, 2003 at 03:57:54PM -0500, Daniel Jacobowitz wrote:
> > > > > if((stream + event_len) < ends) {
> > > > > iwe->len = event_len;
> > > > > memcpy(stream, (char *) iwe, event_len);
> > > > > stream += event_len;
> > > > > }
> > > > > return stream;
> > > > The compiler is free to assume char *stream and struct iw_event *iwe
> > > > point to separate areas of memory, due to strict aliasing.
No.
> Stream is not the same storage as iwe, so this is hardly the issue.
> Writes to stream don't affect iwe. The problem was the assignment to
> iwe->len being moved after the access, according to the report.
GCC can do that with -fstrict-aliasing:
All calls to this inline function are done with constant event_len.
E.g. in case of event_len = IW_EV_UINT_LEN, memcpy macro expands to
__constant_memcpy which does:
*(unsigned long *)to = *(const unsigned long *)from;
*(1+(unsigned long *)to) = *(1+(const unsigned long *)from);
iwe->len is unsigned short and the code writes the value as
unsigned short (iwe->len = 8) and then reads the same memory as unsigned
long (something = *(const unsigned long *)(char *)iwe). But those two types
cannot alias and thus gcc can reorder them.
To fix that, __constant_memcpy would have to access the data through
union, ir you could as well forget about __constant_memcpy and use
__builtin_memcpy where gcc will take care about the constant copying.
Jakub
In article <[email protected]>,
Jakub Jelinek <[email protected]> wrote:
>
>To fix that, __constant_memcpy would have to access the data through
>union,
Which is impossible, since memcpy _fundamentally_ cannot know what the
different types are..
> or you could as well forget about __constant_memcpy and use
>__builtin_memcpy where gcc will take care about the constant copying.
Which is impossible because (a) historically __builtin_memcpy does a bad
job and (b) it doesn't solve the generic case anyway, ie for other
non-memcpy things.
The fact is, for type-based alias analysis gcc needs a way to tell it
"this can alias", which it doesn't have. Unions are _not_ useful,
_regardless_ of what silly language lawyers say, since they are not a
generic method. Unions only work for trivial and largely uninteresting
cases, and it doesn't _matter_ what C99 says about the issue, since that
nasty thing called "real life" interferes.
Until we get some non-union way to say "this can alias", that
-fno-strict-alias has to stay because gcc is too broken to allow us
doing interesting stuff in-line without it.
My personal opinion is (and was several years ago when this started
coming up) that a cast (any cast) should do it. But I don't are _what_
it is, as long as it is syntactically sane and isn't limited to special
cases like unions.
Linus
On Thu, Feb 27, 2003 at 07:30:03PM +0000, Linus Torvalds wrote:
> In article <[email protected]>,
> Jakub Jelinek <[email protected]> wrote:
> >
> >To fix that, __constant_memcpy would have to access the data through
> >union,
>
> Which is impossible, since memcpy _fundamentally_ cannot know what the
> different types are..
>
> > or you could as well forget about __constant_memcpy and use
> >__builtin_memcpy where gcc will take care about the constant copying.
>
> Which is impossible because (a) historically __builtin_memcpy does a bad
> job and (b) it doesn't solve the generic case anyway, ie for other
> non-memcpy things.
>
> The fact is, for type-based alias analysis gcc needs a way to tell it
> "this can alias", which it doesn't have. Unions are _not_ useful,
> _regardless_ of what silly language lawyers say, since they are not a
> generic method. Unions only work for trivial and largely uninteresting
> cases, and it doesn't _matter_ what C99 says about the issue, since that
> nasty thing called "real life" interferes.
>
> Until we get some non-union way to say "this can alias", that
> -fno-strict-alias has to stay because gcc is too broken to allow us
> doing interesting stuff in-line without it.
>
> My personal opinion is (and was several years ago when this started
> coming up) that a cast (any cast) should do it. But I don't are _what_
> it is, as long as it is syntactically sane and isn't limited to special
> cases like unions.
Well, if that's all you're asking for, it's easy - I don't know if
you'll agree that the syntax is sane, but it's there. From the GCC 3.3
manual:
`may_alias'
Accesses to objects with types with this attribute are not
subjected to type-based alias analysis, but are instead assumed to
be able to alias any other type of objects, just like the `char'
type. See `-fstrict-aliasing' for more information on aliasing
issues.
Example of use:
typedef short __attribute__((__may_alias__)) short_a;
int
main (void)
{
int a = 0x12345678;
short_a *b = (short_a *) &a;
b[1] = 0;
if (a == 0x12345678)
abort();
exit(0);
}
If you replaced `short_a' with `short' in the variable
declaration, the above program would abort when compiled with
`-fstrict-aliasing', which is on by default at `-O2' or above in
recent GCC versions.
So you define a typedef for unsigned long which has the __may_alias__
attribute, and you go to town writing memcpy inline with that type
instead of a normal unsigned long.
--
Daniel Jacobowitz
MontaVista Software Debian GNU/Linux Developer
On Thu, 27 Feb 2003, Daniel Jacobowitz wrote:
> >
> > My personal opinion is (and was several years ago when this started
> > coming up) that a cast (any cast) should do it. But I don't are _what_
> > it is, as long as it is syntactically sane and isn't limited to special
> > cases like unions.
>
> Well, if that's all you're asking for, it's easy - I don't know if
> you'll agree that the syntax is sane, but it's there. From the GCC 3.3
> manual:
>
> `may_alias'
Halleluja. It still leaves us with all other compilers unaccounted for,
but yes, this will allow fixing the problem for us in the future.
Too bad the current gcc-3.3 is apparently not useful for the kernel for
other reasons right now (ie the inlining issue which is being discussed on
the gcc lists, and that annoying sign warning), but that may well be
resolved by the time it gets released.
Linus
On Thu, Feb 27, 2003 at 12:00:46PM -0800, Linus Torvalds wrote:
>
> On Thu, 27 Feb 2003, Daniel Jacobowitz wrote:
> > >
> > > My personal opinion is (and was several years ago when this started
> > > coming up) that a cast (any cast) should do it. But I don't are _what_
> > > it is, as long as it is syntactically sane and isn't limited to special
> > > cases like unions.
> >
> > Well, if that's all you're asking for, it's easy - I don't know if
> > you'll agree that the syntax is sane, but it's there. From the GCC 3.3
> > manual:
> >
> > `may_alias'
>
> Halleluja. It still leaves us with all other compilers unaccounted for,
> but yes, this will allow fixing the problem for us in the future.
>
> Too bad the current gcc-3.3 is apparently not useful for the kernel for
> other reasons right now (ie the inlining issue which is being discussed on
> the gcc lists, and that annoying sign warning), but that may well be
> resolved by the time it gets released.
We could work around both of these: disable the sign compare warning,
and use check_gcc to set a high number for -finline-limit...
--
Daniel Jacobowitz
MontaVista Software Debian GNU/Linux Developer
On Thu, 27 Feb 2003, Daniel Jacobowitz wrote:
>
> We could work around both of these: disable the sign compare warning,
> and use check_gcc to set a high number for -finline-limit...
Oh, both are work-aroundable, no question about it. The same way it was
possible to work around the broken aliasing with previous releases. I'm
just hoping that especially the inline thing can be resolved sanely,
otherwise we'll end up having to use something ugly like
-D'inline=inline __attribute__((force_inline))'
on every single command line..
(I find -finline-limit tasteless, since the limit number is apparently
totally meaningless as far as the user is concerned. It's clearly a
command line option that is totally designed for ad-hoc compiler tweaking,
not for any actual useful user stuff).
Linus
Followup to: <[email protected]>
By author: Linus Torvalds <[email protected]>
In newsgroup: linux.dev.kernel
>
> On Thu, 27 Feb 2003, Daniel Jacobowitz wrote:
> >
> > We could work around both of these: disable the sign compare warning,
> > and use check_gcc to set a high number for -finline-limit...
>
> Oh, both are work-aroundable, no question about it. The same way it was
> possible to work around the broken aliasing with previous releases. I'm
> just hoping that especially the inline thing can be resolved sanely,
> otherwise we'll end up having to use something ugly like
>
> -D'inline=inline __attribute__((force_inline))'
>
> on every single command line..
>
Isn't this what compiler.h is for? If the complaint is that some
things don't include compiler.h then we may want to force it with
-include. Better all the cruft in one file IMO.
-hpa
--
<[email protected]> at work, <[email protected]> in private!
"Unix gives you enough rope to shoot yourself in the foot."
Architectures needed: cris ia64 m68k mips64 ppc ppc64 s390 s390x sh v850 x86-64
> Oh, both are work-aroundable, no question about it. The same way it was
> possible to work around the broken aliasing with previous releases. I'm
> just hoping that especially the inline thing can be resolved sanely,
> otherwise we'll end up having to use something ugly like
>
> -D'inline=inline __attribute__((force_inline))'
>
> on every single command line..
>
> (I find -finline-limit tasteless, since the limit number is apparently
> totally meaningless as far as the user is concerned. It's clearly a
> command line option that is totally designed for ad-hoc compiler tweaking,
> not for any actual useful user stuff).
Yep, the instruction count used for inlining seems to be calculated too
early to be useful. Things like sigorsets look huge until all the
redundant cases get optimised away.
Also in gcc 3.2 -Winline was broken, I really hope that gets fixed. We
need it to tune -finline-limit and to catch all those stupidly large
inline functions.
Anton