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

[Colin Watson]

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 <grub/types.h>, 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 <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?

Thanks,

-- 
Colin Watson                                       address@hidden