Date: Tue, 25 Jun 2002 11:39:29 -0700 From: "David O'Brien" <obrien@freebsd.org> To: Mark Murray <mark@grondar.za> Cc: audit@freebsd.org Subject: Re: lib/csu cleanup - review please Message-ID: <20020625113929.A1424@dragon.nuxi.com> In-Reply-To: <200206231830.g5NIUAa4045990@grimreaper.grondar.org>; from mark@grondar.za on Fri, Jun 21, 2002 at 04:58:06PM %2B0100 References: <200206231830.g5NIUAa4045990@grimreaper.grondar.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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/<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.
> -#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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020625113929.A1424>