From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1XQoVY-0005O7-M2 for mharc-grub-devel@gnu.org; Sun, 07 Sep 2014 22:16:44 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47149) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XQoVV-0005N5-KZ for grub-devel@gnu.org; Sun, 07 Sep 2014 22:16:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XQoVT-0000f4-Bh for grub-devel@gnu.org; Sun, 07 Sep 2014 22:16:41 -0400 Received: from b.painless.aa.net.uk ([2001:8b0:0:30::51bb:1e34]:34080 helo=b.b.painless.aa.net.uk) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XQoVT-0000ex-04 for grub-devel@gnu.org; Sun, 07 Sep 2014 22:16:39 -0400 Received: from 3.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.4.1.b.e.2.f.f.b.0.b.8.0.1.0.0.2.ip6.arpa ([2001:8b0:bff2:eb14::3] helo=riva.pelham.vpn.ucam.org) by b.painless.aa.net.uk with esmtps (TLSv1:AES128-SHA:128) (Exim 4.72) (envelope-from ) id 1XQoVD-0005mr-OM for grub-devel@gnu.org; Mon, 08 Sep 2014 03:16:36 +0100 Received: from ns1.pelham.vpn.ucam.org ([172.20.153.2] helo=riva.ucam.org) by riva.pelham.vpn.ucam.org with esmtps (TLS1.2:DHE_RSA_AES_128_CBC_SHA1:128) (Exim 4.80) (envelope-from ) id 1XQoVC-0002UB-Eb for grub-devel@gnu.org; Mon, 08 Sep 2014 03:16:22 +0100 Date: Mon, 8 Sep 2014 03:16:21 +0100 From: Colin Watson To: grub-devel@gnu.org Subject: Re: [RFC PATCH 2/3] Files reorganization and include some libgcc fuctions Message-ID: <20140908021620.GA9140@riva.ucam.org> References: <1409255765-3209-1-git-send-email-pfsmorigo@linux.vnet.ibm.com> <1409255765-3209-3-git-send-email-pfsmorigo@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1409255765-3209-3-git-send-email-pfsmorigo@linux.vnet.ibm.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2001:8b0:0:30::51bb:1e34 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 08 Sep 2014 02:16:43 -0000 On Thu, Aug 28, 2014 at 04:56:04PM -0300, Paulo Flabiano Smorigo wrote: > diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c > index c5c815d..353a207 100644 > --- a/grub-core/kern/misc.c > +++ b/grub-core/kern/misc.c > @@ -1342,3 +1342,110 @@ grub_real_boot_time (const char *file, > grub_error_pop (); > } > #endif > + > +#if defined (NO_LIBGCC) Should this perhaps be restricted to __powerpc__ as well? Alternatively, the prototypes in include/grub/compiler.h (or include/grub/misc.h; see below) should be used on other architectures too. Either way, the declarations and definitions should match. > diff --git a/include/grub/compiler.h b/include/grub/compiler.h > index c9e1d7a..a9a684c 100644 > --- a/include/grub/compiler.h > +++ b/include/grub/compiler.h > @@ -48,4 +48,65 @@ > # define WARN_UNUSED_RESULT > #endif > > +#include "types.h" Shouldn't this be #include , assuming that you need this for grub_uint*_t? Also, includes should generally be grouped at the top of the file. > +union component64 > +{ > + grub_uint64_t full; > + struct > + { > +#ifdef GRUB_CPU_WORDS_BIGENDIAN > + grub_uint32_t high; > + grub_uint32_t low; > +#else > + grub_uint32_t low; > + grub_uint32_t high; > +#endif > + }; > +}; This is only used by grub-core/kern/misc.c. Please move it there rather than putting it somewhere that's included by everything in GRUB. > +#if defined (__powerpc__) Should this be #if defined (__powerpc__) && defined (NO_LIBGCC) or something similar, to match the general way things are set up in configure.ac? (Also see comment above about declarations matching definitions.) Relatedly, have you tested this patch set with a native build on a 32-bit BE powerpc system, as opposed to 32-bit BE built on a 64-bit LE system? This looks like a potential problem there. > +grub_uint64_t EXPORT_FUNC (__lshrdi3) (grub_uint64_t u, int b); > +grub_uint64_t EXPORT_FUNC (__ashrdi3) (grub_uint64_t u, int b); > +grub_uint64_t EXPORT_FUNC (__ashldi3) (grub_uint64_t u, int b); > +int EXPORT_FUNC(__ucmpdi2) (grub_uint64_t a, grub_uint64_t b); > +void EXPORT_FUNC (_restgpr_14_x) (void); [...] These aren't compiler features, so don't belong in include/grub/compiler.h. Other architectures seem to have this kind of thing in include/grub/misc.h inside a big #ifndef GRUB_UTIL conditional, so please move all this to there. > diff --git a/include/grub/libgcc.h b/include/grub/libgcc.h > index 8e93b67..5bdb8fb 100644 > --- a/include/grub/libgcc.h > +++ b/include/grub/libgcc.h > @@ -16,73 +16,6 @@ > * along with GRUB. If not, see . > */ > > -/* We need to include config-util.h.in for HAVE_*. */ > -#ifndef __STDC_VERSION__ > -#define __STDC_VERSION__ 0 > -#endif > -#include > - > -/* On x86 these functions aren't really needed. Save some space. */ > -#if !defined (__i386__) && !defined (__x86_64__) > -# ifdef HAVE___ASHLDI3 > -void EXPORT_FUNC (__ashldi3) (void); > -# endif > -# ifdef HAVE___ASHRDI3 > -void EXPORT_FUNC (__ashrdi3) (void); > -# endif > -# ifdef HAVE___LSHRDI3 > -void EXPORT_FUNC (__lshrdi3) (void); > -# endif > -# ifdef HAVE___UCMPDI2 > -void EXPORT_FUNC (__ucmpdi2) (void); > -# endif > -# ifdef HAVE___BSWAPSI2 > -void EXPORT_FUNC (__bswapsi2) (void); > -# endif > -# ifdef HAVE___BSWAPDI2 > -void EXPORT_FUNC (__bswapdi2) (void); > -# endif > -#endif > - > -#ifdef HAVE__RESTGPR_14_X > -void EXPORT_FUNC (_restgpr_14_x) (void); > -void EXPORT_FUNC (_restgpr_15_x) (void); > -void EXPORT_FUNC (_restgpr_16_x) (void); > -void EXPORT_FUNC (_restgpr_17_x) (void); > -void EXPORT_FUNC (_restgpr_18_x) (void); > -void EXPORT_FUNC (_restgpr_19_x) (void); > -void EXPORT_FUNC (_restgpr_20_x) (void); > -void EXPORT_FUNC (_restgpr_21_x) (void); > -void EXPORT_FUNC (_restgpr_22_x) (void); > -void EXPORT_FUNC (_restgpr_23_x) (void); > -void EXPORT_FUNC (_restgpr_24_x) (void); > -void EXPORT_FUNC (_restgpr_25_x) (void); > -void EXPORT_FUNC (_restgpr_26_x) (void); > -void EXPORT_FUNC (_restgpr_27_x) (void); > -void EXPORT_FUNC (_restgpr_28_x) (void); > -void EXPORT_FUNC (_restgpr_29_x) (void); > -void EXPORT_FUNC (_restgpr_30_x) (void); > -void EXPORT_FUNC (_restgpr_31_x) (void); > -void EXPORT_FUNC (_savegpr_14) (void); > -void EXPORT_FUNC (_savegpr_15) (void); > -void EXPORT_FUNC (_savegpr_16) (void); > -void EXPORT_FUNC (_savegpr_17) (void); > -void EXPORT_FUNC (_savegpr_18) (void); > -void EXPORT_FUNC (_savegpr_19) (void); > -void EXPORT_FUNC (_savegpr_20) (void); > -void EXPORT_FUNC (_savegpr_21) (void); > -void EXPORT_FUNC (_savegpr_22) (void); > -void EXPORT_FUNC (_savegpr_23) (void); > -void EXPORT_FUNC (_savegpr_24) (void); > -void EXPORT_FUNC (_savegpr_25) (void); > -void EXPORT_FUNC (_savegpr_26) (void); > -void EXPORT_FUNC (_savegpr_27) (void); > -void EXPORT_FUNC (_savegpr_28) (void); > -void EXPORT_FUNC (_savegpr_29) (void); > -void EXPORT_FUNC (_savegpr_30) (void); > -void EXPORT_FUNC (_savegpr_31) (void); > -#endif > - > #if defined (__arm__) > void EXPORT_FUNC (__aeabi_lasr) (void); > void EXPORT_FUNC (__aeabi_llsl) (void); I don't think you should touch this file at all. I don't know precisely how it's used, but it only seems to be used to generate symbol lists; furthermore, you're removing some things from it that are not clearly powerpc-specific and either making them powerpc-specific (__ashldi3, __ashrdi3, __lshrdi3, __ucmpdi2) or dropping them altogether (__bswapsi2, __bswapdi2). This is worrisome and suggests the possibility that this will break other architectures. Does your patch work if you just leave include/grub/libgcc.h unmodified? Thanks, -- Colin Watson [cjwatson@ubuntu.com]