Re: lib/csu cleanup - review please

On Tue, 25 Jun 2002, David O'Brien wrote:

> On Fri, Jun 21, 2002 at 04:58:06PM +0100, Mark Murray wrote:
> > Index: alpha/crt1.c
> > ===================================================================
> > RCS file: /home/ncvs/src/lib/csu/alpha/crt1.c,v
> ...
> > +#ifndef lint
> > +
> >  #ifndef __GNUC__
> >  #error "GCC is needed to compile this file"
> >  #endif
>
> Please change Lint to note the #error but to continue processing.  I've
> pondered it and cannot find a good reason for Lint to "obey" #error.

The good reason is that #error is supposed to break compilation.  A gcc-
ware version of lint is required to compile this file, but "lint -g" is
not gcc-aware enough to do so.  Hiding the gcc-specific bits so that
lint can handle the file is less than useful.

> > +#ifndef __alpha__
> > +#error "This file only supports the alpha architecture"
> > +#endif
>
> Why do we need this?  It is the case that /sys/<ARCH> only supports
> <ARCH>.  We don't add this type of guard there.  Why do we need it here?
> Oh, I see it is in i386-elf.  I see no reason for it to be there and
> feel it should just be removed.

I agree.

> > -#pragma weak _DYNAMIC
> > -extern int _DYNAMIC;
> ..snip..
> > @@ -60,6 +68,9 @@
> >  extern int etext;
> >  #endif
> >
> > +extern int _DYNAMIC;
> > +#pragma weak _DYNAMIC
>
> Why?  IMO #pragma's should come as early as possible.

The pragma should be together with the declaration, but _DYNAMIC sorts
before etext anyway.

>...
> > @@ -75,7 +86,7 @@
> > -	argc = * (long *) ap;
> > +	argc = *(long *)(void *)ap;
>
> Is this from a Lint run on i386 or something?  I just compiled crt1.c
> with WARNS=6 and with your _start prototype and another fix I'll commit
> now; I get a clean compile.

ap has type "char **", so casting it to "long *" should cause a
diagnostic.  Casting it via "void *" prevents the diagnostic.  The
behaviour is still undefined, but works in an MD way.  "long *" is for
bug-for-bug compatibility with the kernel.  arc has type int, but the
kernel uses suword(ptr, argc) to put it on the stack.

> > Index: i386-elf/crt1.c
> > ===================================================================
> > RCS file: /home/ncvs/src/lib/csu/i386-elf/crt1.c,v
> > ...
> > +__inline static fptr
> > +get_rtld_cleanup(void)
> > +{
>
> csu/i386-elf/crt1.c:53: warning: function declaration isn't a prototype
> ;-)  I'll post a diff that is WARNS=6 clean.

Also, unusual order of "static __inline".

> I'd rather not guess a "correct" implimention for non-GCC compilers.
>
> > -_start(char *arguments, ...)
> > +_start(char *ap, ...)
>
> Why do we need to rename "arguments"?

Presumably for consistency with the other crt1.c's.  But ap is not a
very good name, sepecially here -- it often means the varargs pointer,
but that is not what it (starts as) here.  The code uses a hand rolled
version of va_arg() to handle "...".  That probably wouldn't work on
more complicated arches.  The corresponding alpha code doesn't even
use "...".  That is more correct, since we don't have a normal varargs
setup (which might have some args in registers) -- we have an array
of "char **"'s which has been initialized using suword() to write a
long into each char ** (this all assumes that longs can hold "char
**"'s, and some other things.  I'm surprised it works for i386's with
64-bit longs).

Bruce


To Unsubscribe: send mail to [email protected]
with "unsubscribe freebsd-audit" in the body of the message