diff mbox

[U-Boot,RFC] x86: Do no use reparm as it break libgcc linkage

Message ID 1320834779-15460-1-git-send-email-graeme.russ@gmail.com
State Rejected
Headers show

Commit Message

Graeme Russ Nov. 9, 2011, 10:32 a.m. UTC
Hi Gabe,

Can you please try this patch - If it solves your libgcc problem, I will
add it to the misc cleanup patch

Thanks,

Graeme
---
 arch/x86/config.mk        |    3 ---
 arch/x86/cpu/interrupts.c |    2 +-
 arch/x86/cpu/start.S      |    5 ++---
 3 files changed, 3 insertions(+), 7 deletions(-)

--
1.7.5.2.317.g391b14

Comments

Mike Frysinger Nov. 9, 2011, 5:12 p.m. UTC | #1
On Wednesday 09 November 2011 05:32:59 Graeme Russ wrote:
> --- a/arch/x86/config.mk
> +++ b/arch/x86/config.mk
> 
> -PLATFORM_CPPFLAGS += -mregparm=3
> -PLATFORM_CPPFLAGS += -fomit-frame-pointer

this sounds like you're throwing the baby out with the bath water.  doesn't 
this severely affect the generated code ?
-mike
Graeme Russ Nov. 9, 2011, 9:42 p.m. UTC | #2
Hi Mike,

On Thu, Nov 10, 2011 at 4:12 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Wednesday 09 November 2011 05:32:59 Graeme Russ wrote:
>> --- a/arch/x86/config.mk
>> +++ b/arch/x86/config.mk
>>
>> -PLATFORM_CPPFLAGS += -mregparm=3
>> -PLATFORM_CPPFLAGS += -fomit-frame-pointer
>
> this sounds like you're throwing the baby out with the bath water.  doesn't
> this severely affect the generated code ?

No - omit-frame-pointer is enabled by -O2 and also:

http://gcc.gnu.org/gcc-4.6/changes.html

"The default setting (when not optimizing for size) for 32-bit
GNU/Linux and Darwin x86 targets has been changed to
-fomit-frame-pointer. The default can be reverted to
-fno-omit-frame-pointer by configuring GCC with the
--enable-frame-pointer configure option."

So the flag is a do-nothing

Regards,

Graeme
Mike Frysinger Nov. 10, 2011, 4:13 a.m. UTC | #3
On Wednesday 09 November 2011 16:42:51 Graeme Russ wrote:
> On Thu, Nov 10, 2011 at 4:12 AM, Mike Frysinger wrote:
> > On Wednesday 09 November 2011 05:32:59 Graeme Russ wrote:
> >> --- a/arch/x86/config.mk
> >> +++ b/arch/x86/config.mk
> >> 
> >> -PLATFORM_CPPFLAGS += -mregparm=3
> >> -PLATFORM_CPPFLAGS += -fomit-frame-pointer
> > 
> > this sounds like you're throwing the baby out with the bath water.
> >  doesn't this severely affect the generated code ?
> 
> No - omit-frame-pointer is enabled by -O2 and also:
> 
> http://gcc.gnu.org/gcc-4.6/changes.html
> 
> "The default setting (when not optimizing for size) for 32-bit
> GNU/Linux and Darwin x86 targets has been changed to
> -fomit-frame-pointer. The default can be reverted to
> -fno-omit-frame-pointer by configuring GCC with the
> --enable-frame-pointer configure option."
> 
> So the flag is a do-nothing

ok, except:
 - we build u-boot with -Os and not -O2
 - i'd say most people aren't using gcc-4.6

i was referring also to throwing away -mregparm=3 ...
-mike
Graeme Russ Nov. 10, 2011, 4:22 a.m. UTC | #4
Hi Mike,

On Thu, Nov 10, 2011 at 3:13 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Wednesday 09 November 2011 16:42:51 Graeme Russ wrote:
>> On Thu, Nov 10, 2011 at 4:12 AM, Mike Frysinger wrote:
>> > On Wednesday 09 November 2011 05:32:59 Graeme Russ wrote:
>> >> --- a/arch/x86/config.mk
>> >> +++ b/arch/x86/config.mk
>> >>
>> >> -PLATFORM_CPPFLAGS += -mregparm=3
>> >> -PLATFORM_CPPFLAGS += -fomit-frame-pointer
>> >
>> > this sounds like you're throwing the baby out with the bath water.
>> >  doesn't this severely affect the generated code ?
>>
>> No - omit-frame-pointer is enabled by -O2 and also:
>>
>> http://gcc.gnu.org/gcc-4.6/changes.html
>>
>> "The default setting (when not optimizing for size) for 32-bit
>> GNU/Linux and Darwin x86 targets has been changed to
>> -fomit-frame-pointer. The default can be reverted to
>> -fno-omit-frame-pointer by configuring GCC with the
>> --enable-frame-pointer configure option."
>>
>> So the flag is a do-nothing
>
> ok, except:
>  - we build u-boot with -Os and not -O2
>  - i'd say most people aren't using gcc-4.6

Ah, OK then I'll leave it in - It was only a cruft reduction plan anyway :)

> i was referring also to throwing away -mregparm=3 ...

Yes, it does effect the code - It makes it ABI compliant like everyone
else (except ARM) :) I expect a code size increase (have not measured
it yet)

As I've stated, I really do not want arbitrary wrapper functions where
it is not obvious that they need to be updated if new code uses
previously unused (and unwrapped) libgcc functions (in particular if
there are new libgcc functions in the future which we can't wrap
todday anyway)

Option a) is to remove regparm=3
Option b) is to use private libgcc
Option c) is to use wrappers

If this patch works, I'll look at the code impact and we can discuss
which option we take :)

Regards,

Graeme
Graeme Russ Nov. 10, 2011, 5:10 a.m. UTC | #5
Hi Mike,

On Thu, Nov 10, 2011 at 3:22 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Mike,
>
> On Thu, Nov 10, 2011 at 3:13 PM, Mike Frysinger <vapier@gentoo.org> wrote:
>> On Wednesday 09 November 2011 16:42:51 Graeme Russ wrote:
>>> On Thu, Nov 10, 2011 at 4:12 AM, Mike Frysinger wrote:
>>> > On Wednesday 09 November 2011 05:32:59 Graeme Russ wrote:
>>> >> --- a/arch/x86/config.mk
>>> >> +++ b/arch/x86/config.mk
>>> >>
>>> >> -PLATFORM_CPPFLAGS += -mregparm=3
>>> >> -PLATFORM_CPPFLAGS += -fomit-frame-pointer
>>> >
>>> > this sounds like you're throwing the baby out with the bath water.
>>> >  doesn't this severely affect the generated code ?
>>>
>>> No - omit-frame-pointer is enabled by -O2 and also:
>>>
>>> http://gcc.gnu.org/gcc-4.6/changes.html
>>>
>>> "The default setting (when not optimizing for size) for 32-bit
>>> GNU/Linux and Darwin x86 targets has been changed to
>>> -fomit-frame-pointer. The default can be reverted to
>>> -fno-omit-frame-pointer by configuring GCC with the
>>> --enable-frame-pointer configure option."
>>>
>>> So the flag is a do-nothing
>>
>> ok, except:
>>  - we build u-boot with -Os and not -O2
>>  - i'd say most people aren't using gcc-4.6

http://gcc.gnu.org/onlinedocs/gcc-4.6.2/gcc/Optimize-Options.html#Optimize-Options

-fomit-frame-pointer
...
Enabled at levels -O, -O2, -O3, -Os

Thats goes back to at least gcc 4.3.x

But I'll keep it in anyway, just in case someone wants to remove -Os
(for testing/debugging etc)

Regards,

Graeme
Mike Frysinger Nov. 10, 2011, 5:15 p.m. UTC | #6
On Wednesday 09 November 2011 23:22:34 Graeme Russ wrote:
> On Thu, Nov 10, 2011 at 3:13 PM, Mike Frysinger wrote:
> > i was referring also to throwing away -mregparm=3 ...
> 
> Yes, it does effect the code - It makes it ABI compliant like everyone
> else (except ARM) :) I expect a code size increase (have not measured
> it yet)

ABI compliance only matters at the boundaries.  since u-boot is largely self-
contained, we shouldn't be afraid to break internal ABI.

> As I've stated, I really do not want arbitrary wrapper functions where
> it is not obvious that they need to be updated if new code uses
> previously unused (and unwrapped) libgcc functions (in particular if
> there are new libgcc functions in the future which we can't wrap
> todday anyway)
> 
> Option a) is to remove regparm=3
> Option b) is to use private libgcc
> Option c) is to use wrappers
> 
> If this patch works, I'll look at the code impact and we can discuss
> which option we take :)

for the record, i'm not against a private libgcc.  it just seems to me that 
the wrapper approach proposed by Gabe has the best pro/con ratio.
-mike
Graeme Russ Nov. 10, 2011, 10:53 p.m. UTC | #7
Hi Mike,

On Fri, Nov 11, 2011 at 4:15 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Wednesday 09 November 2011 23:22:34 Graeme Russ wrote:
>> On Thu, Nov 10, 2011 at 3:13 PM, Mike Frysinger wrote:
>> > i was referring also to throwing away -mregparm=3 ...
>>
>> Yes, it does effect the code - It makes it ABI compliant like everyone
>> else (except ARM) :) I expect a code size increase (have not measured
>> it yet)
>
> ABI compliance only matters at the boundaries.  since u-boot is largely self-
> contained, we shouldn't be afraid to break internal ABI.

And I'm not afraid to do so

>> As I've stated, I really do not want arbitrary wrapper functions where
>> it is not obvious that they need to be updated if new code uses
>> previously unused (and unwrapped) libgcc functions (in particular if
>> there are new libgcc functions in the future which we can't wrap
>> todday anyway)
>>
>> Option a) is to remove regparm=3
>> Option b) is to use private libgcc
>> Option c) is to use wrappers
>>
>> If this patch works, I'll look at the code impact and we can discuss
>> which option we take :)
>
> for the record, i'm not against a private libgcc.  it just seems to me that
> the wrapper approach proposed by Gabe has the best pro/con ratio.

I'm sorry, but I must respectfully disagree...

The biggest con with wrappers is that the proposed patch only wraps four
functions. arch/arm/lib/ has private libgcc implementations for eight
libgcc functions - I can only assume they are used somewhere. The kicker
is that if anyone uses a libgcc function which is not one of the four
already wrapped, and they do not realise this and fail to wrap them
themselves, no warning will be given by the compiler or linker. Now that
unwrapped function may be in a rarely executed code path (as evidenced by
the fact that this bug has been dormant for a year now). So you could have
in-the-wild version of U-Boot which start exhibiting strange behaviour and
nobody knows why

Another big(ish) con, for me, is that we already have a mechanism in place
to resolve this (USE_PRIVATE_LIBGCC) - I don't see any benefit to add a
second to the mix

The final (trivially small) con is the overhead added to these calls

Now if we use USE_PRIVATE_LIBGCC, unimplemented libgcc functions will
result in link errors, so using an unimplemented libgcc will be obvious at
build time - It may lead to a posting on the mailing list, but at least we
won't have latent libgcc related bugs in-the-wild.

Also, We use an existing mechanism and it is in keeping with --no-builtin.
libgcc is really just a library of functions that are too large to
implement as inline functions internally by gcc anyway:

"GCC provides a low-level runtime library, libgcc.a or libgcc_s.so.1 on
 some platforms. GCC generates calls to routines in this library
 automatically, whenever it needs to perform some operation that is too
 complicated to emit inline code for."

Now the downside has been raised regarding keeping the private libgcc
functions in-sync with mainline libgcc - We are only talking about a
handful of functions. There are no floating point or 'special' functions,
so the list is wholly restricted to:

http://gcc.gnu.org/onlinedocs/gccint/Integer-library-routines.html

I haven't looked at them, but I doubt they are very big (going by the ARM
implementations) and I doubt they change very often (probably not in years)

The more I think about this, the more I feel to just use USE_PRIVATE_LIBGCC

Regards,

Graeme
Mike Frysinger Nov. 11, 2011, 12:23 a.m. UTC | #8
On Thursday 10 November 2011 17:53:06 Graeme Russ wrote:
> The biggest con with wrappers is that the proposed patch only wraps four
> functions. arch/arm/lib/ has private libgcc implementations for eight
> libgcc functions - I can only assume they are used somewhere.

i don't think you can look across arches like that.  arm provides a lot more 
libgcc funcs because it, like most RISC/embedded cpus, do not provide 
dedicated math insns in the ISA.  or the number of insns is so large, that 
creating a dedicated library func and emitting a function call makes more 
sense than emitting them inline.  x86 is a fairly "fat" ISA in that most math 
operations can be accomplished in single or a few insns, and is certainly 
better than emitting func calls to an external library.

in fact, building the current eNET board (the only x86 board) shows that it 
doesn't use *any* calls from libgcc:
	make PLATFORM_LIBGCC= eNET -j4

> The kicker
> is that if anyone uses a libgcc function which is not one of the four
> already wrapped, and they do not realise this and fail to wrap them
> themselves, no warning will be given by the compiler or linker. Now that
> unwrapped function may be in a rarely executed code path (as evidenced by
> the fact that this bug has been dormant for a year now). So you could have
> in-the-wild version of U-Boot which start exhibiting strange behaviour and
> nobody knows why

yes, but the better question is whether those funcs should be called in the 
first place

> The final (trivially small) con is the overhead added to these calls

this con is insignificant when weighed against the alternatives: not using 
regparm anywhere.  further, these funcs are rarely used, so you're talking 
about adding a minor amount of overhead to one or two call sites.

> Now if we use USE_PRIVATE_LIBGCC, unimplemented libgcc functions will
> result in link errors, so using an unimplemented libgcc will be obvious at
> build time - It may lead to a posting on the mailing list, but at least we
> won't have latent libgcc related bugs in-the-wild.

perhaps x86 should be setting PLATFORM_LIBGCC to nothing all the time.  the 
funcs Gabe wants to wrap are 64bit operations.  u-boot should not be doing 64-
bit operations.  that's why we have do_div().
-mike
Graeme Russ Nov. 11, 2011, 1:23 a.m. UTC | #9
Hi Mike,

On Fri, Nov 11, 2011 at 11:23 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Thursday 10 November 2011 17:53:06 Graeme Russ wrote:
>> The biggest con with wrappers is that the proposed patch only wraps four
>> functions. arch/arm/lib/ has private libgcc implementations for eight
>> libgcc functions - I can only assume they are used somewhere.
>
> i don't think you can look across arches like that.  arm provides a lot more
> libgcc funcs because it, like most RISC/embedded cpus, do not provide
> dedicated math insns in the ISA.  or the number of insns is so large, that
> creating a dedicated library func and emitting a function call makes more
> sense than emitting them inline.  x86 is a fairly "fat" ISA in that most math
> operations can be accomplished in single or a few insns, and is certainly
> better than emitting func calls to an external library.
>
> in fact, building the current eNET board (the only x86 board) shows that it
> doesn't use *any* calls from libgcc:
>        make PLATFORM_LIBGCC= eNET -j4

Which supports my point - The issue was latent because there were no call
sites

>
>> The kicker
>> is that if anyone uses a libgcc function which is not one of the four
>> already wrapped, and they do not realise this and fail to wrap them
>> themselves, no warning will be given by the compiler or linker. Now that
>> unwrapped function may be in a rarely executed code path (as evidenced by
>> the fact that this bug has been dormant for a year now). So you could have
>> in-the-wild version of U-Boot which start exhibiting strange behaviour and
>> nobody knows why
>
> yes, but the better question is whether those funcs should be called in the
> first place

But we can't control that - especially if the code is not submitted to
mainline. Then get people hitting the ML asking for help with non mainline
code because they used a function they did not know they should not have
used and they are seeing some random weird crash

>> The final (trivially small) con is the overhead added to these calls
>
> this con is insignificant when weighed against the alternatives: not using
> regparm anywhere.  further, these funcs are rarely used, so you're talking
> about adding a minor amount of overhead to one or two call sites.

100% agree

>> Now if we use USE_PRIVATE_LIBGCC, unimplemented libgcc functions will
>> result in link errors, so using an unimplemented libgcc will be obvious at
>> build time - It may lead to a posting on the mailing list, but at least we
>> won't have latent libgcc related bugs in-the-wild.
>
> perhaps x86 should be setting PLATFORM_LIBGCC to nothing all the time.  the
> funcs Gabe wants to wrap are 64bit operations.  u-boot should not be doing 64-
> bit operations.  that's why we have do_div().

Remember that there was a lot of discussion regarding the timer API that
pointed towards using 64-bit counters for all arches. We cannot take it
as gospel that 64-bit operations will never be used in U-Boot. Some people
may have a need for this as part of hardware init.

Regards,

Graeme
Mike Frysinger Nov. 11, 2011, 1:40 a.m. UTC | #10
On Thursday 10 November 2011 20:23:49 Graeme Russ wrote:
> On Fri, Nov 11, 2011 at 11:23 AM, Mike Frysinger wrote:
> > On Thursday 10 November 2011 17:53:06 Graeme Russ wrote:
> >> Now if we use USE_PRIVATE_LIBGCC, unimplemented libgcc functions will
> >> result in link errors, so using an unimplemented libgcc will be obvious
> >> at build time - It may lead to a posting on the mailing list, but at
> >> least we won't have latent libgcc related bugs in-the-wild.
> > 
> > perhaps x86 should be setting PLATFORM_LIBGCC to nothing all the time. 
> > the funcs Gabe wants to wrap are 64bit operations.  u-boot should not be
> > doing 64- bit operations.  that's why we have do_div().
> 
> Remember that there was a lot of discussion regarding the timer API that
> pointed towards using 64-bit counters for all arches. We cannot take it
> as gospel that 64-bit operations will never be used in U-Boot. Some people
> may have a need for this as part of hardware init.

Linux has no problem doing 64bit timers without 64bit mul/div.  i don't see 
how u-boot could possibly be more special than Linux.
-mike
Graeme Russ Nov. 11, 2011, 1:51 a.m. UTC | #11
Hi Mike,

On Fri, Nov 11, 2011 at 12:40 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Thursday 10 November 2011 20:23:49 Graeme Russ wrote:
>> On Fri, Nov 11, 2011 at 11:23 AM, Mike Frysinger wrote:
>> > On Thursday 10 November 2011 17:53:06 Graeme Russ wrote:
>> >> Now if we use USE_PRIVATE_LIBGCC, unimplemented libgcc functions will
>> >> result in link errors, so using an unimplemented libgcc will be obvious
>> >> at build time - It may lead to a posting on the mailing list, but at
>> >> least we won't have latent libgcc related bugs in-the-wild.
>> >
>> > perhaps x86 should be setting PLATFORM_LIBGCC to nothing all the time.
>> > the funcs Gabe wants to wrap are 64bit operations.  u-boot should not be
>> > doing 64- bit operations.  that's why we have do_div().
>>
>> Remember that there was a lot of discussion regarding the timer API that
>> pointed towards using 64-bit counters for all arches. We cannot take it
>> as gospel that 64-bit operations will never be used in U-Boot. Some people
>> may have a need for this as part of hardware init.
>
> Linux has no problem doing 64bit timers without 64bit mul/div.  i don't see
> how u-boot could possibly be more special than Linux.

A few questions (I am unfamiliar with the Linux build environment):

 a) Does Linux link to libgcc
 b) Does Linux use regparm
 c) If a & b are both yes, does Linux use wrappers for libgcc functions

Regards,

Graeme
Mike Frysinger Nov. 11, 2011, 1:55 a.m. UTC | #12
On Thursday 10 November 2011 20:51:47 Graeme Russ wrote:
> A few questions (I am unfamiliar with the Linux build environment):
> 
>  a) Does Linux link to libgcc

no Linux port uses libgcc.  they've always done the equivalent of 
PRIVATE_LIBGCC.  but in the case of x86, i can't see them providing any libgcc 
funcs.  so i don't think u-boot should either.

>  b) Does Linux use regparm

yes, it uses -mregparm=3
-mike
Graeme Russ Nov. 11, 2011, 1:59 a.m. UTC | #13
Hi Mike,

On Fri, Nov 11, 2011 at 12:55 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Thursday 10 November 2011 20:51:47 Graeme Russ wrote:
>> A few questions (I am unfamiliar with the Linux build environment):
>>
>>  a) Does Linux link to libgcc
>
> no Linux port uses libgcc.  they've always done the equivalent of
> PRIVATE_LIBGCC.  but in the case of x86, i can't see them providing any libgcc
> funcs.  so i don't think u-boot should either.
>
>>  b) Does Linux use regparm
>
> yes, it uses -mregparm=3

Well I think we have an answer - use PRIVATE_LIBGCC but do not implement
any libgcc functions and treat link errors as coding errors. If for some
bizarre reason we need to really, truly, honestly use a 64-bit libgcc
function, we'll port it over then

Regards,

Graeme
Gabe Black Nov. 11, 2011, 2:10 a.m. UTC | #14
On Thu, Nov 10, 2011 at 5:59 PM, Graeme Russ <graeme.russ@gmail.com> wrote:

> Hi Mike,
>
> On Fri, Nov 11, 2011 at 12:55 PM, Mike Frysinger <vapier@gentoo.org>
> wrote:
> > On Thursday 10 November 2011 20:51:47 Graeme Russ wrote:
> >> A few questions (I am unfamiliar with the Linux build environment):
> >>
> >>  a) Does Linux link to libgcc
> >
> > no Linux port uses libgcc.  they've always done the equivalent of
> > PRIVATE_LIBGCC.  but in the case of x86, i can't see them providing any
> libgcc
> > funcs.  so i don't think u-boot should either.
> >
> >>  b) Does Linux use regparm
> >
> > yes, it uses -mregparm=3
>
> Well I think we have an answer - use PRIVATE_LIBGCC but do not implement
> any libgcc functions and treat link errors as coding errors. If for some
> bizarre reason we need to really, truly, honestly use a 64-bit libgcc
> function, we'll port it over then
>
> Regards,
>
> Graeme
>

I haven't checked in on this thread in a little while, but I wanted to
point out some things. First, I was originally planning to measure the
performance difference with regparm turned off. I realized that would be
quite annoying to actually do, though, since we have a number of extra
libraries linked into u-boot and they would all have to be recompiled with
different options. Then the things they link with would have to be
recompiled, etc. Even if upstream U-Boot drops regparm, we may need to keep
it just for that reason.

Second, I think I have a solution that preserves regparm, keeps libgcc and
gcc in sync, and also stops unwrapped functions slipping into u-boot. We
can use this command:

objcopy /path/to/your/libgcc/libgcc.a
/somewhere/in/the/u-boot/build/libgcc.a --prefix-symbols=__real

to create a libgcc that has all of its symbols prefixed with the string
__real. Then *all* symbols are prepped for wrapping, regardless of if we
use them now or even know about them. Only the symbols we've explicitly
wrapped will be available. Note that I haven't actually implemented this
yet, but it seems to me like it captures the positive aspects of all the
alternatives.

Gabe
Graeme Russ Nov. 11, 2011, 2:22 a.m. UTC | #15
Hi Gabe,

On Fri, Nov 11, 2011 at 1:10 PM, Gabe Black <gabeblack@google.com> wrote:
> On Thu, Nov 10, 2011 at 5:59 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
>>
>> Hi Mike,
>>
>> On Fri, Nov 11, 2011 at 12:55 PM, Mike Frysinger <vapier@gentoo.org>
>> wrote:
>> > On Thursday 10 November 2011 20:51:47 Graeme Russ wrote:
>> >> A few questions (I am unfamiliar with the Linux build environment):
>> >>
>> >>  a) Does Linux link to libgcc
>> >
>> > no Linux port uses libgcc.  they've always done the equivalent of
>> > PRIVATE_LIBGCC.  but in the case of x86, i can't see them providing any
>> > libgcc
>> > funcs.  so i don't think u-boot should either.
>> >
>> >>  b) Does Linux use regparm
>> >
>> > yes, it uses -mregparm=3
>>
>> Well I think we have an answer - use PRIVATE_LIBGCC but do not implement
>> any libgcc functions and treat link errors as coding errors. If for some
>> bizarre reason we need to really, truly, honestly use a 64-bit libgcc
>> function, we'll port it over then
>>
>> Regards,
>>
>> Graeme
>
> I haven't checked in on this thread in a little while, but I wanted to point
> out some things. First, I was originally planning to measure the performance
> difference with regparm turned off. I realized that would be quite annoying
> to actually do, though, since we have a number of extra libraries linked
> into u-boot and they would all have to be recompiled with different options.

I assume they are all GPL :)

> Then the things they link with would have to be recompiled, etc. Even if
> upstream U-Boot drops regparm, we may need to keep it just for that reason.

Hmm, that raises an interresing point - by using a non-standard ABI we can
run afoul of external libraries. Which raises the question - what external
libraries do you need to link to?

> Second, I think I have a solution that preserves regparm, keeps libgcc and
> gcc in sync, and also stops unwrapped functions slipping into u-boot. We can
> use this command:
> objcopy /path/to/your/libgcc/libgcc.a
> /somewhere/in/the/u-boot/build/libgcc.a --prefix-symbols=__real
> to create a libgcc that has all of its symbols prefixed with the string
> __real. Then *all* symbols are prepped for wrapping, regardless of if we use
> them now or even know about them. Only the symbols we've explicitly wrapped
> will be available. Note that I haven't actually implemented this yet, but it
> seems to me like it captures the positive aspects of all the alternatives.

Sorry, I find that rather un-aesthetic - We are only looking at a trivial
handful of functions, and to copy and rename ALL the symbols in libgcc.a
is rather overkill

The other thing I like about using PRIVATE_LIBGCC is that on 64-bit build
platforms, we don't need to worry about installing 32-bit libraries - All
we need to do is add the -m32 option to tell gcc and ld to generate 32-bit
code

Regards,

Graeme
Gabe Black Nov. 11, 2011, 2:41 a.m. UTC | #16
On Thu, Nov 10, 2011 at 6:22 PM, Graeme Russ <graeme.russ@gmail.com> wrote:

> Hi Gabe,
>
> On Fri, Nov 11, 2011 at 1:10 PM, Gabe Black <gabeblack@google.com> wrote:
> > On Thu, Nov 10, 2011 at 5:59 PM, Graeme Russ <graeme.russ@gmail.com>
> wrote:
> >>
> >> Hi Mike,
> >>
> >> On Fri, Nov 11, 2011 at 12:55 PM, Mike Frysinger <vapier@gentoo.org>
> >> wrote:
> >> > On Thursday 10 November 2011 20:51:47 Graeme Russ wrote:
> >> >> A few questions (I am unfamiliar with the Linux build environment):
> >> >>
> >> >>  a) Does Linux link to libgcc
> >> >
> >> > no Linux port uses libgcc.  they've always done the equivalent of
> >> > PRIVATE_LIBGCC.  but in the case of x86, i can't see them providing
> any
> >> > libgcc
> >> > funcs.  so i don't think u-boot should either.
> >> >
> >> >>  b) Does Linux use regparm
> >> >
> >> > yes, it uses -mregparm=3
> >>
> >> Well I think we have an answer - use PRIVATE_LIBGCC but do not implement
> >> any libgcc functions and treat link errors as coding errors. If for some
> >> bizarre reason we need to really, truly, honestly use a 64-bit libgcc
> >> function, we'll port it over then
> >>
> >> Regards,
> >>
> >> Graeme
> >
> > I haven't checked in on this thread in a little while, but I wanted to
> point
> > out some things. First, I was originally planning to measure the
> performance
> > difference with regparm turned off. I realized that would be quite
> annoying
> > to actually do, though, since we have a number of extra libraries linked
> > into u-boot and they would all have to be recompiled with different
> options.
>
> I assume they are all GPL :)
>
> > Then the things they link with would have to be recompiled, etc. Even if
> > upstream U-Boot drops regparm, we may need to keep it just for that
> reason.
>
> Hmm, that raises an interresing point - by using a non-standard ABI we can
> run afoul of external libraries. Which raises the question - what external
> libraries do you need to link to?
>


You have it reversed. By using a standard ABI we run afoul of external
libraries. We implement our verified boot infrastructure as a library most
notably, and it links in a few other libraries.



>
> > Second, I think I have a solution that preserves regparm, keeps libgcc
> and
> > gcc in sync, and also stops unwrapped functions slipping into u-boot. We
> can
> > use this command:
> > objcopy /path/to/your/libgcc/libgcc.a
> > /somewhere/in/the/u-boot/build/libgcc.a --prefix-symbols=__real
> > to create a libgcc that has all of its symbols prefixed with the string
> > __real. Then *all* symbols are prepped for wrapping, regardless of if we
> use
> > them now or even know about them. Only the symbols we've explicitly
> wrapped
> > will be available. Note that I haven't actually implemented this yet,
> but it
> > seems to me like it captures the positive aspects of all the
> alternatives.
>
> Sorry, I find that rather un-aesthetic - We are only looking at a trivial
> handful of functions, and to copy and rename ALL the symbols in libgcc.a
> is rather overkill
>
> The other thing I like about using PRIVATE_LIBGCC is that on 64-bit build
> platforms, we don't need to worry about installing 32-bit libraries - All
> we need to do is add the -m32 option to tell gcc and ld to generate 32-bit
> code



This argument really doesn't make any sense to me. The number of symbols
involved doesn't make any difference at all. You could imagine we are only
copying a few if you want and there would be no visible, hence aesthetic,
difference. Reimplementing libgcc, along with the inevitable bugs that go
along with new code, breaking builds when new functions are added silently
by new versions of gcc, etc., seems much less appealing in basically every
respect.

Gabe
Mike Frysinger Nov. 11, 2011, 2:44 a.m. UTC | #17
On Thursday 10 November 2011 20:59:46 Graeme Russ wrote:
> Well I think we have an answer - use PRIVATE_LIBGCC but do not implement
> any libgcc functions and treat link errors as coding errors. If for some
> bizarre reason we need to really, truly, honestly use a 64-bit libgcc
> function, we'll port it over then

that's not how USE_PRIVATE_LIBGCC works in u-boot though.  i'd suggest:
	echo "PLATFORM_LIBGCC :=" >> arch/x86/config.mk
-mike
Graeme Russ Nov. 11, 2011, 4:49 a.m. UTC | #18
Hi Gabe,

On Fri, Nov 11, 2011 at 1:41 PM, Gabe Black <gabeblack@google.com> wrote:
>
>
> On Thu, Nov 10, 2011 at 6:22 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
>>
>> Hi Gabe,
>>
>> On Fri, Nov 11, 2011 at 1:10 PM, Gabe Black <gabeblack@google.com> wrote:
>> > On Thu, Nov 10, 2011 at 5:59 PM, Graeme Russ <graeme.russ@gmail.com>
>> > wrote:
>> >>
>> >> Hi Mike,
>> >>
>> >> On Fri, Nov 11, 2011 at 12:55 PM, Mike Frysinger <vapier@gentoo.org>
>> >> wrote:
>> >> > On Thursday 10 November 2011 20:51:47 Graeme Russ wrote:

[snip]

>> Hmm, that raises an interresing point - by using a non-standard ABI we can
>> run afoul of external libraries. Which raises the question - what external
>> libraries do you need to link to?
>
>
> You have it reversed. By using a standard ABI we run afoul of external
> libraries. We implement our verified boot infrastructure as a library most
> notably, and it links in a few other libraries.

Ah, so your libraries are regparm(3)

Still, life is going to get interesting for anyone linking in libraries
that are not the saem ABI as U-Boot (as we have seen with libgcc)

>> > Second, I think I have a solution that preserves regparm, keeps libgcc
>> > and
>> > gcc in sync, and also stops unwrapped functions slipping into u-boot. We
>> > can
>> > use this command:
>> > objcopy /path/to/your/libgcc/libgcc.a
>> > /somewhere/in/the/u-boot/build/libgcc.a --prefix-symbols=__real
>> > to create a libgcc that has all of its symbols prefixed with the string
>> > __real. Then *all* symbols are prepped for wrapping, regardless of if we
>> > use
>> > them now or even know about them. Only the symbols we've explicitly
>> > wrapped
>> > will be available. Note that I haven't actually implemented this yet,
>> > but it
>> > seems to me like it captures the positive aspects of all the
>> > alternatives.
>>
>> Sorry, I find that rather un-aesthetic - We are only looking at a trivial
>> handful of functions, and to copy and rename ALL the symbols in libgcc.a
>> is rather overkill
>>
>> The other thing I like about using PRIVATE_LIBGCC is that on 64-bit build
>> platforms, we don't need to worry about installing 32-bit libraries - All
>> we need to do is add the -m32 option to tell gcc and ld to generate 32-bit
>> code
>
> This argument really doesn't make any sense to me. The number of symbols
> involved doesn't make any difference at all. You could imagine we are only
> copying a few if you want and there would be no visible, hence aesthetic,
> difference. Reimplementing libgcc, along with the inevitable bugs that go
> along with new code, breaking builds when new functions are added silently
> by new versions of gcc, etc., seems much less appealing in basically every
> respect.

Remember, U-Boot uses --no-builtin, so apart from the libgcc functions,
there are no gcc functions included. And we are not 'reimplementing
libgcc', we are either 'reimplementing a select few functions' or 'not
using any libgcc functions in U-Boot at all'. So a build will only break
when code is added that uses a function never before used - There should
not be any build breakage due to 'functions added silently by newer
versions of gcc'

I'm assuming that it is the 'verified boot infrastructure' libraries that
are introducing the calls to libgcc? If that is the case, then it is
starting to get very messy - Why should U-Boot have to deal with this?

Regards,

Graeme
Mike Frysinger Nov. 11, 2011, 5:04 a.m. UTC | #19
On Thursday 10 November 2011 23:49:07 Graeme Russ wrote:
> Remember, U-Boot uses --no-builtin, so apart from the libgcc functions,
> there are no gcc functions included.

i don't think that's generally how gcc builtin's work.  for the vast majority, 
they're of the "optimize away with simple insns when possible" variety.  so if 
you do something like:
	char c[4];
	memset(c, 0, sizeof(c));
gcc will optimize that into a single 32bit load rather than calling memcpy().  
but because we use -fno-builtins, gcc will make sure to call memcpy().

i can't think of any calls off the top of my head which would result in 
invoking a func in libgcc.a.
-mike
Graeme Russ Nov. 11, 2011, 5:16 a.m. UTC | #20
Hi Mike,

On Fri, Nov 11, 2011 at 4:04 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Thursday 10 November 2011 23:49:07 Graeme Russ wrote:
>> Remember, U-Boot uses --no-builtin, so apart from the libgcc functions,
>> there are no gcc functions included.
>
> i don't think that's generally how gcc builtin's work.  for the vast majority,
> they're of the "optimize away with simple insns when possible" variety.  so if
> you do something like:
>        char c[4];
>        memset(c, 0, sizeof(c));
> gcc will optimize that into a single 32bit load rather than calling memcpy().
> but because we use -fno-builtins, gcc will make sure to call memcpy().

List of builtin functions not in libgcc:

http://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html

> i can't think of any calls off the top of my head which would result in
> invoking a func in libgcc.a.

Any function listed here:

http://gcc.gnu.org/onlinedocs/gccint/Libgcc.html

But we can discount any float/double routines, exception handling and
split stack which leaves just:

http://gcc.gnu.org/onlinedocs/gccint/Integer-library-routines.html

Regards,

Graeme
Mike Frysinger Nov. 11, 2011, 4:24 p.m. UTC | #21
On Friday 11 November 2011 00:16:47 Graeme Russ wrote:
> On Fri, Nov 11, 2011 at 4:04 PM, Mike Frysinger wrote:
> > i can't think of any calls off the top of my head which would result in
> > invoking a func in libgcc.a.
> 
> Any function listed here:
> 
> http://gcc.gnu.org/onlinedocs/gccint/Libgcc.html
> 
> But we can discount any float/double routines, exception handling and
> split stack which leaves just:
> 
> http://gcc.gnu.org/onlinedocs/gccint/Integer-library-routines.html

yes, and pretty much all of those are emitted implicitly due to math 
operations (64bit divs/mults/etc...), or explicitly when the user does 
__builtin_xxx() (and use of __builtin_xxx is not affected by -fno-builtins).  
none of those that i can see would come via a C library call that gcc would 
implicitly rewrite.
-mike
Scott Wood Nov. 11, 2011, 7:59 p.m. UTC | #22
On 11/10/2011 07:55 PM, Mike Frysinger wrote:
> On Thursday 10 November 2011 20:51:47 Graeme Russ wrote:
>> A few questions (I am unfamiliar with the Linux build environment):
>>
>>  a) Does Linux link to libgcc
> 
> no Linux port uses libgcc.  they've always done the equivalent of 
> PRIVATE_LIBGCC.  but in the case of x86, i can't see them providing any libgcc 
> funcs.  so i don't think u-boot should either.

Some of the less common architectures (openrisc, h8300, cris, m32r,
tile, xtensa) appear to use libgcc.

unicore32 appears to pull selected objects out of both libgcc and libc.

-Scott
Graeme Russ Nov. 16, 2011, 11 p.m. UTC | #23
Hi Gabe,

On Wed, Nov 9, 2011 at 9:32 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Gabe,
>
> Can you please try this patch - If it solves your libgcc problem, I will
> add it to the misc cleanup patch
>
> Thanks,
>
> Graeme
> ---
>  arch/x86/config.mk        |    3 ---
>  arch/x86/cpu/interrupts.c |    2 +-
>  arch/x86/cpu/start.S      |    5 ++---
>  3 files changed, 3 insertions(+), 7 deletions(-)

I think we all agree this is not the way to go

Regards,

Graeme
diff mbox

Patch

diff --git a/arch/x86/config.mk b/arch/x86/config.mk
index fe9083f..ec5f707 100644
--- a/arch/x86/config.mk
+++ b/arch/x86/config.mk
@@ -23,10 +23,7 @@ 

 CONFIG_STANDALONE_LOAD_ADDR ?= 0x40000

-PLATFORM_CPPFLAGS += -fno-strict-aliasing
 PLATFORM_CPPFLAGS += -Wstrict-prototypes
-PLATFORM_CPPFLAGS += -mregparm=3
-PLATFORM_CPPFLAGS += -fomit-frame-pointer
 PF_CPPFLAGS_X86   := $(call cc-option, -ffreestanding) \
 		     $(call cc-option, -fno-toplevel-reorder, \
 		       $(call cc-option, -fno-unit-at-a-time)) \
diff --git a/arch/x86/cpu/interrupts.c b/arch/x86/cpu/interrupts.c
index e0958eb..a15d70a 100644
--- a/arch/x86/cpu/interrupts.c
+++ b/arch/x86/cpu/interrupts.c
@@ -249,7 +249,7 @@  int disable_interrupts(void)
 }

 /* IRQ Low-Level Service Routine */
-void irq_llsr(struct irq_regs *regs)
+void __attribute__ ((regparm(1))) irq_llsr(struct irq_regs *regs)
 {
 	/*
 	 * For detailed description of each exception, refer to:
diff --git a/arch/x86/cpu/start.S b/arch/x86/cpu/start.S
index f87633b..119ca2d 100644
--- a/arch/x86/cpu/start.S
+++ b/arch/x86/cpu/start.S
@@ -84,9 +84,8 @@  car_init_ret:
 	 */
 	movl	$CONFIG_SYS_INIT_SP_ADDR, %esp

-	/* Set parameter to board_init_f() to boot flags */
-	xorl	%eax, %eax
-	movw	%bx, %ax
+	/* Set parameter to board_init_f() - Unused dummy value */
+	pushl	$0

 	/* Enter, U-boot! */
 	call	board_init_f