Re: [PATCH] memset.S for PowerPC

[Geoff Keating <geoffk at geoffk dot org>]

> Date: Wed, 21 Aug 2002 16:16:36 -0500
> From: Steven Munroe <sjmunroe@vnet.ibm.com>

> Here is the revised memset patch for PowerPC (32-bit).  if this meets 
> your approval I'll start revising my PowerPC64 patches into this form.

This is pretty good, and a big improvement from before.  I do have a
few minor comments:

> 2002-08-20  Steven Munroe  <sjmunroe@us.ibm.com>
> 	* sysdeps/powerpc/elf/libc-start.c : Scan Aux Vector for 
> 	AT_DCACHEBSIZE and copy value to __cache_line_size.
> 	* sysdeps/unix/sysv/linux/powerpc/dl-sysdep.c : Scan Aux Vector for 
> 	AT_DCACHEBSIZE and copy value to __cache_line_size.
> 	* sysdeps/powerpc/memset.S : Define __cache_line_size and use its
> 	value to select the correct stride for dcbz.
> 	
> >>>>>>>>
> diff -rc2P glibc-2.2.5/sysdeps/powerpc/elf/libc-start.c glibc-2.2.5-memset/sysdeps/powerpc/elf/libc-start.c

I'll rewrite this for you.  It should look like:

2002-08-20  Steven Munroe  <sjmunroe@us.ibm.com>

	* sysdeps/powerpc/elf/libc-start.c 
	(__cache_line_size): Declare.
	(__aux_init_cache): New.
	(__libc_start_main): Change type of `auxvec' parameter to
	`ElfW(auxv_t) *'.  Correct walking of aux vector.  Call
	__aux_init_cache.
	* sysdeps/unix/sysv/linux/powerpc/dl-sysdep.c 
	(__cache_line_size): Declare.
	(__aux_init_cache): New.
	(DL_PLATFORM_INIT): Define.
	* sysdeps/powerpc/memset.S: Define __cache_line_size and use its
	value to select the correct stride for dcbz.
	
Note that:
- This has a blank line between the first line and the actual entries.
- It mentions new procedures, but it just says "new", not what they
  do.  If someone wants to see what the new procedure does, they can
  go look at the comment above it.
- There are no spaces before ':'.
- The CVS conflict marker is gone :-).
- The entries are broken down by routine name.  That way, if anyone is
  looking to see when '__aux_init_cache' appeared, they can
  use grep or similar on the ChangeLog.

In these two code chunks (in sysdeps/powerpc/elf/libc-start.c
and sysdeps/unix/sysv/linux/powerpc/dl-sysdep.c):

      case AT_DCACHEBSIZE:
        {
	        int *cls = & __cache_line_size;
          if (cls != NULL)
            *cls = av->a_un.a_val;
		    }
        break;
      }

Do you see that the first '}' is misaligned?

        case AT_DCACHEBSIZE:
      	  {
			      int *cls = & __cache_line_size;
            if (cls != NULL)
              *cls = av->a_un.a_val;
          }
		    break;
      }

Do you see that the lines aren't aligned properly?

I think it's likely that your editor is set to treat tabs in C source
files as equivalent to two spaces.  In GNU code, tabs always mean to
move to the next 8-character boundary.  When I run the last chunk of
code through 'cat -t', I get:

        case AT_DCACHEBSIZE:
      ^I  {
^I^I^I      int *cls = & __cache_line_size;
            if (cls != NULL)
              *cls = av->a_un.a_val;
          }
^I^I    break;
      }

If you can't fix your editor, it's acceptable to just use spaces
instead of tabs.

Anyway, if you correct these problems, the patch is OK, please commit
it then.

-- 
- Geoffrey Keating <geoffk@geoffk.org> <geoffk@redhat.com>