Re: [RFC PATCH 2/3] Files reorganization and include some libgcc fuctions

[Paulo Flabiano Smorigo]

Colin, I changed the patches following your suggestions and making it
more likely to the no-libgcc branch from Vladimir. In this branch,
phcoder added compiler-rt.{c,h,S} with the necessary code in it.

My approach is very minimalist and only for powerpc. I tried to avoid
change the behavior for other architecture since we are in code freeze.

In the future we can think in spread this approach for all archs and use
all implementation that phcoder is doing in the no-libgcc branch.

Mon, Sep 08, 2014 at 03:16:21AM +0100, Colin Watson wrote:
> 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.

Yes, now I only use __powerpc__

> 
> > 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 <grub/types.h>, assuming that you need this
> for grub_uint*_t?  Also, includes should generally be grouped at the top
> of the file.

All in the compile-rt.c now

> 
> > +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.)

Fixed.

> 
> 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.
> 

Yes. I tested it in both BE and LE systems. No problem found. I did
another round of test with this changes and will do some more this week.

> > +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.

Yes, I follow what phcoder did in no-libgcc branch. This lines are in
the compiler-rt.c file.

> 
> > 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 <http://www.gnu.org/licenses/>.
> >   */
> >  
> > -/* We need to include config-util.h.in for HAVE_*.  */
> > -#ifndef __STDC_VERSION__
> > -#define __STDC_VERSION__ 0
> > -#endif
> > -#include <config-util.h>
> > -
> > -/* 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?

Please, check this new patchset. libgcc.h is intact.

> 
> Thanks,
> 
> -- 
> Colin Watson                                       address@hidden
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 

-- 
Paulo Flabiano Smorigo
IBM Linux Technology Center

0002-Skip-libgcc-dependency-for-powerpc.patch
Description: Text document