Re: [PATCH 64bit] utils: port dumper to 64bit

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

On Feb 15 02:02, Yaakov wrote:
> I just uploaded cygwin64-libiconv, cygwin64-gettext, and
> cygwin64-libbfd to Ports, so that dumper.exe could be built.  It
> appears it hasn't been ported yet, so here's a first attempt.  Comments
> welcome.

Looks good, I just have a few style nits.

> Index: dumper.cc
> ===================================================================
> RCS file: /cvs/src/src/winsup/utils/dumper.cc,v
> retrieving revision 1.20.2.1
> diff -u -p -r1.20.2.1 dumper.cc
> --- dumper.cc	23 Nov 2012 15:14:40 -0000	1.20.2.1
> +++ dumper.cc	15 Feb 2013 07:53:24 -0000
> @@ -1,6 +1,6 @@
>  /* dumper.cc
>  
> -   Copyright 1999, 2001, 2002, 2004, 2006, 2007, 2011 Red Hat Inc.
> +   Copyright 1999, 2001, 2002, 2004, 2006, 2007, 2011, 2013 Red Hat Inc.
>  
>     Written by Egor Duda <deo@logos-m.ru>
>  
> @@ -84,7 +84,8 @@ dumper::dumper (DWORD pid, DWORD tid, co
>  			  pid);
>    if (!hProcess)
>      {
> -      fprintf (stderr, "Failed to open process #%lu, error %ld\n", pid, GetLastError ());
> +      fprintf (stderr, "Failed to open process #%lu, error %ld\n",
> +	       (unsigned long) pid, (long) GetLastError ());

In other cases I cast to unsigned int and used %u.  The reason being
that this matches the natural size of pid_t on both platforms.  I'd
prefer to that here as well.

>        return;
>      }
>  
> @@ -192,7 +193,7 @@ dumper::add_thread (DWORD tid, HANDLE hT
>  }
>  
>  int
> -dumper::add_mem_region (LPBYTE base, DWORD size)
> +dumper::add_mem_region (LPBYTE base, SIZE_T size)
>  {
>    if (!sane ())
>      return 0;
> @@ -209,14 +210,15 @@ dumper::add_mem_region (LPBYTE base, DWO
>    new_entity->u.memory.base = base;
>    new_entity->u.memory.size = size;
>  
> -  deb_printf ("added memory region %08x-%08x\n", (DWORD) base, (DWORD) base + size);
> +  deb_printf ("added memory region %0*zx-%0*zx\n", 2 * __SIZEOF_SIZE_T__,
> +	      (SIZE_T) base, 2 * __SIZEOF_SIZE_T__, (SIZE_T) base + size);

Instead of casting to SIZE_T, I'd suggest to use %p for pointers
throughout.  And maybe we should simply drop the length entirely.
16 digits, of which the upper 5 are always 0, are hard to read.

>  int
> -dumper::split_add_mem_region (LPBYTE base, DWORD size)
> +dumper::split_add_mem_region (LPBYTE base, SIZE_T size)
>  {
>    if (!sane ())
>      return 0;
> @@ -255,7 +257,7 @@ dumper::add_module (LPVOID base_address)
>    if (!sane ())
>      return 0;
>  
> -  char *module_name = psapi_get_module_name (hProcess, (DWORD) base_address);
> +  char *module_name = psapi_get_module_name (hProcess, (SIZE_T) base_address);

Shouldn't psapi_get_module_name take an LPVOID instead?  That would
also drop the need for the cast in psapi_get_module_name, like so:

> Index: module_info.cc
> [...]
> @@ -33,7 +33,7 @@ static tf_GetModuleFileNameExA *psapi_Ge
>     Uses psapi.dll. */
>  
>  char *
> -psapi_get_module_name (HANDLE hProcess, DWORD BaseAddress)
> +psapi_get_module_name (HANDLE hProcess, SIZE_T BaseAddress)

  +psapi_get_module_name (HANDLE hProcess, LPVOID BaseAddress)

>  {
>    DWORD len;
>    MODULEINFO mi;
> @@ -103,7 +103,7 @@ psapi_get_module_name (HANDLE hProcess, 
>  	  goto failed;
>  	}
>  
> -      if ((DWORD) (mi.lpBaseOfDll) == BaseAddress)
> +      if ((SIZE_T) (mi.lpBaseOfDll) == BaseAddress)

  +      if (mi.lpBaseOfDll == BaseAddress)


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat