Re: [PATCH] Fix Immediate Values x86_64 support old gcc

[Mathieu Desnoyers]

* Sam Ravnborg (sam@xxxxxxxxxxxx) wrote:
> Hi Mathieu
> > Does this fix make more sense ?
> > 
> > GCC < 4, on x86_64, does not accept symbol+offset operands for "i" constraints
> > asm statements. Fallback on generic immediate values if this compiler is
> > detected.
> > 
> > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxx>
> > ---
> >  arch/x86/Makefile           |    3 +++
> >  arch/x86/kernel/Makefile    |    4 +++-
> >  include/asm-x86/immediate.h |    5 +++++
> >  include/linux/immediate.h   |    4 +++-
> >  kernel/Makefile             |    4 +++-
> >  5 files changed, 17 insertions(+), 3 deletions(-)
> > 
> > Index: linux-2.6-sched-devel/arch/x86/Makefile
> > ===================================================================
> > --- linux-2.6-sched-devel.orig/arch/x86/Makefile	2008-05-21 09:04:52.000000000 -0400
> > +++ linux-2.6-sched-devel/arch/x86/Makefile	2008-05-21 09:22:05.000000000 -0400
> > @@ -78,6 +78,9 @@
> >                  "$(CC)" -fstack-protector-all )
> >  
> >          KBUILD_CFLAGS += $(stackp-y)
> > +
> > +	export GCC_BROKEN_IMMEDIATE
> > +	GCC_BROKEN_IMMEDIATE := $(shell if [ $(call cc-version) -lt 0400 ] ; then echo "y"; fi)
> 
> So here we introduce a global environment variable that tells
> us that gcc has "BROKEN_IMMEDIATE".
> I have absolutely no clue what "BROKEN_IMMEDIATE" is so I guess others
> are in the same boat. Comment please!
> 

Yes, I guess this worth being commented, you are right. I'll write
something along the lines written in the patch header.

> Consider something like this (note: no negative logic involved):
> export USE_IMMEDIATE := $(call cc-ifversion, -ge, 0400, $(CONFIG_IMMEDIATE))
> 

Hrm, if I do that, I would have to add USE_IMMEDIATE to each
architecture's makefiles for which immediate values are supported, e.g.:

export USE_IMMEDIATE := $(CONFIG_IMMEDIATE)

The "BROKEN_IMMEDIATE" (negative) approach only needs to be defined on
x86_64 where gcc 3.x does not support symbol+offset correctly.

> Then the MAkefile fragment can be simplified as:
> > -obj-$(CONFIG_IMMEDIATE)              += immediate.o
> > +obj-$(USE_IMMEDIATE)                 += immediate.o
> 
> (Or find a better name than "USE_IMMEDIATE".
> 
> 
> >  endif
> >  
> >  # Stackpointer is addressed different for 32 bit and 64 bit x86
> > Index: linux-2.6-sched-devel/include/asm-x86/immediate.h
> > ===================================================================
> > --- linux-2.6-sched-devel.orig/include/asm-x86/immediate.h	2008-05-21 09:10:59.000000000 -0400
> > +++ linux-2.6-sched-devel/include/asm-x86/immediate.h	2008-05-21 09:15:43.000000000 -0400
> > @@ -12,6 +12,10 @@
> >  
> >  #include <asm/asm.h>
> >  
> > +#if (defined(CONFIG_X86_64) && __GNUC__ < 4)	/* Detect broken x86_64 gcc */
> > +#undef CONFIG_IMMEDIATE
> > +#else
> 
> And now in the source we use a direct check of the GNUC version. So this is a duplicate
> of GCC_BROKEN_IMMEDIATE and not sensible comments.
> "I ask myself in what way is gcc broken?"
> And you DO NOT fiddle with the CONFIG_* variables - this is just plain wrong.
> 

I could pass the USE_IMMEDIATE as a -D to the compiler flags. That would
be cleaner.

> > --- linux-2.6-sched-devel.orig/include/linux/immediate.h	2008-05-21 09:12:22.000000000 -0400
> > +++ linux-2.6-sched-devel/include/linux/immediate.h	2008-05-21 09:12:59.000000000 -0400
> > @@ -11,8 +11,10 @@
> >   */
> >  
> >  #ifdef CONFIG_IMMEDIATE
> > +#include <asm/immediate.h>	/* May undef CONFIG_IMMEDIATE */
> > +#endif
> 
> So you need to find a better way for this.
> 

I think USE_IMMEDIATE is the way to go then.

Thanks,

Mathieu

> 
> >  
> > -#include <asm/immediate.h>
> > +#ifdef CONFIG_IMMEDIATE
> >  
> >  /**
> >   * imv_set - set immediate variable (with locking)
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/