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>