Re: lib/csu cleanup - review please

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 [email protected]
with "unsubscribe freebsd-audit" in the body of the message