[PATCH] Fix compilation erorr when --enable-gcc-warnings passed

[PATCH] Fix compilation erorr when --enable-gcc-warnings passed

[Alexander Kuleshov]

This patch does not provide functional changes, but contains fixes
for compilation errors if GNU Emacs was configured with the
--enable-gcc-warnings option. Mostly "defined but not used" fixed.

* src/gtkutil.c: set Qreverse_landscape if orientation symbol not
given. Prevents compilation error: orientation_symbol can be not
initialzed.
* src/image.c: add check that COLOR_TABLE_SUPPORT defined. Prevents
compilation error: macro COLOR_TABLE_SUPPORT defined but not used.
* src/xterm.c: move 'Window' declaration in the x_draw_fringe_bitmap()
to prevent errors "defined but not used".

---
 src/gtkutil.c |  2 +-
 src/image.c   | 46 +++++++++++++++++++++++++++++-----------------
 src/xterm.c   |  6 +++---
 3 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/src/gtkutil.c b/src/gtkutil.c
index 768df34..bf7a067 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -4096,7 +4096,7 @@ xg_get_page_setup (void)
     orientation_symbol = Qlandscape;
   else if (orientation == GTK_PAGE_ORIENTATION_REVERSE_PORTRAIT)
     orientation_symbol = Qreverse_portrait;
-  else if (orientation == GTK_PAGE_ORIENTATION_REVERSE_LANDSCAPE)
+  else
     orientation_symbol = Qreverse_landscape;
 
   return listn (CONSTYPE_HEAP, 7,
diff --git a/src/image.c b/src/image.c
index 8bb5ff7..03b4489 100644
--- a/src/image.c
+++ b/src/image.c
@@ -75,7 +75,9 @@ typedef struct x_bitmap_record Bitmap_Record;
 #endif
 
 /* W32_TODO : Color tables on W32.  */
+#ifdef COLOR_TABLE_SUPPORT
 #undef COLOR_TABLE_SUPPORT
+#endif
 
 typedef struct w32_bitmap_record Bitmap_Record;
 #define GET_PIXEL(ximg, x, y) GetPixel (ximg, x, y)
@@ -90,11 +92,16 @@ typedef struct w32_bitmap_record Bitmap_Record;
 #endif /* HAVE_NTGUI */
 
 #ifdef USE_CAIRO
+#ifdef COLOR_TABLE_SUPPORT
 #undef COLOR_TABLE_SUPPORT
 #endif
+#endif
 
 #ifdef HAVE_NS
+
+#ifdef COLOR_TABLE_SUPPORT
 #undef COLOR_TABLE_SUPPORT
+#endif
 
 typedef struct ns_bitmap_record Bitmap_Record;
 
@@ -4615,16 +4622,15 @@ colors_in_color_table (int *n)
 static unsigned long
 lookup_rgb_color (struct frame *f, int r, int g, int b)
 {
-  unsigned long pixel;
-
 #ifdef HAVE_NTGUI
-  pixel = PALETTERGB (r >> 8, g >> 8, b >> 8);
+  return PALETTERGB (r >> 8, g >> 8, b >> 8);
 #endif /* HAVE_NTGUI */
 
 #ifdef HAVE_NS
-  pixel = RGB_TO_ULONG (r >> 8, g >> 8, b >> 8);
+  return RGB_TO_ULONG (r >> 8, g >> 8, b >> 8);
 #endif /* HAVE_NS */
-  return pixel;
+
+  return 0;
 }
 
 static void
@@ -7328,10 +7334,10 @@ tiff_load (struct frame *f, struct image *img)
         for (x = 0; x < width; ++x)
           {
             uint32 abgr = row[x];
-            int r = TIFFGetR (abgr);
-            int g = TIFFGetG (abgr);
-            int b = TIFFGetB (abgr);
-            int a = TIFFGetA (abgr);
+            r = TIFFGetR (abgr);
+            g = TIFFGetG (abgr);
+            b = TIFFGetB (abgr);
+            a = TIFFGetA (abgr);
             *dataptr++ = (a << 24) | (r << 16) | (g << 8) | b;
           }
       }
@@ -7634,13 +7640,11 @@ gif_load (struct frame *f, struct image *img)
 {
   int rc, width, height, x, y, i, j;
   ColorMapObject *gif_color_map;
-  unsigned long pixel_colors[256];
   GifFileType *gif;
   gif_memory_source memsrc;
   Lisp_Object specified_bg = image_spec_value (img->spec, QCbackground, NULL);
   Lisp_Object specified_file = image_spec_value (img->spec, QCfile, NULL);
   Lisp_Object specified_data = image_spec_value (img->spec, QCdata, NULL);
-  unsigned long bgcolor = 0;
   EMACS_INT idx;
   int gif_err;
 
@@ -7648,6 +7652,8 @@ gif_load (struct frame *f, struct image *img)
   unsigned char *data = 0;
 #else
   XImagePtr ximg;
+  unsigned long pixel_colors[256];
+  unsigned long bgcolor = 0;
 #endif
 
   if (NILP (specified_data))
@@ -7834,8 +7840,12 @@ gif_load (struct frame *f, struct image *img)
 
   init_color_table ();
   if (STRINGP (specified_bg))
+#ifndef USE_CAIRO
     bgcolor = x_alloc_image_color (f, img, specified_bg,
-				   FRAME_BACKGROUND_PIXEL (f));
+                                FRAME_BACKGROUND_PIXEL (f));
+#else
+    x_alloc_image_color (f, img, specified_bg, FRAME_BACKGROUND_PIXEL (f));
+#endif
   for (j = 0; j <= idx; ++j)
     {
       /* We use a local variable `raster' here because RasterBits is a
@@ -9182,11 +9192,13 @@ svg_load_image (struct frame *f,         /* Pointer to emacs frame structure.  *
   int height;
   const guint8 *pixels;
   int rowstride;
-  XImagePtr ximg;
-  Lisp_Object specified_bg;
-  XColor background;
+#ifndef USE_CAIRO
   int x;
   int y;
+  XColor background;
+  XImagePtr ximg;
+  Lisp_Object specified_bg;
+#endif
 
 #if ! GLIB_CHECK_VERSION (2, 36, 0)
   /* g_type_init is a glib function that must be called prior to
@@ -9597,8 +9609,6 @@ x_kill_gs_process (Pixmap pixmap, struct frame *f)
 			0, 0, img->width, img->height, ~0, ZPixmap);
       if (ximg)
 	{
-	  int x, y;
-
 	  /* Initialize the color table.  */
 	  init_color_table ();
 
@@ -9606,6 +9616,8 @@ x_kill_gs_process (Pixmap pixmap, struct frame *f)
 	     color table.  After having done so, the color table will
 	     contain an entry for each color used by the image.  */
 #ifdef COLOR_TABLE_SUPPORT
+	  int x, y;
+
 	  for (y = 0; y < img->height; ++y)
 	    for (x = 0; x < img->width; ++x)
 	      {
diff --git a/src/xterm.c b/src/xterm.c
index 5a6d643..530c918 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -1312,7 +1312,6 @@ x_draw_fringe_bitmap (struct window *w, struct glyph_row *row, struct draw_fring
 {
   struct frame *f = XFRAME (WINDOW_FRAME (w));
   Display *display = FRAME_X_DISPLAY (f);
-  Window window = FRAME_X_WINDOW (f);
   GC gc = f->output_data.x->normal_gc;
   struct face *face = p->face;
 
@@ -1359,6 +1358,7 @@ x_draw_fringe_bitmap (struct window *w, struct glyph_row *row, struct draw_fring
       Pixmap pixmap, clipmask = (Pixmap) 0;
       int depth = DefaultDepthOfScreen (FRAME_X_SCREEN (f));
       XGCValues gcv;
+      Window window = FRAME_X_WINDOW (f);
 
       if (p->wd > 8)
 	bits = (char *) (p->bits + p->dh);
@@ -3749,7 +3749,7 @@ x_delete_glyphs (struct frame *f, register int n)
   emacs_abort ();
 }
 
-
+#if !defined USE_CAIRO || !defined USE_TOOLKIT_SCROLL_BARS
 /* Like XClearArea, but check that WIDTH and HEIGHT are reasonable.
    If they are <= 0, this is probably an error.  */
 
@@ -3760,7 +3760,7 @@ x_clear_area1 (Display *dpy, Window window,
   eassert (width > 0 && height > 0);
   XClearArea (dpy, window, x, y, width, height, exposures);
 }
-
+#endif
 
 void
 x_clear_area (struct frame *f, int x, int y, int width, int height)
-- 
2.7.0.25.gfc10eb5

Re: [PATCH] Fix compilation erorr when --enable-gcc-warnings passed

[Paul Eggert]

On 01/22/2016 02:54 AM, Alexander Kuleshov wrote:
> @@ -4615,16 +4622,15 @@ colors_in_color_table (int *n)
>   static unsigned long
>   lookup_rgb_color (struct frame *f, int r, int g, int b)
>   {
> -  unsigned long pixel;
> -
>   #ifdef HAVE_NTGUI
> -  pixel = PALETTERGB (r >> 8, g >> 8, b >> 8);
> +  return PALETTERGB (r >> 8, g >> 8, b >> 8);
>   #endif /* HAVE_NTGUI */
>   
>   #ifdef HAVE_NS
> -  pixel = RGB_TO_ULONG (r >> 8, g >> 8, b >> 8);
> +  return RGB_TO_ULONG (r >> 8, g >> 8, b >> 8);
>   #endif /* HAVE_NS */
> -  return pixel;
> +
> +  return 0;
>   }

Does this change cause lookup_rgb_color to always return 0 on platforms 
other than MS-Windows and NextStep? Is this the right thing to do? This 
part of the change isn't mentioned in the draft ChangeLog entry, so I'm 
a bit lost as to the motivation.

Re: [PATCH] Fix compilation erorr when --enable-gcc-warnings passed

[Alexander Kuleshov]

Hello Paul,

On 01-22-16, Paul Eggert wrote:
> On 01/22/2016 02:54 AM, Alexander Kuleshov wrote:
> >@@ -4615,16 +4622,15 @@ colors_in_color_table (int *n)
> >  static unsigned long
> >  lookup_rgb_color (struct frame *f, int r, int g, int b)
> >  {
> >-  unsigned long pixel;
> >-
> >  #ifdef HAVE_NTGUI
> >-  pixel = PALETTERGB (r >> 8, g >> 8, b >> 8);
> >+  return PALETTERGB (r >> 8, g >> 8, b >> 8);
> >  #endif /* HAVE_NTGUI */
> >  #ifdef HAVE_NS
> >-  pixel = RGB_TO_ULONG (r >> 8, g >> 8, b >> 8);
> >+  return RGB_TO_ULONG (r >> 8, g >> 8, b >> 8);
> >  #endif /* HAVE_NS */
> >-  return pixel;
> >+
> >+  return 0;
> >  }
> 
> Does this change cause lookup_rgb_color to always return 0 on platforms
> other than MS-Windows and NextStep? Is this the right thing to do? This part
> of the change isn't mentioned in the draft ChangeLog entry, so I'm a bit
> lost as to the motivation.

Some motivation: Previous version was:

static unsigned long
lookup_rgb_color (struct frame *f, int r, int g, int b)
{
  unsigned long pixel;

#ifdef HAVE_NTGUI
  pixel = PALETTERGB (r >> 8, g >> 8, b >> 8);
#endif /* HAVE_NTGUI */

#ifdef HAVE_NS
  pixel = RGB_TO_ULONG (r >> 8, g >> 8, b >> 8);
#endif /* HAVE_NS */
  return pixel;
}

So, during compilation with enabled warnings, we will get following
error:

image.c: In function ‘lookup_rgb_color’:
image.c:4634:10: error: ‘pixel’ is used uninitialized in this function [-Werror=uninitialized]
   return pixel;

And seems that this version of the lookup_rgb_color() is only for these platforms, because I see that the src/image.c contains yet another implementation of the lookup_rgb_color() for platforms where COLOR_TABLE_SUPPORT is enabled.

Re: [PATCH] Fix compilation erorr when --enable-gcc-warnings passed

[Paul Eggert]

Alexander Kuleshov wrote:
> And seems that this version of the lookup_rgb_color() is only for these platforms, because I see that the src/image.c contains yet another implementation of the lookup_rgb_color() for platforms where COLOR_TABLE_SUPPORT is enabled.

Although there are two lookup_rgb_color implementations, the buggy one is 
invoked if USE_CAIRO, and this generates garbage on the screen. I have filed a 
bug report; see:

http://bugs.gnu.org/22442

I installed a patch into emacs-25 to replace the compile-time warning with a 
run-time crash, if you configure --with-cairo --enable-checking and visit a .png 
file. This patch should also fix the other warnings you noticed, albeit in 
different ways sometimes.

Thanks for doing this checking! It found a real bug.

bug#22442: 25.0.50; --with-cairo does not display PNG on GNU/Linux

[Paul Eggert]

I installed a patch which should make Bug#22442 easier to reproduce:

./configure --with-cairo --enable-checking
make
wget https://upload.wikimedia.org/wikipedia/commons/f/f5/All_palaeotemps.png
src/emacs -Q All_palaeotemps.png

This should make Emacs dump core.

I'll CC: this to Jan D. as he's expert in this area.

Re: [PATCH] Fix compilation erorr when --enable-gcc-warnings passed

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 23 Jan 2016 01:08:08 -0800
> Cc: emacs-devel@gnu.org
> 
> I installed a patch into emacs-25 to replace the compile-time warning with a 
> run-time crash, if you configure --with-cairo --enable-checking and visit a .png 
> file.

If we know in advance that some scenario doesn't work, then eassert is
not TRT.  We should signal an error instead.  eassert is for things
that "cannot happen", i.e. they are there to help us discover the
(unknown) scenarios where our assumptions are false.  In this case,
the scenario is already known, and moreover it is very simple to
trigger, so aborting is not the best strategy.

bug#22442: 25.0.50; --with-cairo does not display PNG on GNU/Linux

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 23 Jan 2016 01:12:03 -0800
> Cc: Jan D <jan.h.d@swipnet.se>
> 
> I'll CC: this to Jan D. as he's expert in this area.

Alas, Jan is no longer working on Emacs.

Re: [PATCH] Fix compilation erorr when --enable-gcc-warnings passed

[Paul Eggert]

Eli Zaretskii wrote:
> We should signal an error instead.

OK, thanks, I changed it to do that.

Re: [PATCH] Fix compilation erorr when --enable-gcc-warnings passed

[Alexander Kuleshov]

On 01-23-16, Paul Eggert wrote:
> Although there are two lookup_rgb_color implementations, the buggy one is
> invoked if USE_CAIRO, and this generates garbage on the screen. I have filed
> a bug report; see:
> 
> http://bugs.gnu.org/22442
> 
> I installed a patch into emacs-25 to replace the compile-time warning with a
> run-time crash, if you configure --with-cairo --enable-checking and visit a
> .png file. This patch should also fix the other warnings you noticed, albeit
> in different ways sometimes.
> 
> Thanks for doing this checking! It found a real bug.

I'm long-time emacs user, but just started to dive into its source code. Saw
some problems during compilation and decided to send a patch because these minor
things like compiler warnings sometimes brings useful results.

One question, You sad that you installed a ptach..., but where can I find this
patch? I've looked into http://bugs.gnu.org/22442, but can't find it there.

Thank you

Re: [PATCH] Fix compilation erorr when --enable-gcc-warnings passed

[Alexander Kuleshov]

btw, I've build emacs from master with --enable-checks and --with-cairo
and displaying of the *.png image from your example works perfectly for
me.

Re: [PATCH] Fix compilation erorr when --enable-gcc-warnings passed

[Alexander Kuleshov]

On 01-23-16, Alexander Kuleshov wrote:
> btw, I've build emacs from master with --enable-checks and --with-cairo
> and displaying of the *.png image from your example works perfectly for
> me.

Sorry me for spamming, but I've found patch, built emacs with cairo,
but anyway I can't reproduce your crash. The png image displaying
perfectly.

Re: [PATCH] Fix compilation erorr when --enable-gcc-warnings passed

[Paul Eggert]

Alexander Kuleshov wrote:
> I can't reproduce your crash. The png image displaying
> perfectly.

I suspect the bug is now masked because lookup_rgb_color now throws a signal 
under Cairo. I followed up at:

http://bugs.gnu.org/22442#20

Re: [PATCH] Fix compilation erorr when --enable-gcc-warnings passed

[Alexander Kuleshov]

On 01-23-16, Paul Eggert wrote:
> Alexander Kuleshov wrote:
> >I can't reproduce your crash. The png image displaying
> >perfectly.
> 
> I suspect the bug is now masked because lookup_rgb_color now throws a signal
> under Cairo. I followed up at:
> 
> http://bugs.gnu.org/22442#20

Hello,

Even without it I can't reproduce this bug. I've checkout to the previous
commit (035bd8159fe5bcea15eacc8a23ab54f4bc3b6f17) and configured and
built emacs with:

./configure --enable-link-time-optimization --without-pop \
--without-kerberos --without-kerberos5 --without-hesiod   \
--with-sound=alsa --with-x-toolkit=gtk3 --with-xpm --with-jpeg \
--with-tiff --with-gif --with-png --with-rsvg --with-cairo     \
--with-xml2 --with-imagemagick --with-xft --without-libotf     \
--with-xim --with-xaw3d --with-dbus --without-gconf            \
--without-gsettings --without-selinux --with-gnutls --with-zlib \
--with-modules --with-file-notification=inotify --without-makeinfo \
--with-x && make

Than I've opened image as you described in the http://bugs.gnu.org/22442
and it works perfectly for me.

bug#22442: [PATCH] Fix compilation erorr when --enable-gcc-warnings passed

[Paul Eggert]

Alexander Kuleshov wrote:
> Even without it I can't reproduce this bug. I've checkout to the previous
> commit (035bd8159fe5bcea15eacc8a23ab54f4bc3b6f17) and configured and
> built emacs with:
>
> ./configure --enable-link-time-optimization --without-pop \
> --without-kerberos --without-kerberos5 --without-hesiod   \
> --with-sound=alsa --with-x-toolkit=gtk3 --with-xpm --with-jpeg \
> --with-tiff --with-gif --with-png --with-rsvg --with-cairo     \
> --with-xml2 --with-imagemagick --with-xft --without-libotf     \
> --with-xim --with-xaw3d --with-dbus --without-gconf            \
> --without-gsettings --without-selinux --with-gnutls --with-zlib \
> --with-modules --with-file-notification=inotify --without-makeinfo \
> --with-x && make
>
> Than I've opened image as you described in thehttp://bugs.gnu.org/22442
> and it works perfectly for me.

I tried the same scenario and I get a blank image on the screen. Perhaps it 
depends on the imagemagick or cairo version or whatever. I'm running Emacs on 
Fedora 23 x86-64, and displaying on Ubuntu 15.10 x86-64 via an ssh -X link.

If I use the current emacs-25 commit () but keep everything else the same, Emacs 
displays the binary text of the image with a message in the bottom line "Cannot 
display image: (This Emacs mishandles this image file type)", which is what I 
would expect from the current code. I don't know why I wasn't observing this 
behavior earlier, but at least now I know that Bug#22442 is still kicking.

As Mark says, Bug#22442 and Bug#21110 are probably the same bug, so I'll merge 
them. I can't recommend --with-cairo at this point.