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 [cjwat...@ubuntu.com]
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
