On 1 July 2011 20:38, Joseph S. Myers <jos...@codesourcery.com> wrote:
Hi Joseph,
Thanks for your comments.
> On Fri, 1 Jul 2011, Dr. David Alan Gilbert wrote:
>
>> +/* For write */
>> +#include <unistd.h>
>> +/* For abort */
>> +#include <stdlib.h>
>
> Please don't include system headers in libgcc without appropriate
> inhibit_libc checks for bootstrap purposes. In this case, it would seem
> better just to declare the functions you need.
OK.
>> +/* Check that the kernel has a new enough version at load */
>> +void __check_for_sync8_kernelhelper (void)
>
> Shouldn't this function be static?
Yep.
>> +{
>> + if (__kernel_helper_version < 5)
>> + {
>> + const char err[] = "A newer kernel is required to run this binary.
>> (__kernel_cmpxchg64 helper)\n";
>> + /* At this point we need a way to crash with some information
>> + for the user - I'm not sure I can rely on much else being
>> + available at this point, so do the same as generic-morestack.c
>> + write() and abort(). */
>> + write (2 /* stderr */, err, sizeof(err));
>
> "write" is in the user's namespace in ISO C so it's not ideal to have a
> call to it. If there isn't a reserved-namespace version, using the
> syscall directly (hardcoding the syscall number) might be better.
OK, fair enough.
>> +void (*__sync8_kernelhelper_inithook[]) (void) __attribute__ ((section
>> (".init_array"))) = {
>> + &__check_for_sync8_kernelhelper
>> +};
>
> Shouldn't this also be static (marked "used" if needed)? Though I'd have
> thought simply marking the function as a constructor would be simpler and
> better....
OK, can do - I wasn't too sure if constructor would end up later in
the initialisation - I was worrying whether that might end up after a
C++ constructor that might actually use; (although I'm not actually
sure if that's more or less likely to happen with constructor v
init_array).
Dave
