Re: Preview: portable dumper

[Eli Zaretskii]

> From: Daniel Colascione <address@hidden>
> Date: Fri, 16 Feb 2018 13:25:00 -0800
> 
> >> pdumper.c: In function 'dump_read_all':
> >> pdumper.c:4723:45: error: conversion to 'unsigned int' from 'size_t {aka 
> >> long long unsigned int}' may alter its value [-Werror=conversion]
> >>           read (fd, (char*) buf + bytes_read, bytes_to_read - bytes_read);
> >>                                               ^~~~~~~~~~~~~
> >>    CC       data.o
> >> cc1.exe: some warnings being treated as errors
> > 
> > I have seen you have commit many fixes but here the build still fails in 
> > the same manner.. Maybe you have not found the right solution yet..
> 
> On this one, I was waiting to see how the broader discussion went.

OK, I've looked at the code, both in sysdep.c and in pdumper.c.  My
conclusion is that there's no real problem in sysdep.c, due to these
comments:

  /* Maximum number of bytes to read or write in a single system call.
     This works around a serious bug in Linux kernels before 2.6.16; see
     <https://bugzilla.redhat.com/show_bug.cgi?format=multiple&id=612839>.
     It's likely to work around similar bugs in other operating systems, so do 
it
     on all platforms.  Round INT_MAX down to a page size, with the conservative
     assumption that page sizes are at most 2**18 bytes (any kernel with a
     page size larger than that shouldn't have the bug).  */
  #ifndef MAX_RW_COUNT
  #define MAX_RW_COUNT (INT_MAX >> 18 << 18)
  #endif

  /* Read from FD to a buffer BUF with size NBYTE.
     If interrupted, process any quits and pending signals immediately
     if INTERRUPTIBLE, and then retry the read unless quitting.
     Return the number of bytes read, which might be less than NBYTE.
     On error, set errno to a value other than EINTR, and return -1.  */
  static ptrdiff_t
  emacs_intr_read (int fd, void *buf, ptrdiff_t nbyte, bool interruptible)
  {
    ssize_t result;

    /* There is no need to check against MAX_RW_COUNT, since no caller ever
       passes a size that large to emacs_read.  */
    do
      {
        if (interruptible)
          maybe_quit ();
        result = read (fd, buf, nbyte);
      }
    while (result < 0 && errno == EINTR);

Since MAX_RW_COUNT is less than INT_MAX, we have no real problem here
for MS-Windows, as 'nbyte' will never overflow an unsigned int.  We
could add an eassert there, for Windows only, though, to make this
assumption explicit.

As for pdumper, we could do one of 2 things:

 . make a reasonable assumption that no .pdmp file will ever be larger
   than 2GB, change the assertion there which checks against SSIZE_MAX
   to check against UINT_MAX instead, and work internally with
   unsigned int instead of size_t counts; or

 . write a separate Windows-specific version of dump_read_all, which
   in the 64-bit build would limit to UINT_MAX the number of bytes we
   read on each iteration through the loop.

Daniel, which of these 2 alternatives do you prefer?  Or does anyone
have a better proposal?  (I already considered rewriting the code in
w32.c:sys_read so that it accepts a size_t last argument, and decided
it was unjustified for this single caller, as all the other direct
calls to 'read' in Emacs either use fixed small byte counts, or are
not compiled in the Windows build.)

Thanks.