Re: [PATCH 09/10] x86 tune inc files: add x32 abi tune parameters

From: "Kamble, Nitin A" <nitin.a.kamble@intel.com>
To: Patches and discussions about the oe-core layer
	<openembedded-core@lists.openembedded.org>
Subject: Re: [PATCH 09/10] x86 tune inc files: add x32 abi tune parameters
Date: Thu, 4 Aug 2011 15:18:38 -0700	[thread overview]
Message-ID: <9DA5872FEF993D41B7173F58FCF6BE94DE85F4F7@orsmsx504.amr.corp.intel.com> (raw)
In-Reply-To: <1312495032.3169.6.camel@lenovo.internal.reciva.com>

> -----Original Message-----
> From: openembedded-core-bounces@lists.openembedded.org
> [mailto:openembedded-core-bounces@lists.openembedded.org] On Behalf Of
> Phil Blundell
> Sent: Thursday, August 04, 2011 2:57 PM
> To: Patches and discussions about the oe-core layer
> Subject: Re: [OE-core] [PATCH 09/10] x86 tune inc files: add x32 abi
> tune parameters
> 
> On Thu, 2011-08-04 at 08:01 -0700, nitin.a.kamble@intel.com wrote:
> >  # ELF32 ABI
> >  TUNEVALID[m32] = "IA32 ELF32 standard ABI"
> > -TUNECONFLICTS[m32] = "m64"
> > +TUNECONFLICTS[m32] = "m64 mx32"
> >  TUNE_ARCH .= "${@bb.utils.contains("TUNE_FEATURES", "m32",
> "${X86ARCH32}", "" ,d)}"
> > +ABIEXTENSION .= "${@bb.utils.contains("TUNE_FEATURES", "m32", "32",
> "" ,d)}"
> >  TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "m32", "-m32",
> "", d)}"
> 
> This is going to cause TARGET_OS to change for everyone using the i586
> ABI, right?  That doesn't seem like it is either necessary or
> desirable,
> and it isn't mentioned in the checkin comment either.

Correct, this will change the TARGET_OS from linux_gnu to linux_gnu32. And it is also applicable for x86-64 machine set with x86 tune. This change is be needed if multiple tunes are built from the same build directory. If such situation is not important then the ABIEXTENSION part can be dropped.

> 
> >  # ELF64 ABI 
> >  TUNEVALID[m64] = "IA32e (x86_64) ELF64 standard ABI"
> > -TUNECONFLICT[m64] = "m32"
> > +TUNECONFLICT[m64] = "m32 mx32"
> >  TUNE_ARCH .= "${@bb.utils.contains("TUNE_FEATURES", "m64",
> "${X86ARCH64}", "" ,d)}"
> > +ABIEXTENSION .= "${@bb.utils.contains("TUNE_FEATURES", "m64", "64",
> "" ,d)}"
> >  TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "m64", "-m64",
> "", d)}"
> 
> ... and likewise this for anybody using the x86-64 ABI.
This is similar to above.

> 
> > -PACKAGE_EXTRA_ARCHS_tune-x86-64 = "x86_64"
> > +PACKAGE_EXTRA_ARCHS_tune-x86-64 = "x86-64"
> 
> That change might well be fine, but again it isn't mentioned in the
> checkin message.
This is making this consistent with other PACKAGE_EXTRA_ARCHS_tune convention elsewhere. I will update the commit message for this.

> 
> > diff --git a/meta/conf/machine/include/tune-x86_64.inc
> b/meta/conf/machine/include/tune-x86_64.inc
> > index 04b0f96..50f20ba 100644
> > --- a/meta/conf/machine/include/tune-x86_64.inc
> > +++ b/meta/conf/machine/include/tune-x86_64.inc
> > @@ -1,3 +1,3 @@
> >  require conf/machine/include/ia32/arch-ia32.inc
> >
> > -DEFAULTTUNE = "x86-64"
> > +DEFAULTTUNE ?= "x86-64"
> 
> This one is also not mentioned in the checkin message and looks a bit
> more dubious to me.  Why is this required?
> 
It was required at one point when multilist support was quiet unstable. The reason it is done because the arch-ia32.inc file is included for both x86, tune-core, & x86-64 and setting this hard assignment, was breaking x86 or x32 targets. Not sure if it is still needed, I will check it.

> p.
> 
> 
> 
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core