Hello,
[ Replying to my own post since I'm not subscribed to the mailing list ]
I understand your objections about the libm patch, I sent it because because I saw you listed these functions on the libpayload page on the coreboot website so I assumed you might have an interest in adding these to libpayload. I guess it's probably not all that useful to have math support for most applications that use libpayload.
As Patrick pointed out, FPU exceptions are not currently handled, resulting in weird things like log(0) = 0 etc, but at the same time "normal" cases are computed with the same kind of precision one would expect from your everyday libc so if this caveat is documented this code can still be useful for a lot of cases.
Right now on x86, most of the math functions from this patch are actually from the asm sources in libpayload/libm/arch/i387/: they are quite small (1000 loc) and don't depend on other stuff (and except the exp implementation they don't use the 4 clause BSD license). The C source equivalents are there as a fallback for other archs: I kept them because I saw powerpc was also supported. So if it's OK to have x86 only code in libpayload and to drop some C implementations, it's quite easy to only add the x86 asm sources with the caveat of not handling FPU exceptions: if you want some kind of math support in libpayload without adding too much bloat it's probably not a bad deal. Are you interested in having something like that ?
My answers to specific questions below.
Regards, Sylvain
On 2/7/10 5:29 PM, Patrick Georgi wrote:
Then, there are design questions: this libm is made for userland, with a kernel expected to provide support if anything goes wrong. How does the math library cope with FPU exceptions? Does it at all?
I have to second that
Ok but if you were to redo a math library custom made for libpayload that handles FPU exceptions these sources are still a good starting point that will save a lot of work. I doubt a doing non-userland libm would end up with code very much different from what's already in there.
It increases code size of libpayload roughly by a factor of five.. That's an incredible amount of code.
The source size is big but the resulting compiled code size is reasonable. For example with Tinyscheme, linking in the double version of all the math functions results in a 8kb increase.
The code is unfortunately not quite clean and not really adapted to libpayload either. In fact, it looks like the BSD sources have mostly been just untarred in the libpayload directory.
A few checks show
- string.h provides functions like ffs() which is never defined
- string.h provides prototypes for many functions that are still listed
in libpayload.h, like memcpy, memcmp etc
I kind of saw the fact that it uses pristine BSD sources/headers as a feature :) (easier to maintain in the long run and easier to port an application to a library that's structured in the same way as BSD). It's true there's a bunch of functions in these that are not implemented but this doesn't break anything and should you need to implement them in the future the prototype is already there.
- lots of odd undocumented functionality like several setjmp implementations
These can be removed.
- patch to readline() breaks existing code out there
Sorry, please disregard that. I didn't see it was possible to get the functionality I needed using getline.
Roughly I think that this approach of adding code to libpayload is not preferrable. It would be much nicer to see that those functions used by tinyscheme are chosen carefully, be added to libpayload in minimal implementations and commented in the same style we commented all the rest of libpayload.
Tinyscheme doesn't actually need that floating point/math stuff to run. It's configurable at compile time.
As for tinyscheme, if it is maintained it might be a better idea to try to get payload-related changes (if any) into upstream, instead of starting a fork in our repository.
I'll talk to them but I doubt they want to maintain another bunch of #ifdef. How about I send you a patch against the latest version with build instructions for inclusion here: http://www.coreboot.org/Payloads ?