On 17.09.2014 23:43, Paulo Flabiano Smorigo wrote:
> 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.
>
compiler-rt.{c,S} fixes bunch of possible problems when libgcc doesn't
match the intended platform (e.g. because of SSE flags). So I think we
should have whole thing go in.
Biggest problem is the lack of tests for those functions.
> 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 [cjwat...@ubuntu.com]
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
