Problem with library images on Windows (again)

Problem with library images on Windows (again)

[Juanma Barranquero]

For some time now, TIFF images do crash optimized MSVC builds of Emacs
when using the MinGW-supplied TIFF library.

The crash happens in image_spec_value(), but it all boils down to the
fact that on lookup_image(), after

         img->load_failed_p = img->type->load (f, img) == 0;

the value of the argument "spec" cannot be trusted. This is easy to prove; with

   static LispObject spec_backup;
   spec_backup = spec;

   img->load_failed_p = img->type->load (f, img) == 0;

   spec = spec_backup;
   spec_backup = Qnil;

everything works as expected.

Now, when this happened before (with the local variable img) someone
said that the MinGW libraries were not respecting calling conventions,
but I'm not sure. Calling conventions for MSVC say that you cannot
trust EAX, EBX, ECX, EDX, ESI, or EDI registers after a subroutine
call, and AFAICS, the problem is that on optimized builds VC is
expecting spec to still be cached in a register.

So, I can apply the attached patch and at least stop Emacs from
crashing on Windows, but I certainly would appreciate a more
insightful analysis from the "people in the know" wrt image support
and/or Windows.

-- 
                    /L/e/k/t/u

Index: src/image.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/image.c,v
retrieving revision 1.23
diff -u -2 -r1.23 image.c
--- src/image.c	10 May 2005 09:16:46 -0000	1.23
+++ src/image.c	18 May 2005 15:38:22 -0000
@@ -1663,9 +1663,20 @@
       extern Lisp_Object Qpostscript;
 
+#ifdef _MSC_VER
+      static Lisp_Object spec_backup;
+      spec_backup = spec;
+#endif
+
       BLOCK_INPUT;
       img = make_image (spec, hash);
       cache_image (f, img);
+
       img->load_failed_p = img->type->load (f, img) == 0;
 
+#ifdef _MSC_VER
+      spec = spec_backup;
+      spec_backup = Qnil;
+#endif
+
       /* If we can't load the image, and we don't have a width and
 	 height, use some arbitrary width and height so that we can

Re: Problem with library images on Windows (again)

[Andreas Schwab]

Juanma Barranquero <lekktu@gmail.com> writes:

> Now, when this happened before (with the local variable img) someone
> said that the MinGW libraries were not respecting calling conventions,
> but I'm not sure. Calling conventions for MSVC say that you cannot
> trust EAX, EBX, ECX, EDX, ESI, or EDI registers after a subroutine

That's quite different from the Unix calling conventions on i386, where
only EAX, ECX and EDX are call-clobbered.

> call, and AFAICS, the problem is that on optimized builds VC is
> expecting spec to still be cached in a register.

That appears like a blatant bug in the compiler if it cannot properly
track call-clobbered registers.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Problem with library images on Windows (again)

[Juanma Barranquero]

> That's quite different from the Unix calling conventions on i386, where
> only EAX, ECX and EDX are call-clobbered.

I was wrong wrt the MSVC calling conventions. MSDN docs say:

   All arguments are widened to 32 bits when they are passed.
   Return values are also widened to 32 bits and returned in the
   EAX register, except for 8-byte structures, which are returned
   in the EDX:EAX register pair. Larger structures are returned
   in the EAX register as pointers to hidden return structures.
   Parameters are pushed onto the stack from right to left.

   The compiler generates prolog and epilog code to save and
   restore the ESI, EDI, EBX, and EBP registers, if they are used
   in the function.

so it seems it's the same calling convention than on i386 Unixes: EAX
and EDX can be clobbered to return the function's result, and ECX
because it is not saved.

> That appears like a blatant bug in the compiler if it cannot properly
> track call-clobbered registers.

There's a bug somewhere, that's pretty clear. Whether that's an error
of the MSVC compiler or the MinGW libraries, I'm not sure yet. I've
been tracing lookup_image, and the fact is, the lookup_image code is
using EBX to cache "spec", and rightly so. But the call to tiff_load
(via img->type->load()) clobbers EBX!

-- 
                    /L/e/k/t/u

Re: Problem with library images on Windows (again)

[Jason Rumney]

Juanma Barranquero <lekktu@gmail.com> writes:

>> That appears like a blatant bug in the compiler if it cannot properly
>> track call-clobbered registers.
>
> There's a bug somewhere, that's pretty clear. Whether that's an error
> of the MSVC compiler or the MinGW libraries, I'm not sure yet.

FWIW the problem with the jpeg library was that the headers defined
some constant like BOOL or BYTE differently than the MSVC headers,
so it ended up a different size.

Re: Problem with library images on Windows (again)

[Jason Rumney]

Juanma Barranquero <lekktu@gmail.com> writes:

>
> So, I can apply the attached patch and at least stop Emacs from
> crashing on Windows, but I certainly would appreciate a more
> insightful analysis from the "people in the know" wrt image support
> and/or Windows.

Better to disable image support for MSVC builds than to try to hack
things to work by introducing redundant variables to save the values
of other variables in. Such hacks might be worth it to support free
software where we can see whether the hack really does fix the
problem, but not for proprietary blackboxes.

Re: Problem with library images on Windows (again)

[Juanma Barranquero]

> Better to disable image support for MSVC builds than to try to hack
> things to work by introducing redundant variables to save the values
> of other variables in.

Perhaps; note that I didn't say it wasn't a hack. OTOH, I normally use
a MSVC build and certainly don't appreciate Emacs crashing on me just
because I accidentally opened a TIFF file. But disabling all image
support seems a bit extreme.

Anyway: tiff_load is pushing EBX into the stack and getting a
different EBX when poping it out at the end of the function! I've not
yet traced it to determine what or when it's changing the stack, or
whether it is the MSVC-generated tiff_load code or the MinGW-built
TIFF libraries that are called on several spots of tiff_load, but in
any case something's afoot (as Sherlock Holmes would say).

-- 
                    /L/e/k/t/u

Re: Problem with library images on Windows (again)

[Eli Zaretskii]

> Date: Thu, 19 May 2005 00:52:56 +0200
> From: Juanma Barranquero <lekktu@gmail.com>
> 
> Anyway: tiff_load is pushing EBX into the stack and getting a
> different EBX when poping it out at the end of the function!

Can you set a watchpoint on the address where EBX is saved on the
stack, and then see what code clobbers that value?

Re: Problem with library images on Windows (again)

[Juanma Barranquero]

On 5/19/05, Eli Zaretskii <eliz@gnu.org> wrote:

> Can you set a watchpoint on the address where EBX is saved on the
> stack, and then see what code clobbers that value?

Yes. tiff_load pushes EBX into the stack, and then goes around happily
pushing arguments and calling functions from the TIFF library, which
apparently don't remove their arguments from the stack (nor does it
the tiff_load code). So the stack keeps growing and, by the time
tiff_load does the POP EBX, it gets random garbage. On returning from
tiff_load the frame pointer is restored, so the spurious stack data is
just discarded.

So it seems like a mismatch between the calling convention used to
implement the TIFF library and the one MSVC thinks the TIFF functions
are using.

I'll dig around a little more. But any help is much appreciated.

-- 
                    /L/e/k/t/u

Re: Problem with library images on Windows (again)

[Eli Zaretskii]

> Date: Thu, 19 May 2005 10:13:23 +0200
> From: Juanma Barranquero <lekktu@gmail.com>
> 
> tiff_load pushes EBX into the stack, and then goes around happily
> pushing arguments and calling functions from the TIFF library, which
> apparently don't remove their arguments from the stack (nor does it
> the tiff_load code). So the stack keeps growing

This sounds incredible.  MSVC generally produces good code, so I find
it hard to believe that it violates the C ABI, whereby the caller
should pop arguments off the stack.  Are you sure this is indeed what
happens?  Can you show some evidence, like disassembly of code around
the call to tiff_load, which would show how arguments are pushed onto
the stack and never popped off it?

Re: Problem with library images on Windows (again)

[Juanma Barranquero]

> This sounds incredible.  MSVC generally produces good code, so I find
> it hard to believe that it violates the C ABI, whereby the caller
> should pop arguments off the stack.  Are you sure this is indeed what
> happens?

Yes. If, immediately before the POP EBX, I manually restore the stack
to the value it had after the PUSH EBX at the start of the routine,
Emacs does not crash, and indeed it displays the TIFF file.

But I don't think is an error in MSVC. I think it's some mixup between
"pascal" and "C" style calls (as you know, the former remove their
arguments, the latter don't). MSVC must be thinking that the external
TIFF functions do remove their arguments, so it is not doing it.
Sounds like an issue with the external declarations on the header
files.

>  Can you show some evidence, like disassembly of code around
> the call to tiff_load, which would show how arguments are pushed onto
> the stack and never popped off it?

There goes the full tiff_load code, as shown on the Visual Studio
disassembler view. You can see by yourself that, when calling Emacs
functions, MSVC uses "ADD SP, nn" to fix the stack, but it doesn't do
it after the indirect calls to fn_TIFF* functions.

-- 
                    /L/e/k/t/u

----------------------------------------------------------------------------------------

/* Load TIFF image IMG for use on frame F.  Value is non-zero if
   successful.  */

static int
tiff_load (f, img)
     struct frame *f;
     struct image *img;
{
01074117  push        ebp  
01074118  mov         ebp,esp 
0107411A  sub         esp,24h 
0107411D  push        ebx  
  Lisp_Object file, specified_file;
  Lisp_Object specified_data;
  TIFF *tiff;
  int width, height, x, y;
  uint32 *buf;
  int rc;
  XImagePtr ximg;
  struct gcpro gcpro1;
  tiff_memory_source memsrc;

  specified_file = image_spec_value (img->spec, QCfile, NULL);
0107411E  mov         ebx,dword ptr [ebp+0Ch] 
01074121  mov         ecx,dword ptr [ebx+28h] 
01074124  push        esi  
01074125  push        edi  
01074126  xor         edi,edi 
01074128  push        edi  
01074129  push        dword ptr [_QCfile (1109E80h)] 
0107412F  call        image_spec_value (106FF15h) 
  specified_data = image_spec_value (img->spec, QCdata, NULL);
01074134  mov         ecx,dword ptr [ebx+28h] 
01074137  push        edi  
01074138  push        dword ptr [_QCdata (1109064h)] 
0107413E  mov         dword ptr [specified_file],eax 
01074141  call        image_spec_value (106FF15h) 
01074146  add         esp,10h 
  file = Qnil;
  GCPRO1 (file);

  fn_TIFFSetErrorHandler (tiff_error_handler);
01074149  push        offset tiff_error_handler (10723B8h) 
0107414E  mov         esi,eax 
01074150  call        dword ptr [_fn_TIFFSetErrorHandler (1138B64h)] 
  fn_TIFFSetWarningHandler (tiff_warning_handler);
01074156  push        offset tiff_warning_handler (1072400h) 
0107415B  call        dword ptr [_fn_TIFFSetWarningHandler (1138B78h)] 

  if (NILP (specified_data))
01074161  cmp         esi,dword ptr [_Qnil (1108D6Ch)] 
01074167  jne         tiff_load+0AEh (10741C5h) 
    {
      /* Read from a file */
      file = x_find_image_file (specified_file);
01074169  push        dword ptr [specified_file] 
0107416C  call        x_find_image_file (10706E5h) 
01074171  pop         ecx  
      if (!STRINGP (file))
01074172  mov         ecx,eax 
01074174  and         ecx,0E0000000h 
0107417A  cmp         ecx,60000000h 
01074180  mov         dword ptr [buf],eax 
01074183  je          tiff_load+81h (1074198h) 
	{
	  image_error ("Cannot find image file `%s'", specified_file, Qnil);
01074185  push        dword ptr [_Qnil (1108D6Ch)] 
0107418B  push        dword ptr [specified_file] 
0107418E  push        offset string "Cannot find image file `%s'" (1166718h) 
	  UNGCPRO;
	  return 0;
01074193  jmp         tiff_load+11Ah (1074231h) 
	}

      /* Try to open the image file.  */
      tiff = fn_TIFFOpen (SDATA (file), "r");
01074198  push        offset string "r" (115F1E0h) 
0107419D  and         eax,1FFFFFFFh 
010741A2  push        dword ptr [eax+0Ch] 
010741A5  call        dword ptr [_fn_TIFFOpen (1138AC0h)] 
010741AB  mov         esi,eax 
      if (tiff == NULL)
010741AD  cmp         esi,edi 
010741AF  jne         tiff_load+129h (1074240h) 
	{
	  image_error ("Cannot open `%s'", file, Qnil);
010741B5  push        dword ptr [_Qnil (1108D6Ch)] 
010741BB  push        dword ptr [buf] 
010741BE  push        offset string "Cannot open `%s'" (1166DA4h) 
	  UNGCPRO;
	  return 0;
010741C3  jmp         tiff_load+11Ah (1074231h) 
	}
    }
  else
    {
      /* Memory source! */
      memsrc.bytes = SDATA (specified_data);
010741C5  mov         eax,esi 
010741C7  and         eax,1FFFFFFFh 
010741CC  mov         ecx,dword ptr [eax+0Ch] 
010741CF  mov         dword ptr [memsrc],ecx 
      memsrc.len = SBYTES (specified_data);
010741D2  mov         ecx,dword ptr [eax+4] 
010741D5  cmp         ecx,edi 
010741D7  jge         tiff_load+0C9h (10741E0h) 
010741D9  mov         eax,dword ptr [eax] 
010741DB  mov         dword ptr [ebp-20h],eax 
010741DE  jmp         tiff_load+0CCh (10741E3h) 
010741E0  mov         dword ptr [ebp-20h],ecx 
      memsrc.index = 0;

      tiff = fn_TIFFClientOpen ("memory_source", "r", &memsrc,
                                (TIFFReadWriteProc) tiff_read_from_memory,
                                (TIFFReadWriteProc) tiff_write_from_memory,
                                tiff_seek_in_memory,
                                tiff_close_memory,
                                tiff_size_of_memory,
                                tiff_mmap_memory,
                                tiff_unmap_memory);
010741E3  push        offset tiff_unmap_memory (10723ACh) 
010741E8  push        offset tiff_mmap_memory (10723A9h) 
010741ED  push        offset tiff_size_of_memory (10723ADh) 
010741F2  push        offset tiff_close_memory (10723A6h) 
010741F7  push        offset tiff_seek_in_memory (107236Fh) 
010741FC  push        offset tiff_write_from_memory (107236Bh) 
01074201  push        offset tiff_read_from_memory (1072335h) 
01074206  lea         eax,[memsrc] 
01074209  push        eax  
0107420A  push        offset string "r" (115F1E0h) 
0107420F  push        offset string "memory_source" (1166DFCh) 
01074214  mov         dword ptr [ebp-1Ch],edi 
01074217  call        dword ptr [_fn_TIFFClientOpen (1138B00h)] 
0107421D  mov         esi,eax 

      if (!tiff)
0107421F  cmp         esi,edi 
01074221  jne         tiff_load+129h (1074240h) 
	{
	  image_error ("Cannot open memory source for `%s'", img->spec, Qnil);
01074223  push        dword ptr [_Qnil (1108D6Ch)] 
01074229  push        dword ptr [ebx+28h] 
0107422C  push        offset string "Cannot open memory source for
`%"... (1166DD8h)
01074231  call        add_to_log (1035C57h) 
01074236  add         esp,0Ch 
	  UNGCPRO;
	  return 0;
01074239  xor         eax,eax 
0107423B  jmp         tiff_load+272h (1074389h) 
	}
    }

  /* Get width and height of the image, and allocate a raster buffer
     of width x height 32-bit values.  */
  fn_TIFFGetField (tiff, TIFFTAG_IMAGEWIDTH, &width);
01074240  lea         eax,[width] 
01074243  push        eax  
01074244  push        100h 
01074249  push        esi  
0107424A  call        dword ptr [_fn_TIFFGetField (1138B2Ch)] 
  fn_TIFFGetField (tiff, TIFFTAG_IMAGELENGTH, &height);
01074250  lea         eax,[height] 
01074253  push        eax  
01074254  push        101h 
01074259  push        esi  
0107425A  call        dword ptr [_fn_TIFFGetField (1138B2Ch)] 
  buf = (uint32 *) xmalloc (width * height * sizeof *buf);
01074260  mov         eax,dword ptr [height] 
01074263  imul        eax,dword ptr [width] 
01074267  shl         eax,2 
0107426A  push        eax  
0107426B  call        xmalloc (10070AFh) 
01074270  pop         ecx  

  rc = fn_TIFFReadRGBAImage (tiff, width, height, buf, 0);
01074271  push        edi  
01074272  push        eax  
01074273  push        dword ptr [height] 
01074276  mov         dword ptr [buf],eax 
01074279  push        dword ptr [width] 
0107427C  push        esi  
0107427D  call        dword ptr [_fn_TIFFReadRGBAImage (1138B90h)] 
  fn_TIFFClose (tiff);
01074283  push        esi  
01074284  mov         dword ptr [specified_file],eax 
01074287  call        dword ptr [_fn_TIFFClose (1138AECh)] 
  if (!rc)
0107428D  cmp         dword ptr [specified_file],edi 
01074290  jne         tiff_load+198h (10742AFh) 
    {
      image_error ("Error reading TIFF image `%s'", img->spec, Qnil);
01074292  push        dword ptr [_Qnil (1108D6Ch)] 
01074298  push        dword ptr [ebx+28h] 
0107429B  push        offset string "Error reading TIFF image `%s'" (1166DB8h) 
010742A0  call        add_to_log (1035C57h) 
010742A5  add         esp,0Ch 
010742A8  xor         esi,esi 
010742AA  jmp         tiff_load+267h (107437Eh) 
      xfree (buf);
      UNGCPRO;
      return 0;
    }

  /* Create the X image and pixmap.  */
  if (!x_create_x_image_and_pixmap (f, width, height, 0, &ximg, &img->pixmap))
010742AF  lea         eax,[ebx+4] 
010742B2  push        eax  
010742B3  push        dword ptr [height] 
010742B6  lea         esi,[specified_file] 
010742B9  push        dword ptr [width] 
010742BC  xor         eax,eax 
010742BE  push        dword ptr [f] 
010742C1  call        x_create_x_image_and_pixmap (1072AE6h) 
010742C6  add         esp,10h 
010742C9  test        eax,eax 
010742CB  je          tiff_load+191h (10742A8h) 
    {
      xfree (buf);
      UNGCPRO;
      return 0;
    }

  /* Initialize the color table.  */
  init_color_table ();

  /* Process the pixel raster.  Origin is in the lower-left corner.  */
  for (y = 0; y < height; ++y)
010742CD  mov         dword ptr [y],edi 
010742D0  mov         edi,dword ptr [height] 
010742D3  test        edi,edi 
010742D5  jle         tiff_load+224h (107433Bh) 
    {
      uint32 *row = buf + y * width;
010742D7  mov         eax,dword ptr [y] 
010742DA  imul        eax,dword ptr [width] 
010742DE  mov         ecx,dword ptr [buf] 

      for (x = 0; x < width; ++x)
010742E1  xor         esi,esi 
010742E3  cmp         dword ptr [width],esi 
010742E6  lea         eax,[ecx+eax*4] 
010742E9  mov         dword ptr [row],eax 
010742EC  jle         tiff_load+21Ch (1074333h) 
	{
	  uint32 abgr = row[x];
010742EE  mov         eax,dword ptr [row] 
010742F1  mov         eax,dword ptr [eax+esi*4] 
	  int r = TIFFGetR (abgr) << 8;
	  int g = TIFFGetG (abgr) << 8;
	  int b = TIFFGetB (abgr) << 8;
	  XPutPixel (ximg, x, height - 1 - y, lookup_rgb_color (f, r, g, b));
010742F4  mov         ecx,eax 
010742F6  shr         ecx,10h 
010742F9  xor         edx,edx 
010742FB  mov         dh,cl 
010742FD  mov         ecx,eax 
010742FF  shr         ecx,8 
01074302  xor         ebx,ebx 
01074304  mov         bh,cl 
01074306  xor         ecx,ecx 
01074308  mov         ch,al 
0107430A  mov         eax,edx 
0107430C  push        ecx  
0107430D  mov         ecx,ebx 
0107430F  call        lookup_rgb_color (10718CDh) 
01074314  sub         edi,dword ptr [y] 
01074317  mov         ebx,eax 
01074319  dec         edi  
0107431A  push        edi  
0107431B  mov         edi,dword ptr [specified_file] 
0107431E  push        esi  
0107431F  call        XPutPixel (10719CBh) 
01074324  mov         edi,dword ptr [height] 
01074327  add         esp,0Ch 
0107432A  inc         esi  
0107432B  cmp         esi,dword ptr [width] 
0107432E  jl          tiff_load+1D7h (10742EEh) 

      for (x = 0; x < width; ++x)
01074330  mov         ebx,dword ptr [img] 
    {
      xfree (buf);
      UNGCPRO;
      return 0;
    }

  /* Initialize the color table.  */
  init_color_table ();

  /* Process the pixel raster.  Origin is in the lower-left corner.  */
  for (y = 0; y < height; ++y)
01074333  inc         dword ptr [y] 
01074336  cmp         dword ptr [y],edi 
01074339  jl          tiff_load+1C0h (10742D7h) 
	}
    }

#ifdef COLOR_TABLE_SUPPORT
  /* Remember the colors allocated for the image.  Free the color table.  */
  img->colors = colors_in_color_table (&img->ncolors);
  free_color_table ();
#endif /* COLOR_TABLE_SUPPORT */

  img->width = width;
0107433B  mov         eax,dword ptr [width] 
  img->height = height;

  /* Maybe fill in the background field while we have ximg handy. */
  if (NILP (image_spec_value (img->spec, QCbackground, NULL)))
0107433E  mov         ecx,dword ptr [ebx+28h] 
01074341  mov         dword ptr [ebx+1Ch],eax 
01074344  mov         dword ptr [ebx+20h],edi 
01074347  push        0    
01074349  push        dword ptr [_QCbackground (1141828h)] 
0107434F  call        image_spec_value (106FF15h) 
01074354  cmp         eax,dword ptr [_Qnil (1108D6Ch)] 
0107435A  pop         ecx  
0107435B  pop         ecx  
0107435C  jne         tiff_load+25Ch (1074373h) 
    IMAGE_BACKGROUND (img, f, ximg);
0107435E  test        byte ptr [ebx+18h],2 
01074362  jne         tiff_load+25Ch (1074373h) 
01074364  push        dword ptr [specified_file] 
01074367  push        dword ptr [f] 
0107436A  push        ebx  
0107436B  call        image_background (107012Ah) 
01074370  add         esp,0Ch 

  /* Put the image into the pixmap, then free the X image and its buffer.  */
  x_put_x_image (f, ximg, img->pixmap, width, height);
  x_destroy_x_image (ximg);
01074373  mov         eax,dword ptr [specified_file] 
01074376  call        x_destroy_x_image (10706D5h) 
0107437B  xor         esi,esi 
0107437D  inc         esi  
  xfree (buf);
0107437E  push        dword ptr [buf] 
01074381  call        xfree (1007174h) 
01074386  pop         ecx  

  UNGCPRO;
  return 1;
01074387  mov         eax,esi 
01074389  pop         edi  
0107438A  pop         esi  
0107438B  pop         ebx  
}
0107438C  leave            
0107438D  ret

Re: Problem with library images on Windows (again)

[Juanma Barranquero]

> Yes. If, immediately before the POP EBX, I manually restore the stack
> to the value it had after the PUSH EBX at the start of the routine,

I meant: "Immediately before POP EDI, to the value it had after PUSH EDI."

-- 
                    /L/e/k/t/u

Re: Problem with library images on Windows (again)

[Eli Zaretskii]

> Date: Thu, 19 May 2005 21:31:23 +0200
> From: Juanma Barranquero <lekktu@gmail.com>
> 
> But I don't think is an error in MSVC. I think it's some mixup between
> "pascal" and "C" style calls (as you know, the former remove their
> arguments, the latter don't). MSVC must be thinking that the external
> TIFF functions do remove their arguments, so it is not doing it.
> Sounds like an issue with the external declarations on the header
> files.

Can you show the declarations of the TIFF functions as they appear in
the header files visible to MSVC when it compiles Emacs?

Re: Problem with library images on Windows (again)

[Kim F. Storm]

"Eli Zaretskii" <eliz@gnu.org> writes:

>> Date: Thu, 19 May 2005 21:31:23 +0200
>> From: Juanma Barranquero <lekktu@gmail.com>
>> 
>> But I don't think is an error in MSVC. I think it's some mixup between
>> "pascal" and "C" style calls (as you know, the former remove their
>> arguments, the latter don't). MSVC must be thinking that the external
>> TIFF functions do remove their arguments, so it is not doing it.
>> Sounds like an issue with the external declarations on the header
>> files.
>
> Can you show the declarations of the TIFF functions as they appear in
> the header files visible to MSVC when it compiles Emacs?

Could it be that the following define need to specify "Pascal" somewhere
for the tiff library?

#define DEF_IMGLIB_FN(func) FARPROC fn_##func

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Problem with library images on Windows (again)

[Eli Zaretskii]

> Cc: Juanma Barranquero <lekktu@gmail.com>,  emacs-devel@gnu.org
> From: storm@cua.dk (Kim F. Storm)
> Date: Fri, 20 May 2005 10:24:55 +0200
> 
> Could it be that the following define need to specify "Pascal" somewhere
> for the tiff library?
> 
> #define DEF_IMGLIB_FN(func) FARPROC fn_##func

I think it should be something else.  First, from Juanma's
investigations it looks like MSVC already thinks those functions use
the Pascal ABI.  Second, the MinGW compiler does produce correct code,
so the fix should be only for MSVC (perhaps some preprocessor macro
works for GCC, but not MSVC).  And third, I think the right qualifiers
are __stdcall vs __cdecl, not "Pascal".

I asked to see the relevant fragments from libtiff headers used during
the compilation so that we could figure out what goes wrong there.

Re: Problem with library images on Windows (again)

[Jason Rumney]

Kim F. Storm wrote:

>"Eli Zaretskii" <eliz@gnu.org> writes:
>
>  
>
>>Can you show the declarations of the TIFF functions as they appear in
>>the header files visible to MSVC when it compiles Emacs?
>>    
>>
>
>Could it be that the following define need to specify "Pascal" somewhere
>for the tiff library?
>
>#define DEF_IMGLIB_FN(func) FARPROC fn_##func
>  
>
The version of TIFF headers I have uses extern "C" if compiling under 
C++, so I doubt the library is using Pascal calling conventions.
Maybe Juanma's library is different than mine.

Re: Problem with library images on Windows (again)

[Juanma Barranquero]

> The version of TIFF headers I have uses extern "C" if compiling under
> C++, so I doubt the library is using Pascal calling conventions.
> Maybe Juanma's library is different than mine.

Unless I'm wrong, the issue is not how the functions are defined on
the headers, because we don't statically link against the functions,
but how are they compiled in the library and how do we define the
fn_XXX pointers to them. In this case, the GnuWin32 TIFF 3.7.2
libraries seem to be compiled to use C calling conventions, but we
declare pointers to them with DEF_IMGLIB_FN, which says unequivocally
that they're FARPROC (i.e, "pascal").

Defining

 typedef int (FAR CDECL *CFARPROC)();
 #define DEF_IMGLIB_FNC(func) CFARPROC fn_##func

and using DEF_IMGLIB_FNC for the Tiff functions works.

The TIFF page in gnuwin32.sourceforge.net says:

"The GnuWin32 distribution comes in two versions. The ordinary version
uses the standard Unix equivalents, such as fopen and read, for input
and output, the other version (Tiff-win32) uses the Win32 API
functions, such as CreateFile and ReadFile, for input and output. The
ordinary version (Tiff) is more suitable for porting Unix programs,
the Win32-API version is more suitable for writing Win32 programs."

However, I'm getting the same results (problems, I mean) with the
standard and the Win32 versions.

-- 
                    /L/e/k/t/u

Re: Problem with library images on Windows (again)

[Eli Zaretskii]

> Date: Fri, 20 May 2005 12:42:09 +0200
> From: Juanma Barranquero <lekktu@gmail.com>
> 
> Unless I'm wrong, the issue is not how the functions are defined on
> the headers, because we don't statically link against the functions,
> but how are they compiled in the library and how do we define the
> fn_XXX pointers to them.

It's the issue with both: the prototypes in the header files should be
consistent with how the functions were compiled, since those
prototypes are the only way for the compiler to know how the functions
were compiled.  If it doesn't know that, it might produce wrong code.

> In this case, the GnuWin32 TIFF 3.7.2
> libraries seem to be compiled to use C calling conventions, but we
> declare pointers to them with DEF_IMGLIB_FN, which says unequivocally
> that they're FARPROC (i.e, "pascal").

That seems to be the problem.  What I still don't get is how come the
same declaration that uses FARPROC works for the MinGW build?  Can you
figure this out?

If we know this, we could fix the DEF_IMGLIB_FN macro so that it does
TRT for both compilers, e.g., by using __MINGW__ condition.

> The TIFF page in gnuwin32.sourceforge.net says:
> 
> "The GnuWin32 distribution comes in two versions. The ordinary version
> uses the standard Unix equivalents, such as fopen and read, for input
> and output, the other version (Tiff-win32) uses the Win32 API
> functions, such as CreateFile and ReadFile, for input and output. The
> ordinary version (Tiff) is more suitable for porting Unix programs,
> the Win32-API version is more suitable for writing Win32 programs."

This doesn't say anything about the calling conventions, just about
the inner workings of the functions.  So I'm not surprised that both
versions have the same effect.

Re: Problem with library images on Windows (again)

[Jason Rumney]

Eli Zaretskii wrote:

>That seems to be the problem.  What I still don't get is how come the
>same declaration that uses FARPROC works for the MinGW build?  Can you
>figure this out?
>  
>
Without looking at the disassembly, I'd guess that MinGW gcc and 
non-optimised MSVC builds restore the stack pointer from a known 
location regardless of the calling convention, which should always work.


>If we know this, we could fix the DEF_IMGLIB_FN macro so that it does
>TRT for both compilers, e.g., by using __MINGW__ condition.
>  
>
I think TRT will not depend on __MINGW__.  Mingw works despite there 
being a bug, not because of it.

Re: Problem with library images on Windows (again)

[Eli Zaretskii]

> Date: Fri, 20 May 2005 16:46:38 +0100
> From: Jason Rumney <jasonr@gnu.org>
> CC: Juanma Barranquero <lekktu@gmail.com>,  emacs-devel@gnu.org
> 
> Without looking at the disassembly, I'd guess that MinGW gcc and 
> non-optimised MSVC builds restore the stack pointer from a known 
> location regardless of the calling convention, which should always work.
> 
> 
> >If we know this, we could fix the DEF_IMGLIB_FN macro so that it does
> >TRT for both compilers, e.g., by using __MINGW__ condition.
> >  
> >
> I think TRT will not depend on __MINGW__.  Mingw works despite there 
> being a bug, not because of it.

So you are saying that MinGW works simply because it doesn't optimize
as well as MSVC, yes?  That could be it, but I'd prefer that someone
looks at the code produced by MinGW and see that it indeed doesn't
restore the stack pointer, either.

Re: Problem with library images on Windows (again)

[Juanma Barranquero]

> That could be it, but I'd prefer that someone
> looks at the code produced by MinGW and see that it indeed doesn't
> restore the stack pointer, either.

I've not had much luck trying to build with MinGW, so I won't be able
to test it. Jason, could you perhaps try it?

-- 
                    /L/e/k/t/u

Re: Problem with library images on Windows (again)

[Juanma Barranquero]

With the single change from

  #define DEF_IMGLIB_FN(func) FARPROC fn_##func

to

 #define DEF_IMGLIB_FN(func) int (FAR CDECL *fn_##func)()

all test images I have do work beautifully.

MORE IMPORTANT: two horrible hacks that I added about a year ago (when
I first fought this same issue and came to the wrong conclusion) to
keep Emacs from crashing CAN BE REMOVED!

So I'd say this is probably the right answer. I've commited the
changes so people with MinGW builds can test them.

If this works for them, today will be a happy day indeed :)

                                   /L/e/k/t/u

Re: Problem with library images on Windows (again)

[Jason Rumney]

Juanma Barranquero <lekktu@gmail.com> writes:

> With the single change from
>
>   #define DEF_IMGLIB_FN(func) FARPROC fn_##func
>
> to
>
>  #define DEF_IMGLIB_FN(func) int (FAR CDECL *fn_##func)()
>
> all test images I have do work beautifully.

If the other image libraries we use also use C calling conventions,
then that is the correct fix, I think.

Re: Problem with library images on Windows (again)

[Juanma Barranquero]

> If the other image libraries we use also use C calling conventions,
> then that is the correct fix, I think.

I confess I've not taken the time to check whether all the GnuWin32
libraries do use C calling conventions, but I've tested all kind of
images with that fix, and they worked. That suggest they really do use
C calling conventions; calling a C-style function as a Pascal one can
get obscured by the fact that functions do reset the frame and stack
pointers upon returning, but calling a Pascal-style function as a C
one would remove too many things from the stack and it would crash on
the next RET...

However, and in the interest of peace of mind, I'll trace the
functions and see that they're really doing what I think :)

-- 
                    /L/e/k/t/u

Re: Problem with library images on Windows (again)

[Jason Rumney]

Juanma Barranquero <lekktu@gmail.com> writes:

>> If the other image libraries we use also use C calling conventions,
>> then that is the correct fix, I think.
>
> However, and in the interest of peace of mind, I'll trace the
> functions and see that they're really doing what I think :)

Studying the headers, png and jpeg explicitly use _cdecl (or __cdecl
depending on compiler), while xpm and gif specify nothing (so will
default to _cdecl). So the fix you suggested is correct, I think.

Re: Problem with library images on Windows (again)

[Juanma Barranquero]

> Studying the headers, png and jpeg explicitly use _cdecl (or __cdecl
> depending on compiler), while xpm and gif specify nothing (so will
> default to _cdecl).

Yeah. And they indeed do use "ret" and not "ret n".

> So the fix you suggested is correct, I think.

Cool. So, for the first time, the image support on MSVC builds is as
stable as on MinGW builds.

-- 
                    /L/e/k/t/u

Re: Problem with library images on Windows (again)

[Juanma Barranquero]

> Cool. So, for the first time, the image support on MSVC builds is as
> stable as on MinGW builds.

Well, I spoke too soon.

I have another problem with image support on MSVC builds. It is,
AFAICS, unrelated to the previous problem with the calling convention
of the libraries.

Doing

  (insert-image-file "c:/image/test/myimage.png")

works, at works C-o from dired. However, either one of:

  (insert-image (create-image "c:/image/test/myimage.png"))
  (put-image (create-image "c:/image/test/myimage.png") 0)

crashes. In other words: png_load, and jpeg_load, die an horrible death from:

  (image :type xxx :file "...")

but they have no trouble with

  (image :type xxx :data "...")

png_load, for example, dies at the call to

  fn_png_read_info (png_ptr, info_ptr);

on png_load, while jpeg_load dies at:

  fn_jpeg_read_header (&cinfo, 1);

Any clues about what can be happening?

-- 
                    /L/e/k/t/u

Re: Problem with library images on Windows (again)

[David Kastrup]

Juanma Barranquero <lekktu@gmail.com> writes:

>> Cool. So, for the first time, the image support on MSVC builds is as
>> stable as on MinGW builds.
>
> Well, I spoke too soon.
>
> I have another problem with image support on MSVC builds. It is,
> AFAICS, unrelated to the previous problem with the calling convention
> of the libraries.
>
> Doing
>
>   (insert-image-file "c:/image/test/myimage.png")
>
> works, at works C-o from dired. However, either one of:
>
>   (insert-image (create-image "c:/image/test/myimage.png"))
>   (put-image (create-image "c:/image/test/myimage.png") 0)
>
> crashes. In other words: png_load, and jpeg_load, die an horrible death from:
>
>   (image :type xxx :file "...")
>
> but they have no trouble with
>
>   (image :type xxx :data "...")
>
> png_load, for example, dies at the call to
>
>   fn_png_read_info (png_ptr, info_ptr);
>
> on png_load, while jpeg_load dies at:
>
>   fn_jpeg_read_header (&cinfo, 1);
>
> Any clues about what can be happening?

The files are not opened in binary mode or something like that?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Problem with library images on Windows (again)

[jasonr]

Quoting David Kastrup <dak@gnu.org>:

> The files are not opened in binary mode or something like that?

Its a possibility, but Emacs should be linked with binmode.obj, which makes the
default mode binary.

If the link with binmode.obj fails and default is text mode for some reason (as
happened with certain versions of MingW for 21.4 and earlier), we usually get
crashes loading .elc files either during startup or when opening custom
buffers.

Re: Problem with library images on Windows (again)

[Stefan Monnier]

>> The files are not opened in binary mode or something like that?

> Its a possibility, but Emacs should be linked with binmode.obj, which
> makes the default mode binary.

Also opening them in the wrong mode should only cause garbled images,
not crashes, right?


        Stefan

Re: Problem with library images on Windows (again)

[Juanma Barranquero]

On 6/3/05, David Kastrup <dak@gnu.org> wrote:

> The files are not opened in binary mode or something like that?

First thing I thought, but:

      /* Open the image file.  */
      fp = fopen (SDATA (file), "rb");

-- 
                    /L/e/k/t/u

Re: Problem with library images on Windows (again)

[Juanma Barranquero]

Funnier and funnier.

I've been able to compile an MSVC debug release of libpng13; with it,
both the MSVC and the GCC builds of Emacs crash.

It happens inside msvcr71d.dll; (part of) the stack is:

  	ntdll.dll!7c928fea() 	
  	ntdll.dll!7c91104b() 	
  	msvcr71d.dll!_lock_file(void * pf=0x77c2fce0)  Line 236	C
>	msvcr71d.dll!fread(void * buffer=0x0082e268, unsigned int
size=0x00000001, unsigned int count=0x00000004, _iobuf *
stream=0x77c2fce0)  Line 75 + 0x9	C
  	libpng13d.dll!png_default_read_data(png_struct_def *
png_ptr=0x20844540, unsigned char * data=0x0082e268, unsigned int
length=0x00000004)  Line 55 + 0x19	C
  	libpng13d.dll!png_read_data(png_struct_def * png_ptr=0x20844540,
unsigned char * data=0x0082e268, unsigned int length=0x00000004)  Line
31 + 0x14	C
  	libpng13d.dll!png_read_info(png_struct_def * png_ptr=0x20844540,
png_info_struct * info_ptr=0x208483e0)  Line 402 + 0xf	C

The crash is on the _lock_file() call inside the msvcr71
implementation of fread(); the relevant code is:

void __cdecl _lock_file (
        void *pf
        )
{
        /*
         * The way the FILE (pointed to by pf) is locked depends on whether
         * it is part of _iob[] or not
         */
        if ( (pf >= (void *)_iob) && (pf <= (void *)(&_iob[_IOB_ENTRIES-1])) )
            /*
             * FILE lies in _iob[] so the lock lies in _locktable[].
             */
            _lock( _STREAM_LOCKS + (int)((FILE *)pf - _iob) );
        else
            /*
             * Not part of _iob[]. Therefore, *pf is a _FILEX and the
             * lock field of the struct is an initialized critical
             * section.
             */
            EnterCriticalSection( &(((_FILEX *)pf)->lock) );
}

The code decides that the FILE * is not part of _iob[] and calls
EnterCriticalSection, which gets an access violation writing on
0x00000010.

(I suppose I could get around the issue by writing read functions for
libpng and jpeg and bypassing the standard code altogether; still,
it'd be great to understand what's happening.)

-- 
                    /L/e/k/t/u

Re: Problem with library images on Windows (again)

[Juanma Barranquero]

> (I suppose I could get around the issue by writing read functions for
> libpng and jpeg and bypassing the standard code altogether;

And finally, that's what I've done.

The following patch gets rid of png_init_io() and jpeg_stdio_src(),
and instead define custom read functions to read image data from FILE
*. With it, image support finally works as expected on MSVC and MinGW
builds of Emacs on Windows.

Highlights of the patch:

 - It has no ChangeLog entries :-) (I'll write them once the patch is accepted)
 - Deletion of png_init_io() and jpeg_stdio_src() is not conditional
on HAVE_NTGUI; I've assumed it's best to have just one mechanism for
Windows, Unix, GNU/Linux and MacOS platforms. I can reinstate the
deleted functions and put the alternative into #ifdef's if deemed
necessary.
 - I've changed a few names of the memory-reading data source
functions for JPEG so it's clearer the paralelism between from-memory
and from-file data source setups.
 - Most jpeg code I've added is heavily based on jdatasrc.c, from the
JPEG library. I assume there's no legal trouble, as other jpeg code
comes also from JPEG library examples.

-- 
                    /L/e/k/t/u


Index: src/image.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/image.c,v
retrieving revision 1.27
diff -u -2 -r1.27 image.c
--- src/image.c	11 Jun 2005 16:24:36 -0000	1.27
+++ src/image.c	14 Jun 2005 01:46:10 -0000
@@ -5632,5 +5632,4 @@
 DEF_IMGLIB_FN (png_destroy_read_struct);
 DEF_IMGLIB_FN (png_set_read_fn);
-DEF_IMGLIB_FN (png_init_io);
 DEF_IMGLIB_FN (png_set_sig_bytes);
 DEF_IMGLIB_FN (png_read_info);
@@ -5664,5 +5663,4 @@
   LOAD_IMGLIB_FN (library, png_destroy_read_struct);
   LOAD_IMGLIB_FN (library, png_set_read_fn);
-  LOAD_IMGLIB_FN (library, png_init_io);
   LOAD_IMGLIB_FN (library, png_set_sig_bytes);
   LOAD_IMGLIB_FN (library, png_read_info);
@@ -5690,5 +5688,4 @@
 #define fn_png_destroy_read_struct	png_destroy_read_struct
 #define fn_png_set_read_fn		png_set_read_fn
-#define fn_png_init_io			png_init_io
 #define fn_png_set_sig_bytes		png_set_sig_bytes
 #define fn_png_read_info		png_read_info
@@ -5763,4 +5760,21 @@
 
 
+/* Function set as reader function when reading PNG image from a file.
+   PNG_PTR is a pointer to the PNG control structure.  Copy LENGTH
+   bytes from the input to DATA.  */
+
+static void
+png_read_from_file (png_ptr, data, length)
+     png_structp png_ptr;
+     png_bytep data;
+     png_size_t length;
+{
+  FILE *fp = (FILE *) fn_png_get_io_ptr (png_ptr);
+
+  if (fread (data, 1, length, fp) < length)
+    fn_png_error (png_ptr, "Read error");
+}
+
+
 /* Load PNG image IMG for use on frame F.  Value is non-zero if
    successful.  */
@@ -5896,5 +5910,5 @@
     fn_png_set_read_fn (png_ptr, (void *) &tbr, png_read_from_memory);
   else
-    fn_png_init_io (png_ptr, fp);
+    fn_png_set_read_fn (png_ptr, (void *) fp, png_read_from_file);
 
   fn_png_set_sig_bytes (png_ptr, sizeof sig);
@@ -6295,5 +6309,4 @@
 DEF_IMGLIB_FN (jpeg_read_header);
 DEF_IMGLIB_FN (jpeg_read_scanlines);
-DEF_IMGLIB_FN (jpeg_stdio_src);
 DEF_IMGLIB_FN (jpeg_std_error);
 DEF_IMGLIB_FN (jpeg_resync_to_restart);
@@ -6311,5 +6324,4 @@
   LOAD_IMGLIB_FN (library, jpeg_start_decompress);
   LOAD_IMGLIB_FN (library, jpeg_read_header);
-  LOAD_IMGLIB_FN (library, jpeg_stdio_src);
   LOAD_IMGLIB_FN (library, jpeg_CreateDecompress);
   LOAD_IMGLIB_FN (library, jpeg_destroy_decompress);
@@ -6337,5 +6349,4 @@
 #define fn_jpeg_read_header		jpeg_read_header
 #define fn_jpeg_read_scanlines		jpeg_read_scanlines
-#define fn_jpeg_stdio_src		jpeg_stdio_src
 #define fn_jpeg_std_error		jpeg_std_error
 #define jpeg_resync_to_restart_wrapper	jpeg_resync_to_restart
@@ -6359,4 +6370,15 @@
 
 
+/* Method to terminate data source.  Called by
+   jpeg_finish_decompress() after all data has been processed.
+   Common to memory and file source managers.  */
+
+static void
+our_common_term_source (cinfo)
+     j_decompress_ptr cinfo;
+{
+}
+
+
 /* Init source method for JPEG data source manager.  Called by
    jpeg_read_header() before any data is actually read.  See
@@ -6364,5 +6386,5 @@
 
 static void
-our_init_source (cinfo)
+our_memory_init_source (cinfo)
      j_decompress_ptr cinfo;
 {
@@ -6375,5 +6397,5 @@
 
 static boolean
-our_fill_input_buffer (cinfo)
+our_memory_fill_input_buffer (cinfo)
      j_decompress_ptr cinfo;
 {
@@ -6395,5 +6417,5 @@
 
 static void
-our_skip_input_data (cinfo, num_bytes)
+our_memory_skip_input_data (cinfo, num_bytes)
      j_decompress_ptr cinfo;
      long num_bytes;
@@ -6412,14 +6434,4 @@
 
 
-/* Method to terminate data source.  Called by
-   jpeg_finish_decompress() after all data has been processed.  */
-
-static void
-our_term_source (cinfo)
-     j_decompress_ptr cinfo;
-{
-}
-
-
 /* Set up the JPEG lib for reading an image from DATA which contains
    LEN bytes.  CINFO is the decompression info structure created for
@@ -6445,9 +6457,9 @@
 
   src = (struct jpeg_source_mgr *) cinfo->src;
-  src->init_source = our_init_source;
-  src->fill_input_buffer = our_fill_input_buffer;
-  src->skip_input_data = our_skip_input_data;
+  src->init_source = our_memory_init_source;
+  src->fill_input_buffer = our_memory_fill_input_buffer;
+  src->skip_input_data = our_memory_skip_input_data;
   src->resync_to_restart = jpeg_resync_to_restart_wrapper; /* Use
default method.  */
-  src->term_source = our_term_source;
+  src->term_source = our_common_term_source;
   src->bytes_in_buffer = len;
   src->next_input_byte = data;
@@ -6455,4 +6467,136 @@
 
 
+/* JPEG data source manager setup for JPEG images read from a file.
+   Heavily based on jdatasrc.c from the JPEG library.  */
+
+struct my_jpeg_file_src {
+  struct jpeg_source_mgr pub;   /* public fields */
+
+  FILE *infile;                 /* source stream */
+  JOCTET *buffer;               /* start of buffer */
+  boolean start_of_file;        /* have we gotten any data yet? */
+};
+
+typedef struct my_jpeg_file_src *my_jpeg_file_ptr;
+
+#define JPEG_INPUT_BUF_SIZE 4096    /* choose an efficiently fread'able size */
+
+
+/* Init source method for JPEG data source manager.
+   Called by jpeg_read_header() before any data is actually read.  */
+
+static void
+our_file_init_source (j_decompress_ptr cinfo)
+{
+  my_jpeg_file_ptr src = (my_jpeg_file_ptr) cinfo->src;
+
+  /* We reset the empty-input-file flag for each image,
+   * but we don't clear the input buffer.
+   * This is correct behavior for reading a series of images from one source.
+   */
+  src->start_of_file = 1;
+}
+
+
+/* Fill input buffer method for JPEG data source manager.
+   Called whenever more data is needed.  */
+
+static boolean
+our_file_fill_input_buffer (j_decompress_ptr cinfo)
+{
+  my_jpeg_file_ptr src = (my_jpeg_file_ptr) cinfo->src;
+  size_t nbytes;
+
+  nbytes = fread (src->buffer, 1, JPEG_INPUT_BUF_SIZE, src->infile);
+
+  if (nbytes <= 0) {
+    if (src->start_of_file)	/* Treat empty input file as fatal error */
+      ERREXIT (cinfo, JERR_INPUT_EMPTY);
+    WARNMS (cinfo, JWRN_JPEG_EOF);
+    /* Insert a fake EOI marker */
+    src->buffer[0] = (JOCTET) 0xFF;
+    src->buffer[1] = (JOCTET) JPEG_EOI;
+    nbytes = 2;
+  }
+
+  src->pub.next_input_byte = src->buffer;
+  src->pub.bytes_in_buffer = nbytes;
+  src->start_of_file = 0;
+
+  return 1;
+}
+
+
+/* Method to skip over NUM_BYTES bytes in the image data.
+   CINFO->src is the JPEG data source manager.  */
+
+static void
+our_file_skip_input_data (j_decompress_ptr cinfo, long num_bytes)
+{
+  my_jpeg_file_ptr src = (my_jpeg_file_ptr) cinfo->src;
+
+  /* Just a dumb implementation for now.  Could use fseek() except
+   * it doesn't work on pipes.	Not clear that being smart is worth
+   * any trouble anyway --- large skips are infrequent.
+   */
+  if (num_bytes > 0)
+    {
+      while (num_bytes > (long) src->pub.bytes_in_buffer)
+        {
+          num_bytes -= (long) src->pub.bytes_in_buffer;
+          (void) our_file_fill_input_buffer (cinfo);
+          /* note we assume that fill_file_input_buffer will never
return FALSE,
+           * so suspension need not be handled.
+           */
+        }
+      src->pub.next_input_byte += (size_t) num_bytes;
+      src->pub.bytes_in_buffer -= (size_t) num_bytes;
+    }
+}
+
+
+/* Set up the JPEG lib for reading an image from a FILE * via
+   a customized function, thus avoiding FILE * conflicts between
+   different builds of the image library.  CINFO is the
+   decompression info structure created for reading the image.  */
+
+static void
+jpeg_file_src (cinfo, infile)
+     j_decompress_ptr cinfo;
+     FILE *infile;
+{
+    my_jpeg_file_ptr src;
+
+  /* The source object and input buffer are made permanent so that a series
+   * of JPEG images can be read from the same file by calling jpeg_stdio_src
+   * only before the first one.	 (If we discarded the buffer at the end of
+   * one image, we'd likely lose the start of the next one.)
+   * This makes it unsafe to use this manager and a different source
+   * manager serially with the same JPEG object.  Caveat programmer.
+   */
+    if (cinfo->src == NULL)
+      {
+        /* first time for this JPEG object? */
+        cinfo->src = (struct jpeg_source_mgr *)
+          (*cinfo->mem->alloc_small) ((j_common_ptr) cinfo, JPOOL_PERMANENT,
+                                      sizeof (struct my_jpeg_file_src));
+        src = (my_jpeg_file_ptr) cinfo->src;
+        src->buffer = (JOCTET *)
+          (*cinfo->mem->alloc_small) ((j_common_ptr) cinfo, JPOOL_PERMANENT,
+                                      JPEG_INPUT_BUF_SIZE * sizeof (JOCTET));
+      }
+
+    src = (my_jpeg_file_ptr) cinfo->src;
+    src->pub.init_source = our_file_init_source;
+    src->pub.fill_input_buffer = our_file_fill_input_buffer;
+    src->pub.skip_input_data = our_file_skip_input_data;
+    src->pub.resync_to_restart = jpeg_resync_to_restart_wrapper; /*
use default method */
+    src->pub.term_source = our_common_term_source;
+    src->infile = infile;
+    src->pub.bytes_in_buffer = 0; /* forces fill_input_buffer on first read */
+    src->pub.next_input_byte = NULL; /* until buffer loaded */
+}
+
+
 /* Load image IMG for use on frame F.  Patterned after example.c
    from the JPEG lib.  */
@@ -6538,5 +6682,5 @@
 
   if (NILP (specified_data))
-    fn_jpeg_stdio_src (&cinfo, (FILE *) fp);
+    jpeg_file_src (&cinfo, fp);
   else
     jpeg_memory_src (&cinfo, SDATA (specified_data),

Re: Problem with library images on Windows (again)

[David Kastrup]

Juanma Barranquero <lekktu@gmail.com> writes:

>  - Most jpeg code I've added is heavily based on jdatasrc.c, from
> the JPEG library. I assume there's no legal trouble, as other jpeg
> code comes also from JPEG library examples.

The examples are just that: examples.  They are supposed to illustrate
the most basic/natural use of the library and so don't contain
copyrightable material unless somebody defined the interface very
badly.  You can claim copyright only on passages of sufficient
creative value, and examples often are explicitly exempt (don't know
whether this is the case here).

So you can't judge the viability of taking core code (it sounds to me
like that is what you did) by the viability of following example code.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Problem with library images on Windows (again)

[Juanma Barranquero]

On 6/14/05, David Kastrup <dak@gnu.org> wrote:

> So you can't judge the viability of taking core code (it sounds to me
> like that is what you did) by the viability of following example code.

The code for the memory data source in image.c is already pretty similar.

In any case, I found this bit from install.doc, which sums up the
situation perfectly:

   2. Microsoft C cannot pass file pointers between applications and DLLs.
  (See Microsoft Knowledge Base, PSS ID Number Q50336.)  So jdatasrc.c and
  jdatadst.c don't work if you open a file in your application and then pass
  the pointer to the DLL.  One workaround is to make jdatasrc.c/jdatadst.c
  part of your main application rather than part of the DLL.

-- 
                    /L/e/k/t/u

Re: Problem with library images on Windows (again)

[Juanma Barranquero]

> Without looking at the disassembly, I'd guess that MinGW gcc and
> non-optimised MSVC builds restore the stack pointer from a known
> location regardless of the calling convention, which should always work.

Optimized MSVC builds also restore the stack and frame pointers. But
before that it tries to pop local registers saved on the stack. Bingo!

My guess (but I've not compiled un-optimized to test it) is that
non-optimized MSVC builds work because the compiler is reloading
variables and/or being much less aggresive in caching values on
registers.

-- 
                    /L/e/k/t/u

Re: Problem with library images on Windows (again)

[Juanma Barranquero]

> It's the issue with both: the prototypes in the header files should be
> consistent with how the functions were compiled, since those
> prototypes are the only way for the compiler to know how the functions
> were compiled.  If it doesn't know that, it might produce wrong code.

Yes, in general, for static linking (either with static libraries, or
with stubs to dynamic ones). But in this case, MSVC is not
compiling/liking statically against any function as defined in the
header. There are no TIFF functions, as far as it knows, just a bunch
of variables of type FARPROC that we're assigning and calling, so
whether there's

 FAR CDECL TIFFOpen(...);

or

 FARPROC TIFFOpen(...);

on the header should be irrelevant. MSVC knows nothing about TIFFOpen;
it just knows there's a variable fn_TIFFOpen which gets initialized
with a call to GetProcAddress() and called afterwards. And it's the
type of fn_TIFFOpen which counts.

> That seems to be the problem.  What I still don't get is how come the
> same declaration that uses FARPROC works for the MinGW build?  Can you
> figure this out?

Perhaps Jason's right and the bug is just not noticeable.

Think of this: if lookup_image didn't use a temporary variable that
gets assigned to a register and accidentally clobbered by tiff_load,
we wouldn't see the problem with MSVC either.

> This doesn't say anything about the calling conventions, just about
> the inner workings of the functions.  So I'm not surprised that both
> versions have the same effect.

Well, I hoped that the Win32 version would use the more common
convention for DLL exports on Windows (that is, pascal). But it
doesn't.

-- 
                    /L/e/k/t/u

Re: Problem with library images on Windows (again)

[Benjamin Riefenstahl]

Hi Jason,


Jason Rumney writes:
> The version of TIFF headers I have uses extern "C" if compiling
> under C++, so I doubt the library is using Pascal calling
> conventions.  Maybe Juanma's library is different than mine.

'extern "C"' doesn't mean what you think it means, at least not in
most compilers.

In theory 'extern "<Language>"' is the C++ equivalent of the
propriatory '__cdecl', '__stdcall' and '__pascal' keywords known from
the C compilers.  But the exact behaviour of that C++ feature is
implementation defined, especially for languages other than "C".

In practice with MSC++ and GCC, '__stdcall' and 'extern "C"' are
*both* needed (or at least it's highly recommended to use both) in C++
mode to actually get '__stdcall' calling conventions and name
mangling.


benny