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

[Vladimir 'φ-coder/phcoder' Serbinenko]

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                                       address@hidden
>>
>> _______________________________________________
>> Grub-devel mailing list
>> address@hidden
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
> 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

signature.asc
Description: OpenPGP digital signature