From owner-freebsd-audit Tue Jun 25 11:41: 2 2002 Delivered-To: freebsd-audit@freebsd.org Received: from dragon.nuxi.com (trang.nuxi.com [66.92.13.169]) by hub.freebsd.org (Postfix) with ESMTP id BE16B37B41B for ; Tue, 25 Jun 2002 11:39:38 -0700 (PDT) Received: from dragon.nuxi.com (obrien@localhost [127.0.0.1]) by dragon.nuxi.com (8.12.4/8.12.2) with ESMTP id g5PIdbtL002584; Tue, 25 Jun 2002 11:39:37 -0700 (PDT) (envelope-from obrien@dragon.nuxi.com) Received: (from obrien@localhost) by dragon.nuxi.com (8.12.4/8.12.4/Submit) id g5PIdTMj002576; Tue, 25 Jun 2002 11:39:29 -0700 (PDT) Date: Tue, 25 Jun 2002 11:39:29 -0700 From: "David O'Brien" To: Mark Murray Cc: audit@freebsd.org Subject: Re: lib/csu cleanup - review please Message-ID: <20020625113929.A1424@dragon.nuxi.com> Reply-To: obrien@freebsd.org Mail-Followup-To: David O'Brien , Mark Murray , audit@freebsd.org References: <200206231830.g5NIUAa4045990@grimreaper.grondar.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: <200206231830.g5NIUAa4045990@grimreaper.grondar.org>; from mark@grondar.za on Fri, Jun 21, 2002 at 04:58:06PM +0100 X-Operating-System: FreeBSD 5.0-CURRENT Organization: The NUXI BSD group X-Pgp-Rsa-Fingerprint: B7 4D 3E E9 11 39 5F A3 90 76 5D 69 58 D9 98 7A X-Pgp-Rsa-Keyid: 1024/34F9F9D5 Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG On Fri, Jun 21, 2002 at 04:58:06PM +0100, Mark Murray wrote: > Hi John and other folks > > Please check out the following diffs to lib/csu/*/crt1.c. > > I've been carrying a lot of this for nearly a year now with > no problems at all on my laptop or SMP servers. > > There are two separate reasons for the cleanup: WARNS/lint > fixes, and diff-reduction between all of the crt1.c's. > > For the i386-elf version, there are a lot of whitespace diffs > that will be committed separately. > > Also for the i386-elf one, a macro containing GCC-specific > __asm() code has been turned into an inline function. This > makes for neater (IMO) code, and makes it possible to lint. > > Comments? Flames? Awards? :-) > > M > -- > o Mark Murray > \_ > O.\_ Warning: this .sig is umop ap!sdn > 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. > +#ifndef __alpha__ > +#error "This file only supports the alpha architecture" > +#endif Why do we need this? It is the case that /sys/ only supports . 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. > -#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. > +void _start(char **, void (*)(void), struct Struct_Obj_Entry *, > + struct ps_strings *); Needed addition. > @@ -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. > Index: i386-elf/crt1.c > =================================================================== > RCS file: /home/ncvs/src/lib/csu/i386-elf/crt1.c,v > -extern void _fini(void); > extern void _init(void); > +extern void _fini(void); Why? `f' comes before `i'. > extern int main(int, char **, char **); > +void _start(char *, ...); Needed. > #ifdef GCRT > extern void _mcleanup(void); > @@ -48,51 +57,56 @@ > extern int _DYNAMIC; > #pragma weak _DYNAMIC > > -#ifdef __i386__ > -#define get_rtld_cleanup() \ > - ({ fptr __value; \ > - __asm__("movl %%edx,%0" : "=rm"(__value)); \ > - __value; }) > -#else > -#error "This file only supports the i386 architecture" > -#endif > - > char **environ; > const char *__progname = ""; > > +__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. 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"? > { > - fptr rtld_cleanup; > - int argc; > - char **argv; > - char **env; > - const char *s; > + fptr rtld_cleanup; > + int argc; > + char **argv; > + char **env; > + const char *s; Lets seperate the style changes from the "functionality" or warnings ones. While it would be nice for this to be style(9); style(9) says to leave the format as-is when consistent. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message