Date: Wed, 26 Jun 2002 17:36:10 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: "David O'Brien" <obrien@FreeBSD.ORG> Cc: Mark Murray <mark@grondar.za>, <audit@FreeBSD.ORG> Subject: Re: lib/csu cleanup - review please Message-ID: <20020626165726.O30754-100000@gamplex.bde.org> In-Reply-To: <20020625113929.A1424@dragon.nuxi.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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 majordomo@FreeBSD.org
with "unsubscribe freebsd-audit" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020626165726.O30754-100000>