Re: master 7cd8236d35c: Pacify --enable-gcc-warnings with emacs_fdopen

Re: master 7cd8236d35c: Pacify --enable-gcc-warnings with emacs_fdopen

[Po Lu]

Paul Eggert <eggert@cs.ucla.edu> writes:

> branch: master
> commit 7cd8236d35c033fefb7be742e6c3290c63eaf609
> Author: Paul Eggert <eggert@cs.ucla.edu>
> Commit: Paul Eggert <eggert@cs.ucla.edu>
>
>     Pacify --enable-gcc-warnings with emacs_fdopen
>     
>     * src/lisp.h (emacs_fdopen): Now ATTRIBUTE_MALLOC
>     ATTRIBUTE_DEALLOC (emacs_fclose, 1), to pacify gcc
>     -Wsuggest-attribute=malloc on non-Android platforms.
> ---
>  src/lisp.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/lisp.h b/src/lisp.h
> index 447912581d7..85de57b0b2f 100644
> --- a/src/lisp.h
> +++ b/src/lisp.h
> @@ -5086,8 +5086,9 @@ extern int emacs_open (const char *, int, int);
>  extern int emacs_open_noquit (const char *, int, int);
>  extern int emacs_pipe (int[2]);
>  extern int emacs_close (int);
> -extern FILE *emacs_fdopen (int, const char *);
>  extern int emacs_fclose (FILE *);
> +extern FILE *emacs_fdopen (int, const char *)
> +  ATTRIBUTE_MALLOC ATTRIBUTE_DEALLOC (emacs_fclose, 1);
>  extern int emacs_unlink (const char *);
>  extern int emacs_symlink (const char *, const char *);
>  extern int emacs_rmdir (const char *);

Thanks, but now I receive warnings during the automated daily build of
the Android port:

../../emacs/src/xfaces.c: In function ‘Fx_load_color_file’:
../../emacs/src/xfaces.c:7007:7: warning: ‘emacs_fclose’ called on pointer returned from a mismatched allocation function [-Wmismatched-dealloc]
 7007 |       emacs_fclose (fp);
      |       ^~~~~~~~~~~~~~~~~
../../emacs/src/xfaces.c:6984:8: note: returned from ‘emacs_fopen’
 6984 |   fp = emacs_fopen (SSDATA (abspath), "r" FOPEN_TEXT);

Re: master 7cd8236d35c: Pacify --enable-gcc-warnings with emacs_fdopen

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1043 bytes --]

On 2023-08-07 18:07, Po Lu wrote:

> now I receive warnings during the automated daily build of
> the Android port:
> 
> ../../emacs/src/xfaces.c: In function ‘Fx_load_color_file’:
> ../../emacs/src/xfaces.c:7007:7: warning: ‘emacs_fclose’ called on pointer returned from a mismatched allocation function [-Wmismatched-dealloc]
>   7007 |       emacs_fclose (fp);
>        |       ^~~~~~~~~~~~~~~~~
> ../../emacs/src/xfaces.c:6984:8: note: returned from ‘emacs_fopen’
>   6984 |   fp = emacs_fopen (SSDATA (abspath), "r" FOPEN_TEXT);

I gave a shot at fixing that by installing the attached. It works for me 
on Fedora 38, but then again the old version worked too. If this doesn't 
work on Android, it might help to send the preprocessed output.

By the way, I see lisp.h includes stdio.h now. Would it make sense to 
create a new include file for use only by the parts of Emacs that need 
to deal with stdio streams? We could move some of the stuff out of 
lisp.h, which is bulging a bit at the seams nowadays.

[-- Attachment #2: 0001-Fix-some-emacs_fopen-confusion.patch --]
[-- Type: text/x-patch, Size: 4314 bytes --]

From 85b6c150c8a356ebf3442fde2836364954aa938c Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 7 Aug 2023 21:23:28 -0700
Subject: [PATCH] Fix some emacs_fopen confusion
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem reported by Po Lu in:
https://lists.gnu.org/r/emacs-devel/2023-08/msg00195.html
* src/comp.c (comp_hash_source_file, Fcomp__release_ctxt):
* src/sysdep.c (get_up_time, procfs_ttyname, procfs_get_total_memory):
Be more systematic about using emacs_fclose on streams that were
opened with emacs_fopen or emacs_fdopen.  Do this even if not
Android, as this simplifies checking that it's done consistently.
* src/lisp.h (emacs_fclose): If it’s just fclose,
make it a macro rather than a function, to avoid confusing gcc
-Wmismatched-dealloc.
(emacs_fopen): Move decl here from sysstdio.h, because sysstdio.h
is included from non-Emacs executables and emacs_fopen is good
only inside Emacs.
* src/sysdep.c (emacs_fclose): Define as a function only if Android.
---
 src/comp.c     |  4 ++--
 src/lisp.h     |  6 ++++++
 src/sysdep.c   | 12 +++++-------
 src/sysstdio.h |  2 --
 4 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/src/comp.c b/src/comp.c
index 1bde4ae5821..b81a80b00f8 100644
--- a/src/comp.c
+++ b/src/comp.c
@@ -776,7 +776,7 @@ comp_hash_source_file (Lisp_Object filename)
 #else
   int res = md5_stream (f, SSDATA (digest));
 #endif
-  fclose (f);
+  emacs_fclose (f);
 
   if (res)
     xsignal2 (Qfile_notify_error, build_string ("hashing failed"), filename);
@@ -4749,7 +4749,7 @@ DEFUN ("comp--release-ctxt", Fcomp__release_ctxt, Scomp__release_ctxt,
     gcc_jit_context_release (comp.ctxt);
 
   if (logfile)
-    fclose (logfile);
+    emacs_fclose (logfile);
   comp.ctxt = NULL;
 
   return Qt;
diff --git a/src/lisp.h b/src/lisp.h
index 85de57b0b2f..2f26e5eddce 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -5086,9 +5086,15 @@ maybe_disable_address_randomization (int argc, char **argv)
 extern int emacs_open_noquit (const char *, int, int);
 extern int emacs_pipe (int[2]);
 extern int emacs_close (int);
+#if !(defined HAVE_ANDROID && !defined ANDROID_STUBIFY)
+# define emacs_fclose fclose
+#else
 extern int emacs_fclose (FILE *);
+#endif
 extern FILE *emacs_fdopen (int, const char *)
   ATTRIBUTE_MALLOC ATTRIBUTE_DEALLOC (emacs_fclose, 1);
+extern FILE *emacs_fopen (char const *, char const *)
+  ATTRIBUTE_MALLOC ATTRIBUTE_DEALLOC (emacs_fclose, 1);
 extern int emacs_unlink (const char *);
 extern int emacs_symlink (const char *, const char *);
 extern int emacs_rmdir (const char *);
diff --git a/src/sysdep.c b/src/sysdep.c
index a995bc66741..0f8b70c8248 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -2644,15 +2644,13 @@ emacs_fdopen (int fd, const char *mode)
    clear information associated with the FILE's file descriptor if
    necessary.  */
 
+#if defined HAVE_ANDROID && !defined ANDROID_STUBIFY
 int
 emacs_fclose (FILE *stream)
 {
-#if !(defined HAVE_ANDROID && !defined ANDROID_STUBIFY)
-  return fclose (stream);
-#else
   return android_fclose (stream);
-#endif
 }
+#endif
 
 /* Wrappers around unlink, symlink, rename, renameat_noreplace, and
    rmdir.  These operations handle asset and content directories on
@@ -3492,7 +3490,7 @@ get_up_time (void)
 	  Lisp_Object subsec = Fcons (make_fixnum (upfrac), make_fixnum (hz));
 	  up = Ftime_add (sec, subsec);
 	}
-      fclose (fup);
+      emacs_fclose (fup);
     }
   unblock_input ();
 
@@ -3540,7 +3538,7 @@ procfs_ttyname (int rdev)
 		}
 	    }
 	}
-      fclose (fdev);
+      emacs_fclose (fdev);
     }
   unblock_input ();
   return build_string (name);
@@ -3582,7 +3580,7 @@ procfs_get_total_memory (void)
 	  }
       while (!done);
 
-      fclose (fmem);
+      emacs_fclose (fmem);
     }
   unblock_input ();
   return retval;
diff --git a/src/sysstdio.h b/src/sysstdio.h
index 5a973c833cc..8e9e5bec86c 100644
--- a/src/sysstdio.h
+++ b/src/sysstdio.h
@@ -28,8 +28,6 @@ #define EMACS_SYSSTDIO_H
 #include <attribute.h>
 #include <unlocked-io.h>
 
-extern FILE *emacs_fopen (char const *, char const *)
-  ATTRIBUTE_MALLOC ATTRIBUTE_DEALLOC (fclose, 1);
 extern void errputc (int);
 extern void errwrite (void const *, ptrdiff_t);
 extern void close_output_streams (void);
-- 
2.39.2

Re: master 7cd8236d35c: Pacify --enable-gcc-warnings with emacs_fdopen

[Po Lu]

Paul Eggert <eggert@cs.ucla.edu> writes:

> I gave a shot at fixing that by installing the attached. It works for
> me on Fedora 38, but then again the old version worked too. If this
> doesn't work on Android, it might help to send the preprocessed
> output.

It works now, thanks.

> By the way, I see lisp.h includes stdio.h now. Would it make sense to
> create a new include file for use only by the parts of Emacs that need
> to deal with stdio streams? We could move some of the stuff out of
> lisp.h, which is bulging a bit at the seams nowadays.

Fine by me, though IMO someone should first demonstrate that at least
some files which include that new header won't need to include lisp.h
anyway.

Re: master 7cd8236d35c: Pacify --enable-gcc-warnings with emacs_fdopen

[Paul Eggert]

On 2023-08-07 22:26, Po Lu wrote:
> someone should first demonstrate that at least
> some files which include that new header won't need to include lisp.h
> anyway.

That sounds too strict. If we used that rule, Emacs would likely have 
just one enormous lisp.h file, and no other header file. That would not 
be good modularization.