This is the mail archive of the cygwin-apps mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch/rebase] Add a rebase database to keep track of DLL addresses


Hi Chuck,

On Jul 22 20:26, Charles Wilson wrote:
> On 7/21/2011 3:38 PM, Charles Wilson wrote:
> >> IMHO it would make sense to bump LIB_VERSION and create a new rebase
> >> package ASAP, so we can get some more people to test it.
> > 
> > Give me a day or so to do the mingw/msys gloss, before updating
> > LIB_VERSION and cutting a new release, ok?
> 
> The attached allows to build on msys, as well as cygwin & mingw, and
> appears to work properly on all three platforms.

There's definitely a bug in the Mingw code, though.  I reviewed the
patch, comments inline.

> Also, I added a quick-n-dirty rebase-dump application. It actually
> helped me track down a problem on mingw: the db was not being opened in
> binary mode, and for some reason that prevented rebase (and rebase-dump)
> from being able to load the strings (it loaded the other bits of the db
> fine!)
> 
> I don't know why, but when I added O_BINARY everything was copacetic.

LF->CRLF conversion?

> When I say quick-n-dirty, I mean: lots of duplicated and only slightly
> modified code from rebase.c.  There's room for code consolidation, so
> this bit could be put off until later.

Right.  Especially all the db file information should go into a new
header file.

> Index: Makefile.in
> ===================================================================
> RCS file: /cvs/cygwin-apps/rebase/Makefile.in,v
> retrieving revision 1.6
> diff -u -p -r1.6 Makefile.in
> --- Makefile.in	21 Jul 2011 19:10:04 -0000	1.6
> +++ Makefile.in	22 Jul 2011 23:59:08 -0000
> @@ -58,9 +58,10 @@ ASH = @ASH@
>  DEFAULT_INCLUDES = -I. -I$(srcdir) -I$(srcdir)/imagehelper
>  DEFS = @DEFS@ -DVERSION='"$(PACKAGE_VERSION)"' -DLIB_VERSION='"$(LIB_VERSION)"' -DSYSCONFDIR='"$(sysconfdir)"'
>  
> -override CFLAGS+=-Wall -Werror
> -override CXXFLAGS+=-Wall -Werror
> -override LDFLAGS+=-static -static-libgcc
> +override CFLAGS+=-Wall -Werror @EXTRA_CFLAG_OVERRIDES@
> +override CXXFLAGS+=-Wall -Werror @EXTRA_CFLAG_OVERRIDES@
> +override LDFLAGS+=-static @EXTRA_LDFLAG_OVERRIDES@
> +override CXX_LDFLAGS+=@EXTRA_CXX_LDFLAG_OVERRIDES@

Why is CXX_LDFLAGS necessary?  I see what you do but I can't imagine the
msys compiler doesn't know -static-libstdc++.

>  .SUFFIXES:
>  .SUFFIXES: .c .cc .$(O)
> @@ -76,6 +77,9 @@ LIBIMAGEHELPER = imagehelper/libimagehel
>  REBASE_OBJS = rebase.$(O) $(LIBOBJS)
>  REBASE_LIBS = $(LIBIMAGEHELPER)
>  
> +REBASE_DUMP_OBJS = rebase-dump.$(O) $(LIBOBJS)
> +REBASE_DUMP_LIBS =

I'll ignore the rebase-dump stuff for now.

> Index: peflagsall.in
> ===================================================================
> RCS file: /cvs/cygwin-apps/rebase/peflagsall.in,v
> retrieving revision 1.1
> diff -u -p -r1.1 peflagsall.in
> --- peflagsall.in	20 Jun 2011 23:27:00 -0000	1.1
> +++ peflagsall.in	22 Jul 2011 23:59:08 -0000
> @@ -131,9 +131,39 @@ ArgDynBase=
>  # First see if caller requested help
>  check_args_for_help "$@"
>  
> +# Determine platform
> +Platform=`uname -s`
> +case $Platform in
> + *MINGW*  | *mingw*  ) Platform=mingw  ;;
> + *CYGWIN* | *cygwin* ) Platform=cygwin ;;
> + *MSYS*   | *msys*   ) Platform=msys   ;;
> + * )
> +    echo "Unsupported platform: $Platform" 1>&2
> +    exit 1
> +    ;;
> +esac
> +
>  # Verify only ash or dash processes are running
> -grep -E -q -i -v '/d?ash(.exe)?$' /proc/[0-9]*/exename
> -if [ $? -eq 0 -a -z "$RebaseDebug" ]
> +ProcessResult=0
> +case $Platform in
> +  mingw|msys )
> +    # Also exclude the commands we're using below, which is not ideal
> +    /bin/ps -s | /bin/sed -e '1d' | /bin/awk '{print $NF}' |\
> +      /bin/sed -e '/\/bin\/ps$/d'   -e '/\/bin\/ps\.exe$/d'  \
> +               -e '/\/bin\/sed$/d'  -e '/\/bin\/sed\.exe$/d' \
> +	       -e '/\/bin\/awk$/d'  -e '/\/bin\/awk\.exe$/d' \
> +	       -e '/\/bin\/sort$/d' -e '/\/bin\/sort\.exe$/d' \
> +	       -e '/\/bin\/uniq$/d' -e '/\/bin\/uniq\.exe$/d' \
> +               -e '/\/bin\/dash$/d' -e '/\/bin\/dash\.exe$/d' |\

What's the reason to do that?  I don't see how that should be necessary.
All the tools are used before peflags is called, so there's no problem
to change them as well, just as on Cygwin.

> Index: rebase.c
> ===================================================================
> RCS file: /cvs/cygwin-apps/rebase/rebase.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 rebase.c
> --- rebase.c	21 Jul 2011 19:10:04 -0000	1.6
> +++ rebase.c	22 Jul 2011 23:59:09 -0000
> @@ -50,6 +56,10 @@ FILE *file_list_fopen (const char *file_
>  char *file_list_fgets (char *buf, int size, FILE *file);
>  int file_list_fclose (FILE *file);
>  void version ();
> +#if defined(__MSYS__)
> +/* MSYS has no strtoull */
> +unsigned long long strtoull(const char *, char **, int);
> +#endif

Does it have strtoll?  It should have since the function is available
in Cygwin since October 2001, which means it was available in Cygwin
1.3.4 already.  Msys has been forked after that, afaics.

So, if we have strtoll, you could simply use that and cast the result to
uint64_t, rather than to paste some external strtoull implementation.

> @@ -160,11 +172,20 @@ main (int argc, char *argv[])
>    if (image_storage_flag)
>      {
>        if (load_image_info () < 0)
> -      	return 2;
> +	return 2;
>        img_info_rebase_start = img_info_size;
>      }
>  
> -#ifdef __CYGWIN__
> +#if defined(__MSYS__)
> +  if (machine == IMAGE_FILE_MACHINE_I386)
> +    {
> +      GetImageInfos64 ("/bin/msys-1.0.dll", NULL,
> +	               &cygwin_dll_image_base, &cygwin_dll_image_size);
> +      /* See cygwin code, below */
> +      cygwin_dll_image_base -= 3 * ALLOCATION_SLOT;
> +      cygwin_dll_image_size += 3 * ALLOCATION_SLOT + 8 * ALLOCATION_SLOT;

This is not correct for msys.  Msys has been forked off from Cygwin 1.3.
Back in 1.3 days, the shared memory areas were not allocated in front of
the DLL.  Rather, the default address for the shared memory areas was
0xa000000, bottom up.  I also doubt that it's really necessary to add the
slop factor to the end.

> @@ -308,7 +331,8 @@ save_image_info ()
>    hdr.offset = offset;
>    hdr.down_flag = down_flag;
>    hdr.count = img_info_size;
> -  if (write (fd, &hdr, sizeof hdr) < 0)
> +  errno = 0;
> +  if (write (fd, &hdr, sizeof (hdr)) < 0)

Why are you setting errno to 0, if it's only printed if write fails,
which sets errno anyway?

> @@ -480,7 +505,7 @@ load_image_info ()
>        for (i = 0; i < img_info_size; ++i)
>  	{
>  	  img_info_list[i].name = (char *)
> -				  malloc (img_info_list[i].name_size);
> +				  calloc (img_info_list[i].name_size, sizeof(char));

There's no reason to use calloc, it's overwritten by the subsequent
read() anyway.  This is differnt from the calloc for img_info_list,
which will only get partially filled by the read call.

> @@ -526,13 +551,15 @@ merge_image_info ()
>    for (i = img_info_rebase_start; i + 1 < img_info_size; ++i)
>      if ((img_info_list[i].name_size == img_info_list[i + 1].name_size
>  	 && !strcmp (img_info_list[i].name, img_info_list[i + 1].name))
> -#ifdef __CYGWIN__
> +#if defined(__MSYS__)
> +	|| !strcmp (img_info_list[i].name, "/usr/bin/msys-1.0.dll")
> +#elif defined(__CYGWIN__)
>  	|| !strcmp (img_info_list[i].name, "/usr/bin/cygwin1.dll")
>  #endif

What about defining the name of the DLL like this:

  #if defined (__MSYS__)
  #define CYGWIN_DLL "/usr/bin/msys-1.0.dll"
  #elif defined (__CYGWIN__)
  #define CYGWIN_DLL "/usr/bin/cygwin1.dll"
  #endif

and just use it subsequently, rather than adding more #if's?

> +#else
> +  {
> +    /* Borrow cygwin code for extracting module path, but use ANSI */
> +    char exepath[PATH_MAX];

PATH_MAX?  How big is that in native Win32?  If it's equivalent to
MAX_PATH, you don't have to worry about long path prefixes.

> +    char* p = NULL;
> +    char* p2 = NULL;
> +    size_t sz = 0;
> +
> +    if (!GetModuleFileNameA (NULL, exepath, PATH_MAX))
> +      fprintf (stderr, "%s: can't determine rebase installation path\n",
> +	       progname);
> +    p = exepath;
> +    if (strncmp (p, "\\\\?\\", 4))   /* No long path prefix. */
> +      {
> +        if (!strncasecmp (p, "\\\\", 2))     /* UNC */
> +          {
> +            p = strcpy (p, "\\??\\UN");

Hang on.  You're adding the native NT path prefix for DOS devices?
What's that supposed to accomplish, given that the subsequent code
uses msvcrt functions, which work with Win32 paths?

> +            GetModuleFileNameA (NULL, p, PATH_MAX - 6);
> +            *p = 'C';

That...


> +          }
> +        else
> +          {
> +            p = strcpy (p, "\\??\\");

Same NT path prefix weirdness.

> +            GetModuleFileNameA (NULL, p, PATH_MAX - 4);

...and that won't work.  You replaced calls to wcpcpy with calls to
strcpy.  Either use stpcpy, or add strlen(p) to p.

Fortunately the strcpy's are wrong, so the path is just what
GetModuleFileNameA returned, a plain Win32 path, which is what you need.
So, in fact you should just use the path returned by GetModuleFileNameA
and...

> +    /* strip off exename and trailing slash */

..yes, exactly.

> +#if defined(__MSYS__)
> +/* implementation adapted from google andriod bionic libc's

s/andriod/android

However, as mentioned above, I'd remove this code and just create a
small wrapper around the strtoll function.

> Index: rebaseall.in
> ===================================================================
> RCS file: /cvs/cygwin-apps/rebase/rebaseall.in,v
> retrieving revision 1.4
> diff -u -p -r1.4 rebaseall.in
> --- rebaseall.in	21 Jul 2011 19:10:04 -0000	1.4
> +++ rebaseall.in	22 Jul 2011 23:59:09 -0000
> @@ -64,9 +64,39 @@ case `uname -m` in
>  	;;
>  esac
>  
> +# Determine platform
> +Platform=`uname -s`
> +case $Platform in
> + *MINGW*  | *mingw*  ) Platform=mingw  ;;
> + *CYGWIN* | *cygwin* ) Platform=cygwin ;;
> + *MSYS*   | *msys*   ) Platform=msys   ;;
> + * )
> +    echo "Unsupported platform: $Platform" 1>&2
> +    exit 1
> +    ;;
> +esac
> +
>  # Verify only ash or dash processes are running
> -grep -E -q -i -v '/d?ash(.exe)?$' /proc/[0-9]*/exename
> -if [ $? -eq 0 -a -z "${RebaseDebug}" ]
> +ProcessResult=0
> +case $Platform in
> +  mingw|msys )
> +    # Also exclude the commands we're using below, which is not ideal
> +    /bin/ps -s | /bin/sed -e '1d' | /bin/awk '{print $NF}' |\
> +      /bin/sed -e '/\/bin\/ps$/d'   -e '/\/bin\/ps\.exe$/d'  \
> +               -e '/\/bin\/sed$/d'  -e '/\/bin\/sed\.exe$/d' \
> +               -e '/\/bin\/awk$/d'  -e '/\/bin\/awk\.exe$/d' \
> +               -e '/\/bin\/sort$/d' -e '/\/bin\/sort\.exe$/d' \
> +               -e '/\/bin\/uniq$/d' -e '/\/bin\/uniq\.exe$/d' \
> +               -e '/\/bin\/dash$/d' -e '/\/bin\/dash\.exe$/d' |\
> +               sort | uniq | grep -E '.'

Same as in peflagsall.in, I don't see why this should be necessary.


Corinna

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]