bug#75451: scratch/igc: Enable CHECK_STRUCTS

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Stefan Kangas]

Severity: wishlist

Gerd, how do you feel about the below change?  There's no rush to make
this change if it's inconvenient for you, but since you use IN_MY_FORK
elsewhere, I figured that it might be okay for you here also?

If it's too much trouble, let's just revisit this later.  This bug
report will serve as a reminder, if we just lave it open.

diff --git a/src/igc.c b/src/igc.c
index 079b6a90ac6..b03c64a52b8 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -76,8 +76,7 @@
 # error "HAVE_PDUMPER required"
 #endif

-#if 0 /* Not yet because that make transfer between GNU and my fork
-	 painful.  */
+#ifndef IN_MY_FORK
 #ifdef CHECK_STRUCTS
 # include "dmpstruct.h"

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Gerd Möllmann]

Stefan Kangas <stefankangas@gmail.com> writes:

> Severity: wishlist
>
> Gerd, how do you feel about the below change?  There's no rush to make
> this change if it's inconvenient for you, but since you use IN_MY_FORK
> elsewhere, I figured that it might be okay for you here also?
>
> If it's too much trouble, let's just revisit this later.  This bug
> report will serve as a reminder, if we just lave it open.
>
> diff --git a/src/igc.c b/src/igc.c
> index 079b6a90ac6..b03c64a52b8 100644
> --- a/src/igc.c
> +++ b/src/igc.c
> @@ -76,8 +76,7 @@
>  # error "HAVE_PDUMPER required"
>  #endif
>
> -#if 0 /* Not yet because that make transfer between GNU and my fork
> -	 painful.  */
> +#ifndef IN_MY_FORK
>  #ifdef CHECK_STRUCTS
>  # include "dmpstruct.h"

No no, it's no trouble. Thanks for working on this!

I'd even go a step further and remove the whole #if. I think it's more
useful nowadays to have the checks than to make patching easier. (BTW,
the contents of the #if might be out of date, not sure if we meanwhile
fix/scan additional structs.)

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Stefan Kangas]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
>> Severity: wishlist
>>
>> Gerd, how do you feel about the below change?  There's no rush to make
>> this change if it's inconvenient for you, but since you use IN_MY_FORK
>> elsewhere, I figured that it might be okay for you here also?
>>
>> If it's too much trouble, let's just revisit this later.  This bug
>> report will serve as a reminder, if we just lave it open.
>>
>> diff --git a/src/igc.c b/src/igc.c
>> index 079b6a90ac6..b03c64a52b8 100644
>> --- a/src/igc.c
>> +++ b/src/igc.c
>> @@ -76,8 +76,7 @@
>>  # error "HAVE_PDUMPER required"
>>  #endif
>>
>> -#if 0 /* Not yet because that make transfer between GNU and my fork
>> -	 painful.  */
>> +#ifndef IN_MY_FORK
>>  #ifdef CHECK_STRUCTS
>>  # include "dmpstruct.h"
>
> No no, it's no trouble. Thanks for working on this!
>
> I'd even go a step further and remove the whole #if. I think it's more
> useful nowadays to have the checks than to make patching easier.

OK, thanks!  I removed the #if on scratch/igc (commit 826491d501d).

> (BTW, the contents of the #if might be out of date, not sure if we
> meanwhile fix/scan additional structs.)

I've now added checks for all structs that we use in "fix_*" functions
(commit 76a0d739024).  I kept notes to make sure I didn't miss anything,
but it's not impossible that I did.  I'll double-check things later.

I plan to do anything passed to the "finalize_*" ones next, which should
cover most of the ones we currently depend on, I think.

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Gerd Möllmann]

Stefan Kangas <stefankangas@gmail.com> writes:

>
>> (BTW, the contents of the #if might be out of date, not sure if we
>> meanwhile fix/scan additional structs.)
>
> I've now added checks for all structs that we use in "fix_*" functions
> (commit 76a0d739024).  I kept notes to make sure I didn't miss anything,
> but it's not impossible that I did.  I'll double-check things later.
>
> I plan to do anything passed to the "finalize_*" ones next, which should
> cover most of the ones we currently depend on, I think.

Very nice! Thanks for taking this on!

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Pip Cet via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
>>
>>> (BTW, the contents of the #if might be out of date, not sure if we
>>> meanwhile fix/scan additional structs.)
>>
>> I've now added checks for all structs that we use in "fix_*" functions
>> (commit 76a0d739024).  I kept notes to make sure I didn't miss anything,
>> but it's not impossible that I did.  I'll double-check things later.
>>
>> I plan to do anything passed to the "finalize_*" ones next, which should
>> cover most of the ones we currently depend on, I think.
>
> Very nice! Thanks for taking this on!

Thanks!

This isn't strictly about the scratch/igc branch, but I personally think
struct hashes should be checked in all builds, mismatches should be
downgraded to #warnings, and --enable-checking=all could include
-Werror=cpp.  (So the warnings would still abort a build with
--enable-checking=all, but they'd *also* show up in regular builds.)

Also, we should include them in the nativecomp ABI hash, as nativecomp
relies on struct layout and the tagging scheme in nontrivial ways
(most-positive-fixnum, for example, is treated as a compile-time
constant by comp.el, but I'm not sure all changes to it would
automatically affect the comp abi hash).

Changes required might include:

* autodetection of gawk vs awk (I think this simply means using $(AWK)
  rather than "awk" in src/Makefile.in; this is important on some
  machines which provide a very different "awk")

* igc.o and comp.o  should depend explicitly on dmpstruct.h (this is
  important for re-builds; in theory, the current scratch/igc branch is
  broken for highly parallel builds because dmpstruct.h might be
  generated after igc.o, but that is not important).

* a hash-of-hashes for nativecomp

Proposed partial patch:

diff --git a/src/Makefile.in b/src/Makefile.in
index 3d249a1abdd..f0ff9203b9d 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -546,10 +546,13 @@ dmpstruct_headers=
 	$(srcdir)/treesit.h $(srcdir)/window.h $(srcdir)/xwidget.h
 ifeq ($(CHECK_STRUCTS),true)
 pdumper.o: dmpstruct.h
+igc.o: dmpstruct.h
+comp.o: dmpstruct.h
 endif
+AWK = @AWK@
 dmpstruct.h: $(srcdir)/dmpstruct.awk
 dmpstruct.h: $(libsrc)/make-fingerprint$(EXEEXT) $(dmpstruct_headers)
-	$(AM_V_GEN)POSIXLY_CORRECT=1 awk -f $(srcdir)/dmpstruct.awk \
+	$(AM_V_GEN)POSIXLY_CORRECT=1 $(AWK) -f $(srcdir)/dmpstruct.awk \
 		$(dmpstruct_headers) > $@
 
 AUTO_DEPEND = @AUTO_DEPEND@
diff --git a/src/comp.c b/src/comp.c
index cf15817c2fc..c029572d612 100644
--- a/src/comp.c
+++ b/src/comp.c
@@ -24,6 +24,7 @@
 #include "igc.h"
 #include "comp.h"
 #include "pdumper.h"
+#include "dmpstruct.h"
 
 #ifdef HAVE_NATIVE_COMP
 
@@ -821,9 +822,12 @@ hash_native_abi (void)
 
   Vcomp_abi_hash =
     comp_hash_string (
-      concat3 (build_string (ABI_VERSION),
-	       concat3 (Vemacs_version, Vsystem_configuration,
-			Vsystem_configuration_options),
+      CALLN (Fconcat,
+	       build_string (ABI_VERSION),
+	       Vemacs_version,
+	       Vsystem_configuration,
+	       Vsystem_configuration_options,
+	       build_string (HASH_DMPSTRUCT),
 	       Fmapconcat (intern_c_string ("comp--subr-signature"),
 			   Vcomp_subr_list, build_string (""))));
 
diff --git a/src/dmpstruct.awk b/src/dmpstruct.awk
index e5e359e10b7..ac639082719 100644
--- a/src/dmpstruct.awk
+++ b/src/dmpstruct.awk
@@ -21,6 +21,7 @@ BEGIN {
   print "#define EMACS_DMPSTRUCT_H"
   struct_name = ""
   tmpfile = "dmpstruct.tmp"
+  tmpfile2 = "dmpstruct2.tmp"
 }
 # Match a type followed by optional syntactic whitespace
 /^(enum|struct|union) [a-zA-Z0-9_]+([\t ]|\/\*.*\*\/)*$/ {
@@ -29,6 +30,7 @@ BEGIN {
 }
 /^(enum|struct|union) [a-zA-Z0-9_]+([\t ]|\/\*.*\*\/)*$/, /^(  )?};$/ {
   print $0 > tmpfile
+  print $0 > tmpfile2
 }
 /^(  )?} *(GCALIGNED_STRUCT)? *;$/ {
   if (struct_name != "") {
@@ -41,5 +43,11 @@ BEGIN {
   }
 }
 END {
+  fflush (tmpfile2)
+  cmd = "../lib-src/make-fingerprint -r " tmpfile2
+  cmd | getline hash
+  close (cmd)
+  printf "#define HASH_%s_%.10s\n", "dmpstruct", hash
+  printf "#define HASH_DMPSTRUCT \"%.10s\"\n", hash
   print "#endif /* EMACS_DMPSTRUCT_H */"
 }

Pip

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Stefan Kangas]

Pip Cet <pipcet@protonmail.com> writes:

> This isn't strictly about the scratch/igc branch, but I personally think
> struct hashes should be checked in all builds, mismatches should be
> downgraded to #warnings, and --enable-checking=all could include
> -Werror=cpp.  (So the warnings would still abort a build with
> --enable-checking=all, but they'd *also* show up in regular builds.)

Sounds good to me.

> Also, we should include them in the nativecomp ABI hash, as nativecomp
> relies on struct layout and the tagging scheme in nontrivial ways
> (most-positive-fixnum, for example, is treated as a compile-time
> constant by comp.el, but I'm not sure all changes to it would
> automatically affect the comp abi hash).

I'm copying in Andrea for this part.

> Changes required might include:
>
> * autodetection of gawk vs awk (I think this simply means using $(AWK)
>   rather than "awk" in src/Makefile.in; this is important on some
>   machines which provide a very different "awk")

Using $(AWK) instead of raw "awk" makes sense, because why not.

But which awk implementations are you concerned about more specifically?
I would expect us to just use POSIX awk, without any gawk extensions.
For example, I have had no issues building on Debian (using mawk by
default) or macOS (with the BSD awk that descends from nawk, IIUC).

> * igc.o and comp.o  should depend explicitly on dmpstruct.h (this is
>   important for re-builds; in theory, the current scratch/igc branch is
>   broken for highly parallel builds because dmpstruct.h might be
>   generated after igc.o, but that is not important).

Sounds good also.

> * a hash-of-hashes for nativecomp
>
> Proposed partial patch:

(I didn't look at the patch yet, though the above sounds like it might
come out to more than one patch.)

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Stefan Kangas]

Pip Cet <pipcet@protonmail.com> writes:

> This isn't strictly about the scratch/igc branch, but I personally think
> struct hashes should be checked in all builds, mismatches should be
> downgraded to #warnings, and --enable-checking=all could include
> -Werror=cpp.  (So the warnings would still abort a build with
> --enable-checking=all, but they'd *also* show up in regular builds.)

As I was continuing to work on CHECK_STRUCTS, I stumbled into this and
re-read parts of the old discussion linked below:

commit 9994bf17cf532f2e1d4310341da7180342202191
Author: Paul Eggert <eggert@cs.ucla.edu>
Date:   Wed Apr 10 19:42:37 2019 -0700

    Bring back dmpstruct.h

    Bring back the dmpstruct.h checking, and use it when
    --enable-checking=structs is specified.  The checking can be helpful
    to some developers, although it gets in the way of others and is
    not needed for ordinary tarball builds.
    * src/dmpstruct.awk: Restore this file, with mode 644 not 755.
    * configure.ac: New option-arg --enable-checking=structs,
    implied by --enable-checking.
    (CHECK_STRUCTS): New macro and var.
    * src/Makefile.in (CHECK_STRUCTS): New macro.
    (dmpstruct_headers, dmpstruct.h, dmpstruct.h):
    Restore these macros and rules.
    (pdumper.o): Restore this dependency if $(CHECK_STRUCTS) is true.
    (mostlyclean): Remove dmpstruct.h.
    * src/pdumper.c [CHECK_STRUCTS]: Include dmpstruct.h,
    and restore checks against hashes.

commit 44a39e3e761c0774cd1bb9360db7f49e1d66ec06
Author: Paul Eggert <eggert@cs.ucla.edu>
Date:   Tue Apr 9 15:42:10 2019 -0700

    Remove dmpstruct.h

    The hassles of updating the dmpstruct.h-using code bit me again.
    These updates are more trouble than they’re worth.  See:
    https://lists.gnu.org/r/emacs-devel/2019-03/msg00122.html
    As I’m the main person who’s made changes in this area since
    dmpstruct.h was introduced, I’m the most motivated to clean up
    the situation.

I'm curios to know what is the experience while developing scratch/igc:
have these checks been very helpful?

Meanwhile, for enums, I'm wondering if something like this wouldn't make
more sense?  (Or perhaps in addition to CHECK_STRUCTS.  Hmm.)

diff --git a/src/igc.c b/src/igc.c
index 37929765522..6c8ad55878c 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -2770,6 +2776,8 @@ fix_vector (mps_ss_t ss, struct Lisp_Vector *v)
 #endif
 	IGC_FIX_CALL_FN (ss, struct Lisp_Vector, v, fix_vectorlike);
 	break;
+      default:
+	emacs_abort ();
       }
   }
   MPS_SCAN_END (ss);

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Gerd Möllmann]

Stefan Kangas <stefankangas@gmail.com> writes:

> commit 44a39e3e761c0774cd1bb9360db7f49e1d66ec06
> Author: Paul Eggert <eggert@cs.ucla.edu>
> Date:   Tue Apr 9 15:42:10 2019 -0700
>
>     Remove dmpstruct.h
>
>     The hassles of updating the dmpstruct.h-using code bit me again.
>     These updates are more trouble than they’re worth.  See:
>     https://lists.gnu.org/r/emacs-devel/2019-03/msg00122.html
>     As I’m the main person who’s made changes in this area since
>     dmpstruct.h was introduced, I’m the most motivated to clean up
>     the situation.
>
> I'm curios to know what is the experience while developing scratch/igc:
> have these checks been very helpful?

It was helpful for me, in the pdumper, because I hadn't activated it in
igc.c. Reason for it being that I have some other differences in my fork,
like CL packages :-).

> Meanwhile, for enums, I'm wondering if something like this wouldn't make
> more sense?  (Or perhaps in addition to CHECK_STRUCTS.  Hmm.)
>
> diff --git a/src/igc.c b/src/igc.c
> index 37929765522..6c8ad55878c 100644
> --- a/src/igc.c
> +++ b/src/igc.c
> @@ -2770,6 +2776,8 @@ fix_vector (mps_ss_t ss, struct Lisp_Vector *v)
>  #endif
>  	IGC_FIX_CALL_FN (ss, struct Lisp_Vector, v, fix_vectorlike);
>  	break;
> +      default:
> +	emacs_abort ();
>        }
>    }
>    MPS_SCAN_END (ss);

I'm personally a big fan of exhaustive switches, as igc.c might show
:-). I find they make it a lot easier to find all relevant places when
adding enums. And I think the default case would work against that.

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Paul Eggert]

On 2025-01-10 10:58, Gerd Möllmann wrote:
> Stefan Kangas <stefankangas@gmail.com> writes:

>> I'm curios to know what is the experience while developing scratch/igc:
>> have these checks been very helpful?
> 
> It was helpful for me, in the pdumper, because I hadn't activated it in
> igc.c. Reason for it being that I have some other differences in my fork,
> like CL packages :-).

I haven't been developing in scratch/igc and so can't say anything there.

As I recall, CHECK_STRUCTS was introduced for pdumper and got in the way 
of everything else I developed, where it generated nothing but false alarms.

> I'm personally a big fan of exhaustive switches, as igc.c might show
> :-). I find they make it a lot easier to find all relevant places when
> adding enums. And I think the default case would work against that.

Yes, inserting 'default: emacs_abort ();' switches from static to 
dynamic checking, which is less reliable. Although 'default: emacs_abort 
();' is sometimes the best one can do, it's better to avoid it when 
possible.

I use 'default: eassume (false);' more often than 'default: emacs_abort 
();', though for a completely different purpose: to tell GCC something 
that it's not smart enough to figure out on its own, so that GCC can do 
better static checking elsewhere.

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Pip Cet via Bug reports for GNU Emacs, the Swiss army knife of text editors]

"Stefan Kangas" <stefankangas@gmail.com> writes:

> Pip Cet <pipcet@protonmail.com> writes:
>
>> This isn't strictly about the scratch/igc branch, but I personally think
>> struct hashes should be checked in all builds, mismatches should be
>> downgraded to #warnings, and --enable-checking=all could include
>> -Werror=cpp.  (So the warnings would still abort a build with
>> --enable-checking=all, but they'd *also* show up in regular builds.)
>
> As I was continuing to work on CHECK_STRUCTS, I stumbled into this and
> re-read parts of the old discussion linked below:
>
> commit 9994bf17cf532f2e1d4310341da7180342202191
> Author: Paul Eggert <eggert@cs.ucla.edu>
> Date:   Wed Apr 10 19:42:37 2019 -0700
>
>     Bring back dmpstruct.h
>
>     Bring back the dmpstruct.h checking, and use it when
>     --enable-checking=structs is specified.  The checking can be helpful
>     to some developers, although it gets in the way of others and is
>     not needed for ordinary tarball builds.
>     * src/dmpstruct.awk: Restore this file, with mode 644 not 755.
>     * configure.ac: New option-arg --enable-checking=structs,
>     implied by --enable-checking.
>     (CHECK_STRUCTS): New macro and var.
>     * src/Makefile.in (CHECK_STRUCTS): New macro.
>     (dmpstruct_headers, dmpstruct.h, dmpstruct.h):
>     Restore these macros and rules.
>     (pdumper.o): Restore this dependency if $(CHECK_STRUCTS) is true.
>     (mostlyclean): Remove dmpstruct.h.
>     * src/pdumper.c [CHECK_STRUCTS]: Include dmpstruct.h,
>     and restore checks against hashes.
>
> commit 44a39e3e761c0774cd1bb9360db7f49e1d66ec06
> Author: Paul Eggert <eggert@cs.ucla.edu>
> Date:   Tue Apr 9 15:42:10 2019 -0700
>
>     Remove dmpstruct.h
>
>     The hassles of updating the dmpstruct.h-using code bit me again.
>     These updates are more trouble than they’re worth.  See:
>     https://lists.gnu.org/r/emacs-devel/2019-03/msg00122.html
>     As I’m the main person who’s made changes in this area since
>     dmpstruct.h was introduced, I’m the most motivated to clean up
>     the situation.
>
> I'm curios to know what is the experience while developing scratch/igc:
> have these checks been very helpful?

I can't really say that much about scratch/igc, but I'm modifying the
relevant structs a lot, on several branches.  I'm quite forgetful, but I
can't remember a time when the #error saved me; that's why I'd prefer a
#warning (but I understand the reluctance to downgrade errors into
warnings, thus my proposal of using -Werror=cpp).

And I did check code into scratch/igc which broke the --enable-checking
build.  A warning would most likely have prevented that.

So this is another weird case of "let's extend this, then remove the
original usage": IIUC, the reason for having pdumper include the errors
is that it was expected people would forget to adjust pdumper.c and test
only the unexec version.  Once unexec is removed, we can simply remove
the errors in pdumper.c.  scratch/igc is in a similar situation as
pdumper was when it was merged: it'll be optional for a long time, so we
need to take extra care to ensure that no one modifies the core
structures but forgets to update igc.c

(For example, right now both igc.c and alloc.c fail to scan the
"command_modes" member of struct Lisp_Subr when we're compiling without
native compilation: if someone were to use that field for a non-fixnum
Lisp object, that someone would run into weird bugs and crashes and
eventually figure out they need to fix alloc.c, but probably not igc.c,
so we'd need to know about such changes.  The hashing wouldn't
necessarily save us, but it might, because they'd probably reorder the
Lisp_Subr fields at the same time :-) ).

I can't really say whether it would make more sense for nativecomp to
simply create .eln files that are specific to one compiled Emacs rather
than trying to reuse old files if the ABI hash matches, but either that
or including the header hash would have saved me some trouble.

> Meanwhile, for enums, I'm wondering if something like this wouldn't make
> more sense?  (Or perhaps in addition to CHECK_STRUCTS.  Hmm.)

> diff --git a/src/igc.c b/src/igc.c
> index 37929765522..6c8ad55878c 100644
> --- a/src/igc.c
> +++ b/src/igc.c
> @@ -2770,6 +2776,8 @@ fix_vector (mps_ss_t ss, struct Lisp_Vector *v)
>  #endif
>  	IGC_FIX_CALL_FN (ss, struct Lisp_Vector, v, fix_vectorlike);
>  	break;
> +      default:
> +	emacs_abort ();
>        }
>    }
>    MPS_SCAN_END (ss);

Doesn't GCC have an option to turn non-exhaustive enum switches into an
error?  I concur with https://www.jwz.org/doc/java.html:

        that life-saving warning, ``enumeration value `x' not handled in
        switch''

It might be best to build Emacs with -Ddefault=nodefault.

Pip

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Paul Eggert]

On 2025-01-10 11:56, Pip Cet wrote:
> Doesn't GCC have an option to turn non-exhaustive enum switches into an
> error?

Yes, GCC has several such options. Emacs builds with 
--enable-gcc-warnings use -Wall, which enables -Wswitch, which I think 
does what you're asking for.

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Pip Cet via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

> On 2025-01-10 11:56, Pip Cet wrote:
>> Doesn't GCC have an option to turn non-exhaustive enum switches into an
>> error?
>
> Yes, GCC has several such options. Emacs builds with
> --enable-gcc-warnings use -Wall, which enables -Wswitch, which I think
> does what you're asking for.

The GCC options are rather strange: one of them is to warn about all
switches that do NOT contain a "default" label, which is the opposite of
what we want!

What we want, I think, is to prohibit a switch which contains a "case
ENUM_VALUE:" label (where ENUM_VALUE is, well, any enum's value) from
being either non-exhaustive across the enum or containing a default
label as well.

So I guess we need to hack GCC to add this useful combination?

Note that this fails for enums that end with a "fake" entry indicating
the number of cases, such as FONT_OBJECT_MAX (this value is also
misnamed, as it isn't the maximum index but the first index which isn't
used).  My understanding is that that practice is deprecated anyway,
because someone might want to use the enum as a bit field and including
an extra value may waste a bit.

Here's the easy part (GCC patch; build with --disable-werror because the
new warnings break the build):

diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc
index d547b08f55d..f1ce18ad851 100644
--- a/gcc/c-family/c-warn.cc
+++ b/gcc/c-family/c-warn.cc
@@ -1673,9 +1673,9 @@ c_do_switch_warnings (splay_tree cases, location_t switch_location,
     return;
 
   default_node = splay_tree_lookup (cases, (splay_tree_key) NULL);
-  if (!default_node)
+  if (default_node)
     warning_at (switch_location, OPT_Wswitch_default,
-		"switch missing default case");
+		"switch has a default case!");
 
   /* There are certain cases where -Wswitch-bool warnings aren't
      desirable, such as
@@ -1746,8 +1746,15 @@ c_do_switch_warnings (splay_tree cases, location_t switch_location,
       O(N), since the nature of the splay tree will keep the next
       element adjacent to the root at all times.  */
 
+  bool warned = false;
   for (chain = TYPE_VALUES (type); chain; chain = TREE_CHAIN (chain))
     {
+      if (default_node && !warned)
+	{
+	  warning_at (switch_location, OPT_Wswitch_default,
+		      "enumerated switch has a default case!");
+	  warned = true;
+	}
       tree value = TREE_VALUE (chain);
       tree attrs = DECL_ATTRIBUTES (value);
       value = DECL_INITIAL (value);
@@ -1806,9 +1813,7 @@ c_do_switch_warnings (splay_tree cases, location_t switch_location,
 	 to warn using -Wswitch because -Wswitch is enabled by -Wall
 	 while -Wswitch-enum is explicit.  */
       warning_at (switch_location,
-		  (default_node || !warn_switch
-		   ? OPT_Wswitch_enum
-		   : OPT_Wswitch),
+		  OPT_Wswitch,
 		  "enumeration value %qE not handled in switch",
 		  TREE_PURPOSE (chain));
     }

The hard part is extending this warning to

switch ((int)enum_value)
  {
  ...
  }

IIRC, it's quite common to effectively do that because your enum value
is stored in a bitfield, for example.  Possibly not a problem for Emacs
C code in src/, which is supposed to use ENUM_BF, IIUC.

(The patch doesn't handle empty enums, but GCC throws an error for
those, as it should :-) )

198 warnings to check, I guess...

Pip

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Paul Eggert]

On 2025-01-14 07:22, Pip Cet wrote:
> The GCC options are rather strange:

Yes, it seems there are so many differing opinions about enums that it's 
hard to (ahem) enumerate them all.


> What we want, I think, is to prohibit a switch which contains a "case
> ENUM_VALUE:" label (where ENUM_VALUE is, well, any enum's value) from
> being either non-exhaustive across the enum or containing a default
> label as well.

I wouldn't want to prohibit all such switches. I can think of lots of 
reasons to allow them, e.g., "enum { NAME = EXPR };" is a common way to 
name an integer constant expression.

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Pip Cet via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

> On 2025-01-14 07:22, Pip Cet wrote:
>> The GCC options are rather strange:
>
> Yes, it seems there are so many differing opinions about enums that it's
> hard to (ahem) enumerate them all.

Indeed, but I agree with Gerd that exhaustive switches are the thing to
do, when at all possible.

>> What we want, I think, is to prohibit a switch which contains a "case
>> ENUM_VALUE:" label (where ENUM_VALUE is, well, any enum's value) from
>> being either non-exhaustive across the enum or containing a default
>> label as well.
>
> I wouldn't want to prohibit all such switches. I can think of lots of
> reasons to allow them, e.g., "enum { NAME = EXPR };" is a common way to
> name an integer constant expression.

You're right.  I think it would be okay to add two exceptions:

1. If an enum contains a single value, it's not "switchable", so we
don't warn about it.
2. If an enum's values don't form a contiguous range, it's not
"switchable" either.

Together with the very useful exception already present in gcc (if we
switch on a value which is an integer constant, we don't warn about the
switch statement being non-exhaustive), I think that would work.  Kind
of does, with the patch below, but it obviously needs some more work :-)

svg_css_length_to_pixels doesn't handle RSVG_UNIT_CH; draw_fringe_bitmap
doesn't handle DEFAULT_CURSOR; treesit_query_error_to_string doesn't
handle TSQueryErrorLanguage.  The rest seem like false positives, at
first glance.

Of course, if it encourages people to use "if" instead of "switch", that
would usually defeat the purpose.

Pip

diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
index 81ca5d34df8..00b192a05da 100644
--- a/gcc/c-family/c-common.cc
+++ b/gcc/c-family/c-common.cc
@@ -5166,7 +5166,7 @@ case_compare (splay_tree_key k1, splay_tree_key k2)
 
 tree
 c_add_case_label (location_t loc, splay_tree cases, tree cond,
-		  tree low_value, tree high_value, tree attrs)
+		  tree low_value, tree high_value, tree original_low_value, tree original_high_value, tree attrs)
 {
   tree type;
   tree label;
@@ -5333,7 +5333,7 @@ c_add_case_label (location_t loc, splay_tree cases, tree cond,
     }
 
   /* Add a CASE_LABEL to the statement-tree.  */
-  case_label = add_stmt (build_case_label (low_value, high_value, label));
+  case_label = add_stmt (build_case_label (low_value, high_value, original_low_value, original_high_value, label));
   /* Register this case label in the splay tree.  */
   splay_tree_insert (cases,
 		     (splay_tree_key) low_value,
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index efd942c482b..1700784beaf 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -1067,6 +1067,7 @@ extern void c_init_preprocess (void);
 #define ENUM_FIXED_UNDERLYING_TYPE_P(NODE) (TYPE_LANG_FLAG_5 (NODE))
 
 extern tree do_case (location_t, tree, tree, tree);
+extern tree do_case (location_t, tree, tree, tree, tree);
 extern tree build_stmt (location_t, enum tree_code, ...);
 extern tree build_real_imag_expr (location_t, enum tree_code, tree);
 
@@ -1095,6 +1096,7 @@ extern tree boolean_increment (enum tree_code, tree);
 extern int case_compare (splay_tree_key, splay_tree_key);
 
 extern tree c_add_case_label (location_t, splay_tree, tree, tree, tree,
+			      tree, tree,
 			      tree = NULL_TREE);
 extern bool c_switch_covers_all_cases_p (splay_tree, tree);
 extern bool c_block_may_fallthru (const_tree);
diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc
index d547b08f55d..94cbe203428 100644
--- a/gcc/c-family/c-warn.cc
+++ b/gcc/c-family/c-warn.cc
@@ -1578,6 +1578,57 @@ match_case_to_enum (splay_tree_node node, void *data)
 
 /* Handle -Wswitch*.  Called from the front end after parsing the
    switch construct.  */
+
+static bool
+c_switchable_enum (tree type)
+{
+  if (type == NULL_TREE)
+    return false;
+
+  if (TREE_CODE (type) != ENUMERAL_TYPE)
+    return false;
+
+  tree min_value = NULL_TREE;
+  tree max_value = NULL_TREE;
+  size_t n_values = 0;
+  for (tree chain = TYPE_VALUES (type); chain; chain = TREE_CHAIN (chain))
+    {
+      tree value = TREE_VALUE (chain);
+      tree attrs = DECL_ATTRIBUTES (value);
+      value = DECL_INITIAL (value);
+      if (!min_value || tree_int_cst_compare (value, min_value) < 0)
+	min_value = value;
+      if (!max_value || tree_int_cst_compare (value, max_value) > 0)
+	max_value = value;
+      n_values++;
+    }
+
+  /* single-value enum */
+  if (n_values < 2 || tree_int_cst_compare (min_value, max_value) == 0)
+    return false;
+
+  while (tree_int_cst_compare (min_value, max_value) <= 0)
+    {
+      bool okay = false;
+      for (tree chain = TYPE_VALUES (type); chain; chain = TREE_CHAIN (chain))
+	{
+	  tree value = TREE_VALUE (chain);
+	  value = DECL_INITIAL (value);
+	  if (tree_int_cst_compare (value, min_value) == 0)
+	    {
+	      okay = true;
+	      break;
+	    }
+	}
+      if (!okay)
+	return false;
+
+      min_value = build_int_cst (TREE_TYPE (min_value), tree_to_shwi (min_value) + 1);
+    }
+
+  return true;
+}
+
 /* ??? Should probably be somewhere generic, since other languages
    besides C and C++ would want this.  At the moment, however, C/C++
    are the only tree-ssa languages that support enumerations at all,
@@ -1591,7 +1642,62 @@ c_do_switch_warnings (splay_tree cases, location_t switch_location,
   splay_tree_node node;
   tree chain;
   bool outside_range_p = false;
+  tree enum_type = NULL;
+  tree int_type = NULL;
+  bool inferred_type = false;
+  tree last_high_value = NULL_TREE;
+  for (splay_tree_node node = splay_tree_successor (cases, (splay_tree_key) NULL);
+       ;
+       node = splay_tree_successor (cases, (splay_tree_key) CASE_LOW ((tree)node->value)))
+    {
+      if (!node)
+	break;
+      if (node->key)
+	{
+	  if (CASE_ORIGINAL_LOW ((tree) node->value))
+	    {
+	      tree case_type = CASE_ORIGINAL_LOW ((tree) node->value);
+	    again:
+	      if (TREE_CODE (case_type) == ENUMERAL_TYPE)
+		{
+		  if (enum_type && enum_type != case_type)
+		    {
+		      location_t loc = EXPR_LOCATION ((tree) node->value);
+		      warning_at (loc,
+				  OPT_Wswitch_enum,
+				  "two different enums used in the same switch");
+		    }
+		  if (int_type)
+		    {
+		      location_t loc = EXPR_LOCATION ((tree) node->value);
+		      warning_at (loc,
+				  OPT_Wswitch_enum,
+				  "enums and integer values used in the same switch");
+		    }
+		  enum_type = case_type;
+		}
+	      else if (INTEGRAL_TYPE_P (case_type))
+		{
+		  if (enum_type)
+		    {
+		      location_t loc = EXPR_LOCATION ((tree) node->value);
+		      warning_at (loc,
+				  OPT_Wswitch_enum,
+				  "enums and integer values used in the same switch");
+		    }
 
+		  int_type = case_type;
+		}
+	      else
+		{
+		  case_type = TREE_TYPE (case_type);
+		  goto again;
+		}
+	    }
+	}
+      if (!(splay_tree_key) CASE_LOW ((tree) node->value))
+	break;
+    }
   if (type != error_mark_node
       && type != TREE_TYPE (cond)
       && INTEGRAL_TYPE_P (type)
@@ -1723,6 +1829,24 @@ c_do_switch_warnings (splay_tree cases, location_t switch_location,
 		    "switch condition has boolean value");
     }
 
+  if (TREE_CODE (type) != ENUMERAL_TYPE)
+    {
+      if (!enum_type)
+	return;
+
+      type = enum_type;
+      inferred_type = true;
+    }
+  else
+    {
+      if (enum_type && enum_type != type)
+	{
+	  location_t loc = EXPR_LOCATION (cond);
+	  warning_at (loc,
+		      OPT_Wswitch_enum,
+		      "enum type mismatch");
+	}
+    }
   /* From here on, we only care about enumerated types.  */
   if (!type || TREE_CODE (type) != ENUMERAL_TYPE)
     return;
@@ -1746,8 +1870,15 @@ c_do_switch_warnings (splay_tree cases, location_t switch_location,
       O(N), since the nature of the splay tree will keep the next
       element adjacent to the root at all times.  */
 
+  bool warned = false;
   for (chain = TYPE_VALUES (type); chain; chain = TREE_CHAIN (chain))
     {
+      if (default_node && !warned)
+	{
+	  warning_at (switch_location, OPT_Wswitch_default,
+		      !inferred_type ? "enumerated switch has a default case" : "enumerated switch has a default case (inferred)");
+	  warned = true;
+	}
       tree value = TREE_VALUE (chain);
       tree attrs = DECL_ATTRIBUTES (value);
       value = DECL_INITIAL (value);
@@ -1805,12 +1936,11 @@ c_do_switch_warnings (splay_tree cases, location_t switch_location,
 	 Wswitch-enum.  Otherwise, if both are enabled then we prefer
 	 to warn using -Wswitch because -Wswitch is enabled by -Wall
 	 while -Wswitch-enum is explicit.  */
-      warning_at (switch_location,
-		  (default_node || !warn_switch
-		   ? OPT_Wswitch_enum
-		   : OPT_Wswitch),
-		  "enumeration value %qE not handled in switch",
-		  TREE_PURPOSE (chain));
+      if (c_switchable_enum (type))
+	warning_at (switch_location,
+		    OPT_Wswitch,
+		    !inferred_type ? "enumeration value %qE not handled in switch" : "enumeration value %qE not handled in switch (inferred type)",
+		    TREE_PURPOSE (chain));
     }
 
   /* Attribute flag_enum means bitwise combinations are OK.  */
diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
index d2f45912cc4..eafc247f37a 100644
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
@@ -7816,15 +7816,21 @@ c_parser_label (c_parser *parser, tree std_attrs)
 
   if (c_parser_next_token_is_keyword (parser, RID_CASE))
     {
+      struct c_expr expr;
       tree exp1, exp2;
+      tree exp1o, exp2o;
       c_parser_consume_token (parser);
+      expr = c_parser_expr_no_commas (parser, NULL),
       exp1 = convert_lvalue_to_rvalue (loc1,
-				       c_parser_expr_no_commas (parser, NULL),
+				       expr,
 				       true, true).value;
+      exp1o = convert_lvalue_to_rvalue (loc1,
+					expr,
+					true, true, true).value;
       if (c_parser_next_token_is (parser, CPP_COLON))
 	{
 	  c_parser_consume_token (parser);
-	  label = do_case (loc1, exp1, NULL_TREE, std_attrs);
+	  label = do_case (loc1, exp1o, expr.original_type ? expr.original_type : TREE_TYPE (exp1o), NULL_TREE, std_attrs);
 	}
       else if (c_parser_next_token_is (parser, CPP_ELLIPSIS))
 	{
@@ -7834,7 +7840,7 @@ c_parser_label (c_parser *parser, tree std_attrs)
 								    NULL),
 					   true, true).value;
 	  if (c_parser_require (parser, CPP_COLON, "expected %<:%>"))
-	    label = do_case (loc1, exp1, exp2, std_attrs);
+	    label = do_case (loc1, exp1o, expr.original_type ? expr.original_type : TREE_TYPE (exp1o), exp2, std_attrs);
 	}
       else
 	c_parser_error (parser, "expected %<:%> or %<...%>");
@@ -7843,7 +7849,7 @@ c_parser_label (c_parser *parser, tree std_attrs)
     {
       c_parser_consume_token (parser);
       if (c_parser_require (parser, CPP_COLON, "expected %<:%>"))
-	label = do_case (loc1, NULL_TREE, NULL_TREE, std_attrs);
+	label = do_case (loc1, NULL_TREE, NULL_TREE, NULL_TREE, std_attrs);
     }
   else
     {
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index 6e40f7edf02..75e92e6f48e 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -12777,7 +12777,53 @@ tree
 do_case (location_t loc, tree low_value, tree high_value, tree attrs)
 {
   tree label = NULL_TREE;
+  tree original_low_value = low_value;
+  tree original_high_value = high_value;
+  if (low_value && TREE_CODE (low_value) != INTEGER_CST)
+    {
+      low_value = c_fully_fold (low_value, false, NULL);
+      if (TREE_CODE (low_value) == INTEGER_CST)
+	pedwarn (loc, OPT_Wpedantic,
+		 "case label is not an integer constant expression");
+    }
+
+  if (high_value && TREE_CODE (high_value) != INTEGER_CST)
+    {
+      high_value = c_fully_fold (high_value, false, NULL);
+      if (TREE_CODE (high_value) == INTEGER_CST)
+	pedwarn (input_location, OPT_Wpedantic,
+		 "case label is not an integer constant expression");
+    }
 
+  if (c_switch_stack == NULL)
+    {
+      if (low_value)
+	error_at (loc, "case label not within a switch statement");
+      else
+	error_at (loc, "%<default%> label not within a switch statement");
+      return NULL_TREE;
+    }
+
+  if (c_check_switch_jump_warnings (c_switch_stack->bindings,
+				    EXPR_LOCATION (c_switch_stack->switch_stmt),
+				    loc))
+    return NULL_TREE;
+
+  label = c_add_case_label (loc, c_switch_stack->cases,
+			    SWITCH_STMT_COND (c_switch_stack->switch_stmt),
+			    low_value, high_value,
+			    original_low_value, original_high_value, attrs);
+  if (label == error_mark_node)
+    label = NULL_TREE;
+  return label;
+}
+
+tree
+do_case (location_t loc, tree low_value, tree low_original_type, tree high_value, tree attrs)
+{
+  tree label = NULL_TREE;
+  tree original_low_value = low_value;
+  tree original_high_value = high_value;
   if (low_value && TREE_CODE (low_value) != INTEGER_CST)
     {
       low_value = c_fully_fold (low_value, false, NULL);
@@ -12810,7 +12856,8 @@ do_case (location_t loc, tree low_value, tree high_value, tree attrs)
 
   label = c_add_case_label (loc, c_switch_stack->cases,
 			    SWITCH_STMT_COND (c_switch_stack->switch_stmt),
-			    low_value, high_value, attrs);
+			    low_value, high_value,
+			    low_original_type, original_high_value, attrs);
   if (label == error_mark_node)
     label = NULL_TREE;
   return label;
diff --git a/gcc/tree.cc b/gcc/tree.cc
index eab40008e8b..4ef82052971 100644
--- a/gcc/tree.cc
+++ b/gcc/tree.cc
@@ -2795,6 +2795,26 @@ build_case_label (tree low_value, tree high_value, tree label_decl)
 
   CASE_LOW (t) = low_value;
   CASE_HIGH (t) = high_value;
+  CASE_ORIGINAL_LOW (t) = NULL_TREE;
+  CASE_ORIGINAL_HIGH (t) = NULL_TREE;
+  CASE_LABEL (t) = label_decl;
+  CASE_CHAIN (t) = NULL_TREE;
+
+  return t;
+}
+
+tree
+build_case_label (tree low_value, tree high_value, tree original_low_value, tree original_high_value, tree label_decl)
+{
+  tree t = make_node (CASE_LABEL_EXPR);
+
+  TREE_TYPE (t) = void_type_node;
+  SET_EXPR_LOCATION (t, DECL_SOURCE_LOCATION (label_decl));
+
+  CASE_LOW (t) = low_value;
+  CASE_HIGH (t) = high_value;
+  CASE_ORIGINAL_LOW (t) = original_low_value;
+  CASE_ORIGINAL_HIGH (t) = original_high_value;
   CASE_LABEL (t) = label_decl;
   CASE_CHAIN (t) = NULL_TREE;
 
diff --git a/gcc/tree.def b/gcc/tree.def
index 44871367d0d..b6213d1fa39 100644
--- a/gcc/tree.def
+++ b/gcc/tree.def
@@ -1006,7 +1006,7 @@ DEFTREECODE (SWITCH_EXPR, "switch_expr", tcc_statement, 2)
    Operand 3 is CASE_CHAIN.  This operand is only used in tree-cfg.cc to
      speed up the lookup of case labels which use a particular edge in
      the control flow graph.  */
-DEFTREECODE (CASE_LABEL_EXPR, "case_label_expr", tcc_statement, 4)
+DEFTREECODE (CASE_LABEL_EXPR, "case_label_expr", tcc_statement, 6)
 
 /* Used to represent an inline assembly statement.  ASM_STRING returns a
    STRING_CST for the instruction (e.g., "mov x, y"). ASM_OUTPUTS,
diff --git a/gcc/tree.h b/gcc/tree.h
index aaaf703186c..c7a51036a8e 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -1407,6 +1407,8 @@ class auto_suppress_location_wrappers
 #define CASE_HIGH(NODE)         	TREE_OPERAND (CASE_LABEL_EXPR_CHECK (NODE), 1)
 #define CASE_LABEL(NODE)		TREE_OPERAND (CASE_LABEL_EXPR_CHECK (NODE), 2)
 #define CASE_CHAIN(NODE)		TREE_OPERAND (CASE_LABEL_EXPR_CHECK (NODE), 3)
+#define CASE_ORIGINAL_LOW(NODE)         TREE_OPERAND (CASE_LABEL_EXPR_CHECK (NODE), 4)
+#define CASE_ORIGINAL_HIGH(NODE)        TREE_OPERAND (CASE_LABEL_EXPR_CHECK (NODE), 5)
 
 /* The operands of a TARGET_MEM_REF.  Operands 0 and 1 have to match
    corresponding MEM_REF operands.  */
@@ -4743,6 +4745,8 @@ extern tree copy_list (tree);
 
 extern tree build_case_label (tree, tree, tree);
 
+extern tree build_case_label (tree, tree, tree, tree, tree);
+
 /* Make a BINFO.  */
 extern tree make_tree_binfo (unsigned CXX_MEM_STAT_INFO);
 

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Pip Cet via Bug reports for GNU Emacs, the Swiss army knife of text editors]

"Stefan Kangas" <stefankangas@gmail.com> writes:

> Pip Cet <pipcet@protonmail.com> writes:
>
>> This isn't strictly about the scratch/igc branch, but I personally think
>> struct hashes should be checked in all builds, mismatches should be
>> downgraded to #warnings, and --enable-checking=all could include
>> -Werror=cpp.  (So the warnings would still abort a build with
>> --enable-checking=all, but they'd *also* show up in regular builds.)
>
> Sounds good to me.
>
>> Also, we should include them in the nativecomp ABI hash, as nativecomp
>> relies on struct layout and the tagging scheme in nontrivial ways
>> (most-positive-fixnum, for example, is treated as a compile-time
>> constant by comp.el, but I'm not sure all changes to it would
>> automatically affect the comp abi hash).
>
> I'm copying in Andrea for this part.
>
>> Changes required might include:
>>
>> * autodetection of gawk vs awk (I think this simply means using $(AWK)
>>   rather than "awk" in src/Makefile.in; this is important on some
>>   machines which provide a very different "awk")
>
> Using $(AWK) instead of raw "awk" makes sense, because why not.
>
> But which awk implementations are you concerned about more specifically?
> I would expect us to just use POSIX awk, without any gawk extensions.
> For example, I have had no issues building on Debian (using mawk by
> default) or macOS (with the BSD awk that descends from nawk, IIUC).
>
>> * igc.o and comp.o  should depend explicitly on dmpstruct.h (this is
>>   important for re-builds; in theory, the current scratch/igc branch is
>>   broken for highly parallel builds because dmpstruct.h might be
>>   generated after igc.o, but that is not important).
>
> Sounds good also.
>
>> * a hash-of-hashes for nativecomp
>>
>> Proposed partial patch:
>
> (I didn't look at the patch yet, though the above sounds like it might
> come out to more than one patch.)

You're right.  Let's start by fixing the build on Solaris 10.

Here's a WIP patch which *almost* fixes the build (with
--enable-checking=all) there.  The problem is that "ar" doesn't work,
and AC_PROG_AR, which I naively assumed would define @AR@, doesn't exist
either, on that system.  So you still have to build with "gmake AR=gar",
I guess.  (I also have to trick the makeinfo detection into letting me
build Emacs, but that's a well-known unrelated problem).

commit 738f12b467016e59086bad7c38f86b59b85f662e (HEAD)
Author: Pip Cet <pipcet@protonmail.com>
Date:   Thu Jan 16 13:42:03 2025 +0000

    Fix build on Solaris 10 (bug#75451)
    
    * autogen.sh: Avoid bashism.
    * configure.ac (AC_PROG_AWK): Use.
    * src/Makefile.in (AWK): Set.
    (dmpstruct.h): Use "$(AWK)", not "awk".
    * src/dired.c (DT_UNKNOWN, DT_DIR, DT_LNK): Define all three constants
    or none of them.

diff --git a/autogen.sh b/autogen.sh
index 00c20c73263..3d641b6e1f9 100755
--- a/autogen.sh
+++ b/autogen.sh
@@ -115,7 +115,7 @@ do_check=
 do_autoconf=false
 do_git=false
 
-for arg; do
+for arg in "$@"; do
     case $arg in
       --help)
 	exec echo "$0: usage: $0 [--no-check] [target...]
diff --git a/configure.ac b/configure.ac
index f78997ede24..269cbf70b60 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2004,6 +2004,8 @@ AC_DEFUN
 
 dnl Some other nice autoconf tests.
 AC_PROG_INSTALL
+dnl use "gawk", not "awk", where appropriate
+AC_PROG_AWK
 dnl These are commented out, since gl_EARLY and/or Autoconf already does them.
 dnl AC_PROG_MKDIR_P
 dnl if test "x$RANLIB" = x; then
diff --git a/src/Makefile.in b/src/Makefile.in
index 784aadd1689..d987124d29d 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -60,6 +60,7 @@ LDFLAGS =
 EXEEXT = @EXEEXT@
 version = @version@
 MKDIR_P = @MKDIR_P@
+AWK = @AWK@
 # Don't use LIBS.  configure puts stuff in it that either shouldn't be
 # linked with Emacs or is duplicated by the other stuff below.
 # LIBS = @LIBS@
@@ -543,7 +544,7 @@ pdumper.o:
 endif
 dmpstruct.h: $(srcdir)/dmpstruct.awk
 dmpstruct.h: $(libsrc)/make-fingerprint$(EXEEXT) $(dmpstruct_headers)
-	$(AM_V_GEN)POSIXLY_CORRECT=1 awk -f $(srcdir)/dmpstruct.awk \
+	$(AM_V_GEN)POSIXLY_CORRECT=1 $(AWK) -f $(srcdir)/dmpstruct.awk \
 		$(dmpstruct_headers) > $@
 
 AUTO_DEPEND = @AUTO_DEPEND@
diff --git a/src/dired.c b/src/dired.c
index 89d6033f9b9..bff83ca0c95 100644
--- a/src/dired.c
+++ b/src/dired.c
@@ -79,7 +79,15 @@ dirent_namelen (struct dirent *dp)
 }
 
 #ifndef HAVE_STRUCT_DIRENT_D_TYPE
-enum { DT_UNKNOWN, DT_DIR, DT_LNK };
+enum {
+#if !defined (DT_UNKNOWN) && !defined (DT_DIR) && !defined (DT_LNK)
+  DT_UNKNOWN,
+  DT_DIR,
+  DT_LNK,
+#elif defined (DT_UNKNOWN) || defined (DT_DIR) || defined (DT_LNK)
+#error "Cannot determine DT_UNKNOWN, DT_DIR, DT_LNK"
+#endif
+};
 #endif
 
 /* Return the file type of DP.  */

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Andrea Corallo]

Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text
editors" <bug-gnu-emacs@gnu.org> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Stefan Kangas <stefankangas@gmail.com> writes:
>>
>>>
>>>> (BTW, the contents of the #if might be out of date, not sure if we
>>>> meanwhile fix/scan additional structs.)
>>>
>>> I've now added checks for all structs that we use in "fix_*" functions
>>> (commit 76a0d739024).  I kept notes to make sure I didn't miss anything,
>>> but it's not impossible that I did.  I'll double-check things later.
>>>
>>> I plan to do anything passed to the "finalize_*" ones next, which should
>>> cover most of the ones we currently depend on, I think.
>>
>> Very nice! Thanks for taking this on!
>
> Thanks!
>
> This isn't strictly about the scratch/igc branch, but I personally think
> struct hashes should be checked in all builds, mismatches should be
> downgraded to #warnings, and --enable-checking=all could include
> -Werror=cpp.  (So the warnings would still abort a build with
> --enable-checking=all, but they'd *also* show up in regular builds.)
>
> Also, we should include them in the nativecomp ABI hash

+1

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Stefan Kangas]

Pip Cet <pipcet@protonmail.com> writes:

> You're right.  Let's start by fixing the build on Solaris 10.
>
> Here's a WIP patch which *almost* fixes the build (with
> --enable-checking=all) there.  The problem is that "ar" doesn't work,
> and AC_PROG_AR, which I naively assumed would define @AR@, doesn't exist
> either, on that system.  So you still have to build with "gmake AR=gar",
> I guess.  (I also have to trick the makeinfo detection into letting me
> build Emacs, but that's a well-known unrelated problem).

LGTM with an optional nit below.

> commit 738f12b467016e59086bad7c38f86b59b85f662e (HEAD)
> Author: Pip Cet <pipcet@protonmail.com>
> Date:   Thu Jan 16 13:42:03 2025 +0000
>
>     Fix build on Solaris 10 (bug#75451)
>
>     * autogen.sh: Avoid bashism.
>     * configure.ac (AC_PROG_AWK): Use.
>     * src/Makefile.in (AWK): Set.
>     (dmpstruct.h): Use "$(AWK)", not "awk".
>     * src/dired.c (DT_UNKNOWN, DT_DIR, DT_LNK): Define all three constants
>     or none of them.
>
> diff --git a/autogen.sh b/autogen.sh
> index 00c20c73263..3d641b6e1f9 100755
> --- a/autogen.sh
> +++ b/autogen.sh
> @@ -115,7 +115,7 @@ do_check=
>  do_autoconf=false
>  do_git=false
>
> -for arg; do
> +for arg in "$@"; do
>      case $arg in
>        --help)
>  	exec echo "$0: usage: $0 [--no-check] [target...]
> diff --git a/configure.ac b/configure.ac
> index f78997ede24..269cbf70b60 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -2004,6 +2004,8 @@ AC_DEFUN
>
>  dnl Some other nice autoconf tests.
>  AC_PROG_INSTALL
> +dnl use "gawk", not "awk", where appropriate
> +AC_PROG_AWK

Should this perhaps say

    dnl use "gawk" where possible

instead?

>  dnl These are commented out, since gl_EARLY and/or Autoconf already does them.
>  dnl AC_PROG_MKDIR_P
>  dnl if test "x$RANLIB" = x; then
> diff --git a/src/Makefile.in b/src/Makefile.in
> index 784aadd1689..d987124d29d 100644
> --- a/src/Makefile.in
> +++ b/src/Makefile.in
> @@ -60,6 +60,7 @@ LDFLAGS =
>  EXEEXT = @EXEEXT@
>  version = @version@
>  MKDIR_P = @MKDIR_P@
> +AWK = @AWK@
>  # Don't use LIBS.  configure puts stuff in it that either shouldn't be
>  # linked with Emacs or is duplicated by the other stuff below.
>  # LIBS = @LIBS@
> @@ -543,7 +544,7 @@ pdumper.o:
>  endif
>  dmpstruct.h: $(srcdir)/dmpstruct.awk
>  dmpstruct.h: $(libsrc)/make-fingerprint$(EXEEXT) $(dmpstruct_headers)
> -	$(AM_V_GEN)POSIXLY_CORRECT=1 awk -f $(srcdir)/dmpstruct.awk \
> +	$(AM_V_GEN)POSIXLY_CORRECT=1 $(AWK) -f $(srcdir)/dmpstruct.awk \
>  		$(dmpstruct_headers) > $@
>
>  AUTO_DEPEND = @AUTO_DEPEND@
> diff --git a/src/dired.c b/src/dired.c
> index 89d6033f9b9..bff83ca0c95 100644
> --- a/src/dired.c
> +++ b/src/dired.c
> @@ -79,7 +79,15 @@ dirent_namelen (struct dirent *dp)
>  }
>
>  #ifndef HAVE_STRUCT_DIRENT_D_TYPE
> -enum { DT_UNKNOWN, DT_DIR, DT_LNK };
> +enum {
> +#if !defined (DT_UNKNOWN) && !defined (DT_DIR) && !defined (DT_LNK)
> +  DT_UNKNOWN,
> +  DT_DIR,
> +  DT_LNK,
> +#elif defined (DT_UNKNOWN) || defined (DT_DIR) || defined (DT_LNK)
> +#error "Cannot determine DT_UNKNOWN, DT_DIR, DT_LNK"
> +#endif
> +};
>  #endif
>
>  /* Return the file type of DP.  */

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Pip Cet via Bug reports for GNU Emacs, the Swiss army knife of text editors]

"Stefan Kangas" <stefankangas@gmail.com> writes:

> Pip Cet <pipcet@protonmail.com> writes:
>> diff --git a/configure.ac b/configure.ac
>> index f78997ede24..269cbf70b60 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -2004,6 +2004,8 @@ AC_DEFUN
>>
>>  dnl Some other nice autoconf tests.
>>  AC_PROG_INSTALL
>> +dnl use "gawk", not "awk", where appropriate
>> +AC_PROG_AWK
>
> Should this perhaps say
>
>     dnl use "gawk" where possible
>
> instead?

Done.

I initially thought I disagreed with this, but then I saw that

https://www.gnu.org/software/gawk/

uses "gawk", not "awk", so the GNU project no longer claims the "awk"
identifier.  If that is incorrect, please let me know.

Pip

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Pip Cet via Bug reports for GNU Emacs, the Swiss army knife of text editors]

"Andrea Corallo" <acorallo@gnu.org> writes:

> Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text
> editors" <bug-gnu-emacs@gnu.org> writes:
>
>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>
>>> Stefan Kangas <stefankangas@gmail.com> writes:
>>>
>>>>
>>>>> (BTW, the contents of the #if might be out of date, not sure if we
>>>>> meanwhile fix/scan additional structs.)
>>>>
>>>> I've now added checks for all structs that we use in "fix_*" functions
>>>> (commit 76a0d739024).  I kept notes to make sure I didn't miss anything,
>>>> but it's not impossible that I did.  I'll double-check things later.
>>>>
>>>> I plan to do anything passed to the "finalize_*" ones next, which should
>>>> cover most of the ones we currently depend on, I think.
>>>
>>> Very nice! Thanks for taking this on!
>>
>> Thanks!
>>
>> This isn't strictly about the scratch/igc branch, but I personally think
>> struct hashes should be checked in all builds, mismatches should be
>> downgraded to #warnings, and --enable-checking=all could include
>> -Werror=cpp.  (So the warnings would still abort a build with
>> --enable-checking=all, but they'd *also* show up in regular builds.)
>>
>> Also, we should include them in the nativecomp ABI hash
>
> +1

Here's a patch series:

1. fix a minor bug in dmpstruct.awk: it would continue to write lines to
dmpstruct.tmp in some cases after generating the hash.  This was
harmless because the hash was already generated, but it broke my first
attempt's code.

2. generate a hash covering all structs.  It's not a hash-of-hashes, but
it does the same thing.

3. include it in the nativecomp ABI hash.  This will cause all .eln files to
be rebuilt when it is applied!  (Unless we win the lottery and hit a
hash collision, that is.  This is entirely possible because the
nativecomp hash is only 32 bits.)

4. always build dmpstruct.h.  This will avoid the problematic situation
in which Emacs builds on a system, we try to rebuild with
--enable-checking=all to catch the bug, and it won't build.

5. Generate an sed script to adjust all hashes.  I think Emacs
developers are adults (or very precocious children) and can be trusted
with this powerful tool.

6. Mark autogenerated files as autogenerated.  I see no reason not to do
that.

7. Adjust hashes (they changed because of (6)).

(Might break the DOS build.  I'll check.)

Pip

commit 40c07936b4ce1dbc6f05137d9d4f69e42b03b37c
Author: Pip Cet <pipcet@protonmail.com>

    ; * src/dmpstruct.awk: Don't print extra lines to dmpstruct.tmp

diff --git a/src/dmpstruct.awk b/src/dmpstruct.awk
index e5e359e10b7..d95552c6105 100644
--- a/src/dmpstruct.awk
+++ b/src/dmpstruct.awk
@@ -27,7 +27,7 @@ BEGIN {
   struct_name = $2
   close (tmpfile)
 }
-/^(enum|struct|union) [a-zA-Z0-9_]+([\t ]|\/\*.*\*\/)*$/, /^(  )?};$/ {
+/^(enum|struct|union) [a-zA-Z0-9_]+([\t ]|\/\*.*\*\/)*$/, /^(  )?} *(GCALIGNED_STRUCT)? *;$/ {
   print $0 > tmpfile
 }
 /^(  )?} *(GCALIGNED_STRUCT)? *;$/ {

commit 2e3ca484f870f2a09153c8cf5aec42c23da1b6a2
Author: Pip Cet <pipcet@protonmail.com>

    Cover common structs in the nativecomp ABI hash (bug#75451)
    
    * src/Makefile.in (dmpstruct.h): Always generate.  Make pdumper.o and
    comp.o depend on it.
    * src/comp.c (hash_native_abi): Include overall hash in nativecomp ABI
    hash.
    * src/dmpstruct.awk: Mark autogenerated files.  Generate a hash for
    all hashed structures.

diff --git a/src/Makefile.in b/src/Makefile.in
index d987124d29d..c06449748cd 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -539,14 +539,14 @@ .PHONY:
 
 dmpstruct_headers=$(srcdir)/lisp.h $(srcdir)/buffer.h $(srcdir)/itree.h \
 	$(srcdir)/intervals.h $(srcdir)/charset.h $(srcdir)/bignum.h
-ifeq ($(CHECK_STRUCTS),true)
-pdumper.o: dmpstruct.h
-endif
 dmpstruct.h: $(srcdir)/dmpstruct.awk
 dmpstruct.h: $(libsrc)/make-fingerprint$(EXEEXT) $(dmpstruct_headers)
 	$(AM_V_GEN)POSIXLY_CORRECT=1 $(AWK) -f $(srcdir)/dmpstruct.awk \
 		$(dmpstruct_headers) > $@
 
+pdumper.o: dmpstruct.h
+comp.o: dmpstruct.h
+
 AUTO_DEPEND = @AUTO_DEPEND@
 DEPDIR = deps
 ifeq ($(AUTO_DEPEND),yes)
diff --git a/src/comp.c b/src/comp.c
index 2603a2f4334..12743af8e01 100644
--- a/src/comp.c
+++ b/src/comp.c
@@ -40,6 +40,7 @@
 #include "md5.h"
 #include "sysstdio.h"
 #include "zlib.h"
+#include "dmpstruct.h"
 
 
 /********************************/
@@ -796,11 +797,14 @@ hash_native_abi (void)
 
   Vcomp_abi_hash =
     comp_hash_string (
-      concat3 (build_string (ABI_VERSION),
-	       concat3 (Vemacs_version, Vsystem_configuration,
-			Vsystem_configuration_options),
-	       Fmapconcat (intern_c_string ("comp--subr-signature"),
-			   Vcomp_subr_list, build_string (""))));
+      CALLN (Fconcat,
+	     build_string (ABI_VERSION),
+	     Vemacs_version,
+	     Vsystem_configuration,
+	     Vsystem_configuration_options,
+	     build_string (HASH_DMPSTRUCT),
+	     Fmapconcat (intern_c_string ("comp--subr-signature"),
+			 Vcomp_subr_list, build_string (""))));
 
   Lisp_Object version = Vemacs_version;
 
diff --git a/src/dmpstruct.awk b/src/dmpstruct.awk
index d95552c6105..f00d7fd0136 100644
--- a/src/dmpstruct.awk
+++ b/src/dmpstruct.awk
@@ -21,6 +21,8 @@ BEGIN {
   print "#define EMACS_DMPSTRUCT_H"
   struct_name = ""
   tmpfile = "dmpstruct.tmp"
+  tmpfile_all = "dmpstruct-all.tmp"
+  print "/* AUTOGENERATED. DO NOT EDIT.  This file is used for generating the HASH_DMPSTRUCT hash. */" > tmpfile_all
 }
 # Match a type followed by optional syntactic whitespace
 /^(enum|struct|union) [a-zA-Z0-9_]+([\t ]|\/\*.*\*\/)*$/ {
@@ -28,7 +30,9 @@ BEGIN {
   close (tmpfile)
 }
 /^(enum|struct|union) [a-zA-Z0-9_]+([\t ]|\/\*.*\*\/)*$/, /^(  )?} *(GCALIGNED_STRUCT)? *;$/ {
-  print $0 > tmpfile
+  print "/* AUTOGENERATED. DO NOT EDIT.  This file is used for generating a dmpstruct hash. */" > tmpfile
+  print $0 >> tmpfile
+  print $0 >> tmpfile_all
 }
 /^(  )?} *(GCALIGNED_STRUCT)? *;$/ {
   if (struct_name != "") {
@@ -41,5 +45,11 @@ BEGIN {
   }
 }
 END {
+  fflush (tmpfile_all)
+  cmd = "../lib-src/make-fingerprint -r " tmpfile_all
+  cmd | getline hash
+  close (cmd)
+  printf "#define HASH_%s_%.12s\n", "dmpstruct", hash
+  printf "#define HASH_DMPSTRUCT \"%.12s\"\n", hash
   print "#endif /* EMACS_DMPSTRUCT_H */"
 }

commit 29ce0f65c0cb9781d173c2ad5229626ffa55cc3a
Author: Pip Cet <pipcet@protonmail.com>

    Generate an SED script to adjust dmpstruct values (bug#75451)
    
    * configure.ac: Adjust config.h comment.
    * src/dmpstruct.awk: Create SED file.
    * src/Makefile.in (fix-hashes): New target.

diff --git a/configure.ac b/configure.ac
index 936afc0e7df..5ed9ae7a7fe 100644
--- a/configure.ac
+++ b/configure.ac
@@ -755,9 +755,9 @@ AC_DEFUN
     AC_DEFINE([CHECK_STRUCTS], [1],
       [Define this to check whether someone updated the portable dumper
        code after changing the layout of a structure that it uses.
-       If you change one of these structures, check that the pdumper.c
-       code is still valid, and update the pertinent hash in pdumper.c
-       by manually copying the hash from the newly-generated dmpstruct.h.])
+       If you change one of these structures, check that the relevant
+       code is still valid, and update the pertinent hash by running
+       sed -f src/dmpstruct.sed.tmp on the right file.])
   fi
   AC_SUBST([CHECK_STRUCTS])
   if test x$ac_gc_check_stringbytes != x ; then
diff --git a/src/Makefile.in b/src/Makefile.in
index c06449748cd..0402f96c42f 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -1027,3 +1027,6 @@ $(bootstrap_pdmp):
 check-syntax:
 	$(AM_V_CC)$(CC) -c $(CPPFLAGS) $(ALL_CFLAGS) ${CHK_SOURCES} || true
 .PHONY: check-syntax
+
+fix-hashes: dmpstruct.h
+	sed -f dmpstruct.sed.tmp -i pdumper.c
diff --git a/src/dmpstruct.awk b/src/dmpstruct.awk
index f00d7fd0136..00de127611f 100644
--- a/src/dmpstruct.awk
+++ b/src/dmpstruct.awk
@@ -22,7 +22,9 @@ BEGIN {
   struct_name = ""
   tmpfile = "dmpstruct.tmp"
   tmpfile_all = "dmpstruct-all.tmp"
+  sedfile = "dmpstruct.sed.tmp"
   print "/* AUTOGENERATED. DO NOT EDIT.  This file is used for generating the HASH_DMPSTRUCT hash. */" > tmpfile_all
+  print "# AUTOGENERATED. DO NOT EDIT.  This file is used for adjusting hashes." > sedfile
 }
 # Match a type followed by optional syntactic whitespace
 /^(enum|struct|union) [a-zA-Z0-9_]+([\t ]|\/\*.*\*\/)*$/ {
@@ -41,6 +43,7 @@ BEGIN {
     cmd | getline hash
     close (cmd)
     printf "#define HASH_%s_%.10s\n", struct_name, hash
+    printf "s/HASH_%s_[0-9A-Fa-f]*/HASH_%s_%.10s/g\n", struct_name, struct_name, hash >> sedfile
     struct_name = ""
   }
 }

commit 1c5505558d49962a95202a777c53cc6f2a9a75f4 (HEAD -> bug75451)
Author: Pip Cet <pipcet@protonmail.com>

    Adjust changed dmpstruct hashes (bug#75451)

diff --git a/src/pdumper.c b/src/pdumper.c
index 97d3d1412b7..a4765dfd38e 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -2000,7 +2000,7 @@ dump_pseudovector_lisp_fields (struct dump_context *ctx,
 static dump_off
 dump_cons (struct dump_context *ctx, const struct Lisp_Cons *cons)
 {
-#if CHECK_STRUCTS && !defined (HASH_Lisp_Cons_00EEE63F67)
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Cons_EF7EE95CF6)
 # error "Lisp_Cons changed. See CHECK_STRUCTS comment in config.h."
 #endif
   struct Lisp_Cons out;
@@ -2015,7 +2015,7 @@ dump_interval_tree (struct dump_context *ctx,
                     INTERVAL tree,
                     dump_off parent_offset)
 {
-#if CHECK_STRUCTS && !defined (HASH_interval_1B38941C37)
+#if CHECK_STRUCTS && !defined (HASH_interval_858621D7FE)
 # error "interval changed. See CHECK_STRUCTS comment in config.h."
 #endif
   /* TODO: output tree breadth-first?  */
@@ -2059,7 +2059,7 @@ dump_interval_tree (struct dump_context *ctx,
 static dump_off
 dump_string (struct dump_context *ctx, const struct Lisp_String *string)
 {
-#if CHECK_STRUCTS && !defined (HASH_Lisp_String_03B2DF1C8E)
+#if CHECK_STRUCTS && !defined (HASH_Lisp_String_C136309593)
 # error "Lisp_String changed. See CHECK_STRUCTS comment in config.h."
 #endif
   /* If we have text properties, write them _after_ the string so that
@@ -2105,7 +2105,7 @@ dump_string (struct dump_context *ctx, const struct Lisp_String *string)
 static dump_off
 dump_marker (struct dump_context *ctx, const struct Lisp_Marker *marker)
 {
-#if CHECK_STRUCTS && !defined (HASH_Lisp_Marker_642DBAF866)
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Marker_AC394EFE74)
 # error "Lisp_Marker changed. See CHECK_STRUCTS comment in config.h."
 #endif
 
@@ -2128,7 +2128,7 @@ dump_marker (struct dump_context *ctx, const struct Lisp_Marker *marker)
 static dump_off
 dump_interval_node (struct dump_context *ctx, struct itree_node *node)
 {
-#if CHECK_STRUCTS && !defined (HASH_itree_node_03626AFCA9)
+#if CHECK_STRUCTS && !defined (HASH_itree_node_4EBE12CCEE)
 # error "itree_node changed. See CHECK_STRUCTS comment in config.h."
 #endif
   struct itree_node out;
@@ -2170,7 +2170,7 @@ dump_interval_node (struct dump_context *ctx, struct itree_node *node)
 static dump_off
 dump_overlay (struct dump_context *ctx, const struct Lisp_Overlay *overlay)
 {
-#if CHECK_STRUCTS && !defined (HASH_Lisp_Overlay_5F9D7E02FC)
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Overlay_55E80432A2)
 # error "Lisp_Overlay changed. See CHECK_STRUCTS comment in config.h."
 #endif
   START_DUMP_PVEC (ctx, &overlay->header, struct Lisp_Overlay, out);
@@ -2202,7 +2202,7 @@ dump_field_finalizer_ref (struct dump_context *ctx,
 dump_finalizer (struct dump_context *ctx,
                 const struct Lisp_Finalizer *finalizer)
 {
-#if CHECK_STRUCTS && !defined (HASH_Lisp_Finalizer_D58E647CB8)
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Finalizer_F93576AA23)
 # error "Lisp_Finalizer changed. See CHECK_STRUCTS comment in config.h."
 #endif
   START_DUMP_PVEC (ctx, &finalizer->header, struct Lisp_Finalizer, out);
@@ -2239,7 +2239,7 @@ dump_treesit_compiled_query (struct dump_context *ctx,
 static dump_off
 dump_bignum (struct dump_context *ctx, Lisp_Object object)
 {
-#if CHECK_STRUCTS && !defined (HASH_Lisp_Bignum_661945DE2B)
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Bignum_E044A28F86)
 # error "Lisp_Bignum changed. See CHECK_STRUCTS comment in config.h."
 #endif
   const struct Lisp_Bignum *bignum = XBIGNUM (object);
@@ -2277,7 +2277,7 @@ dump_bignum (struct dump_context *ctx, Lisp_Object  static dump_off
 dump_float (struct dump_context *ctx, const struct Lisp_Float *lfloat)
 {
-#if CHECK_STRUCTS && !defined (HASH_Lisp_Float_7E7D284C02)
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Float_E025D05C8C)
 # error "Lisp_Float changed. See CHECK_STRUCTS comment in config.h."
 #endif
   eassert (ctx->header.cold_start);
@@ -2290,7 +2290,7 @@ dump_float (struct dump_context *ctx, const struct Lisp_Float *lfloat)
 static dump_off
 dump_fwd_int (struct dump_context *ctx, const struct Lisp_Intfwd *intfwd)
 {
-#if CHECK_STRUCTS && !defined HASH_Lisp_Intfwd_4D887A7387
+#if CHECK_STRUCTS && !defined HASH_Lisp_Intfwd_6E2E07BDCC
 # error "Lisp_Intfwd changed. See CHECK_STRUCTS comment in config.h."
 #endif
   dump_emacs_reloc_immediate_intmax_t (ctx, intfwd->intvar, *intfwd->intvar);
@@ -2304,7 +2304,7 @@ dump_fwd_int (struct dump_context *ctx, const struct Lisp_Intfwd *intfwd)
 static dump_off
 dump_fwd_bool (struct dump_context *ctx, const struct Lisp_Boolfwd *boolfwd)
 {
-#if CHECK_STRUCTS && !defined (HASH_Lisp_Boolfwd_0EA1C7ADCC)
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Boolfwd_4BDBCBC7D2)
 # error "Lisp_Boolfwd changed. See CHECK_STRUCTS comment in config.h."
 #endif
   dump_emacs_reloc_immediate_bool (ctx, boolfwd->boolvar, *boolfwd->boolvar);
@@ -2318,7 +2318,7 @@ dump_fwd_bool (struct dump_context *ctx, const struct Lisp_Boolfwd *boolfwd)
 static dump_off
 dump_fwd_obj (struct dump_context *ctx, const struct Lisp_Objfwd *objfwd)
 {
-#if CHECK_STRUCTS && !defined (HASH_Lisp_Objfwd_45D3E513DC)
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Objfwd_33156D0D7A)
 # error "Lisp_Objfwd changed. See CHECK_STRUCTS comment in config.h."
 #endif
   if (NILP (Fgethash (dump_off_to_lisp (emacs_offset (objfwd->objvar)),
@@ -2336,7 +2336,7 @@ dump_fwd_obj (struct dump_context *ctx, const struct Lisp_Objfwd *objfwd)
 dump_fwd_buffer_obj (struct dump_context *ctx,
                      const struct Lisp_Buffer_Objfwd *buffer_objfwd)
 {
-#if CHECK_STRUCTS && !defined (HASH_Lisp_Buffer_Objfwd_611EBD13FF)
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Buffer_Objfwd_AF869E0C7C)
 # error "Lisp_Buffer_Objfwd changed. See CHECK_STRUCTS comment in config.h."
 #endif
   struct Lisp_Buffer_Objfwd out;
@@ -2352,7 +2352,7 @@ dump_fwd_buffer_obj (struct dump_context *ctx,
 dump_fwd_kboard_obj (struct dump_context *ctx,
                      const struct Lisp_Kboard_Objfwd *kboard_objfwd)
 {
-#if CHECK_STRUCTS && !defined (HASH_Lisp_Kboard_Objfwd_CAA7E71069)
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Kboard_Objfwd_49A54EE796)
 # error "Lisp_Intfwd changed. See CHECK_STRUCTS comment in config.h."
 #endif
   struct Lisp_Kboard_Objfwd out;
@@ -2365,7 +2365,7 @@ dump_fwd_kboard_obj (struct dump_context *ctx,
 static dump_off
 dump_fwd (struct dump_context *ctx, lispfwd fwd)
 {
-#if CHECK_STRUCTS && !defined (HASH_Lisp_Fwd_Type_9CBA6EE55E)
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Fwd_Type_010C8CEF02)
 # error "Lisp_Fwd_Type changed. See CHECK_STRUCTS comment in config.h."
 #endif
   void const *p = fwd.fwdptr;
@@ -2399,7 +2399,7 @@ dump_fwd (struct dump_context *ctx, lispfwd fwd)
 dump_blv (struct dump_context *ctx,
           const struct Lisp_Buffer_Local_Value *blv)
 {
-#if CHECK_STRUCTS && !defined HASH_Lisp_Buffer_Local_Value_3C363FAC3C
+#if CHECK_STRUCTS && !defined HASH_Lisp_Buffer_Local_Value_BC9625B82F
 # error "Lisp_Buffer_Local_Value changed. See CHECK_STRUCTS comment in config.h."
 #endif
   struct Lisp_Buffer_Local_Value out;
@@ -2465,10 +2465,10 @@ dump_symbol (struct dump_context *ctx,
              Lisp_Object object,
              dump_off offset)
 {
-#if CHECK_STRUCTS && !defined HASH_Lisp_Symbol_61B174C9F4
+#if CHECK_STRUCTS && !defined HASH_Lisp_Symbol_A10971AE16
 # error "Lisp_Symbol changed. See CHECK_STRUCTS comment in config.h."
 #endif
-#if CHECK_STRUCTS && !defined (HASH_symbol_redirect_EA72E4BFF5)
+#if CHECK_STRUCTS && !defined (HASH_symbol_redirect_F62BD8F1FE)
 # error "symbol_redirect changed. See CHECK_STRUCTS comment in config.h."
 #endif
 
@@ -2562,7 +2562,7 @@ dump_symbol (struct dump_context *ctx,
 dump_vectorlike_generic (struct dump_context *ctx,
 			 const union vectorlike_header *header)
 {
-#if CHECK_STRUCTS && !defined (HASH_vectorlike_header_785E52047B)
+#if CHECK_STRUCTS && !defined (HASH_vectorlike_header_331153C5E8)
 # error "vectorlike_header changed. See CHECK_STRUCTS comment in config.h."
 #endif
   const struct Lisp_Vector *v = (const struct Lisp_Vector *) header;
@@ -2734,7 +2734,7 @@ dump_hash_table_contents (struct dump_context *ctx, struct Lisp_Hash_Table *h)
 static dump_off
 dump_hash_table (struct dump_context *ctx, Lisp_Object object)
 {
-#if CHECK_STRUCTS && !defined HASH_Lisp_Hash_Table_0360833954
+#if CHECK_STRUCTS && !defined HASH_Lisp_Hash_Table_E855664723
 # error "Lisp_Hash_Table changed. See CHECK_STRUCTS comment in config.h."
 #endif
   const struct Lisp_Hash_Table *hash_in = XHASH_TABLE (object);
@@ -2789,7 +2789,7 @@ dump_obarray_buckets (struct dump_context *ctx, const struct Lisp_Obarray *o)
 static dump_off
 dump_obarray (struct dump_context *ctx, Lisp_Object object)
 {
-#if CHECK_STRUCTS && !defined HASH_Lisp_Obarray_D2757E61AD
+#if CHECK_STRUCTS && !defined HASH_Lisp_Obarray_552E55F2A1
 # error "Lisp_Obarray changed. See CHECK_STRUCTS comment in config.h."
 #endif
   const struct Lisp_Obarray *in_oa = XOBARRAY (object);
@@ -2811,7 +2811,7 @@ dump_obarray (struct dump_context *ctx, Lisp_Object object)
 static dump_off
 dump_buffer (struct dump_context *ctx, const struct buffer *in_buffer)
 {
-#if CHECK_STRUCTS && !defined HASH_buffer_B02F648B82
+#if CHECK_STRUCTS && !defined HASH_buffer_2562CF7465
 # error "buffer changed. See CHECK_STRUCTS comment in config.h."
 #endif
   struct buffer munged_buffer = *in_buffer;
@@ -2949,7 +2949,7 @@ dump_buffer (struct dump_context *ctx, const struct buffer *in_buffer)
 static dump_off
 dump_bool_vector (struct dump_context *ctx, const struct Lisp_Vector *v)
 {
-#if CHECK_STRUCTS && !defined (HASH_Lisp_Vector_3091289B35)
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Vector_FBEF167750)
 # error "Lisp_Vector changed. See CHECK_STRUCTS comment in config.h."
 #endif
   /* No relocation needed, so we don't need dump_object_start.  */
@@ -2966,7 +2966,7 @@ dump_bool_vector (struct dump_context *ctx, const struct Lisp_Vector *v)
 static dump_off
 dump_subr (struct dump_context *ctx, const struct Lisp_Subr *subr)
 {
-#if CHECK_STRUCTS && !defined (HASH_Lisp_Subr_EE5F7351CC)
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Subr_1B888CFDDA)
 # error "Lisp_Subr changed. See CHECK_STRUCTS comment in config.h."
 #endif
   struct Lisp_Subr out;
@@ -3065,7 +3065,7 @@ dump_vectorlike (struct dump_context *ctx,
                  Lisp_Object lv,
                  dump_off offset)
 {
-#if CHECK_STRUCTS && !defined HASH_pvec_type_99104541E2
+#if CHECK_STRUCTS && !defined HASH_pvec_type_933496B918
 # error "pvec_type changed. See CHECK_STRUCTS comment in config.h."
 #endif
   const struct Lisp_Vector *v = XVECTOR (lv);
@@ -3172,7 +3172,7 @@ dump_vectorlike (struct dump_context *ctx,
 static dump_off
 dump_object (struct dump_context *ctx, Lisp_Object object)
 {
-#if CHECK_STRUCTS && !defined (HASH_Lisp_Type_45F0582FD7)
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Type_2A0F47A74F)
 # error "Lisp_Type changed. See CHECK_STRUCTS comment in config.h."
 #endif
   eassert (!EQ (object, dead_object ()));
@@ -3275,7 +3275,7 @@ dump_object_for_offset (struct dump_context *ctx, Lisp_Object object)
 static dump_off
 dump_charset (struct dump_context *ctx, int cs_i)
 {
-#if CHECK_STRUCTS && !defined (HASH_charset_E31F4B5D96)
+#if CHECK_STRUCTS && !defined (HASH_charset_DDAF793226)
 # error "charset changed. See CHECK_STRUCTS comment in config.h."
 #endif
   /* We can't change the alignment here, because ctx->offset is what

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Eli Zaretskii]

> Date: Fri, 17 Jan 2025 16:51:04 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: Gerd Möllmann <gerd.moellmann@gmail.com>, Gerd Moellmann <gerd@gnu.org>, 75451-done@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>, Andrea Corallo <acorallo@gnu.org>
> 
> I initially thought I disagreed with this, but then I saw that
> 
> https://www.gnu.org/software/gawk/
> 
> uses "gawk", not "awk", so the GNU project no longer claims the "awk"
> identifier.  If that is incorrect, please let me know.

Not sure if this answers the question, but as of the latest release
5.3.1 of Gawk, "make install" installs both 'gawk' and 'awk'.

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Stefan Kangas]

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

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

> Yes, inserting 'default: emacs_abort ();' switches from static to
> dynamic checking, which is less reliable. Although 'default: emacs_abort
> ();' is sometimes the best one can do, it's better to avoid it when
> possible.
>
> I use 'default: eassume (false);' more often than 'default: emacs_abort
> ();', though for a completely different purpose: to tell GCC something
> that it's not smart enough to figure out on its own, so that GCC can do
> better static checking elsewhere.

Thanks, that's helpful.  Based on the above, I came up with the attached
patch for pdumper.c.

But could you explain your last paragraph above?  In my attached patch,
there is one removal of 'default: eassume(false)'; how can I know if
it's worth keeping or not?

[-- Attachment #2: 0001-Prefer-static-switch-case-checks-in-pdumper.patch --]
[-- Type: text/x-patch, Size: 3932 bytes --]

From 737a5ad20adeee0a919adecf57f2ba52d81f0d78 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Mon, 20 Jan 2025 02:47:30 +0100
Subject: [PATCH] Prefer static switch-case checks in pdumper

* src/pdumper.c (dump_fwd, dump_symbol, dump_drain_cold_data)
(decode_emacs_reloc, dump_do_fixup, dump_anonymous_allocate_w32)
(dump_anonymous_allocate_posix, dump_map_file_w32, dump_map_file_posix)
(dump_do_emacs_relocation): Remove default clauses to switch from
dynamic to static checking.
---
 src/pdumper.c | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

diff --git a/src/pdumper.c b/src/pdumper.c
index e83c7bcf9a1..86576af9d8a 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -2390,8 +2390,6 @@ dump_fwd (struct dump_context *ctx, lispfwd fwd)
     case Lisp_Fwd_Kboard_Obj:
       offset = dump_fwd_kboard_obj (ctx, p);
       break;
-    default:
-      emacs_abort ();
     }
 
   return offset;
@@ -2523,8 +2521,6 @@ dump_symbol (struct dump_context *ctx,
     case SYMBOL_FORWARDED:
       dump_field_fixup_later (ctx, &out, symbol, &symbol->u.s.val.fwd);
       break;
-    default:
-      emacs_abort ();
     }
   dump_field_lv (ctx, &out, symbol, &symbol->u.s.function, WEIGHT_NORMAL);
   dump_field_lv (ctx, &out, symbol, &symbol->u.s.plist, WEIGHT_NORMAL);
@@ -3601,8 +3597,6 @@ dump_drain_cold_data (struct dump_context *ctx)
 	  dump_cold_native_subr (ctx, data);
 	  break;
 #endif
-        default:
-          emacs_abort ();
         }
     }
 
@@ -3815,7 +3809,7 @@ decode_emacs_reloc (struct dump_context *ctx, Lisp_Object lreloc)
   int type = XFIXNUM (dump_pop (&lreloc));
   reloc.emacs_offset = dump_off_from_lisp (dump_pop (&lreloc));
   dump_check_emacs_off (reloc.emacs_offset);
-  switch (type)
+  switch ((enum emacs_reloc_type) type)
     {
     case RELOC_EMACS_COPY_FROM_DUMP:
       {
@@ -3882,8 +3876,6 @@ decode_emacs_reloc (struct dump_context *ctx, Lisp_Object lreloc)
           }
       }
       break;
-    default:
-      eassume (!"not reached");
     }
 
   /* We should have consumed the whole relocation descriptor.  */
@@ -4067,8 +4059,6 @@ dump_do_fixup (struct dump_context *ctx,
         do_write = false;
         break;
       }
-    default:
-      emacs_abort ();
     }
   if (do_write)
     dump_write (ctx, &dump_value, sizeof (dump_value));
@@ -4527,8 +4517,6 @@ dump_anonymous_allocate_w32 (void *base,
       mem_type = MEM_COMMIT;
       mem_prot = PAGE_READWRITE;
       break;
-    default:
-      emacs_abort ();
     }
 
   ret = VirtualAlloc (base, size, mem_type, mem_prot);
@@ -4567,8 +4555,6 @@ dump_anonymous_allocate_posix (void *base,
     case DUMP_MEMORY_ACCESS_READWRITE:
       mem_prot = PROT_READ | PROT_WRITE;
       break;
-    default:
-      emacs_abort ();
     }
 
   int mem_flags = MAP_PRIVATE | MAP_ANONYMOUS;
@@ -4661,7 +4647,6 @@ dump_map_file_w32 (void *base, int fd, off_t offset, size_t size,
     case DUMP_MEMORY_ACCESS_READWRITE:
       protect = PAGE_WRITECOPY;	/* for Windows 9X */
       break;
-    default:
     case DUMP_MEMORY_ACCESS_NONE:
     case DUMP_MEMORY_ACCESS_READ:
       protect = PAGE_READONLY;
@@ -4689,8 +4674,6 @@ dump_map_file_w32 (void *base, int fd, off_t offset, size_t size,
     case DUMP_MEMORY_ACCESS_READWRITE:
       map_access = FILE_MAP_COPY;
       break;
-    default:
-      emacs_abort ();
     }
 
   ret = MapViewOfFileEx (section,
@@ -4733,8 +4716,6 @@ dump_map_file_posix (void *base, int fd, off_t offset, size_t size,
       mem_prot = PROT_READ | PROT_WRITE;
       mem_flags = MAP_PRIVATE;
       break;
-    default:
-      emacs_abort ();
     }
 
   if (base)
@@ -5601,8 +5582,6 @@ dump_do_emacs_relocation (const uintptr_t dump_base,
         memcpy (emacs_ptr_at (reloc.emacs_offset), &lv, sizeof (lv));
         break;
       }
-    default:
-      fatal ("unrecognied relocation type %d", (int) reloc.type);
     }
 }
 
-- 
2.48.0

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Gerd Möllmann]

Stefan Kangas <stefankangas@gmail.com> writes:

> Thanks, that's helpful.  Based on the above, I came up with the attached
> patch for pdumper.c.

LGTM

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Paul Eggert]

On 2025-01-19 18:01, Stefan Kangas wrote:
> In my attached patch,
> there is one removal of 'default: eassume(false)'; how can I know if
> it's worth keeping or not?

There are a few reasons to have such a line. First, to pacify a GCC 
false alarm. Second, to make the code clearer to a human reader. Third 
(less likely), to generate more efficient code. If no reason applies I 
would omit such lines.


>    int type = XFIXNUM (dump_pop (&lreloc));
>    reloc.emacs_offset = dump_off_from_lisp (dump_pop (&lreloc));
>    dump_check_emacs_off (reloc.emacs_offset);
> -  switch (type)
> +  switch ((enum emacs_reloc_type) type)

When it's easy, let's avoid casts like that as they're too powerful: you 
can cast pointers by mistake. Here it's easy to omit the cast, and 
declare the local variable 'type' to be of type 'enum emacs_reloc_type' 
rather than of type 'int'. (I assume you know for other reasons that the 
value is in range for the enum - otherwise the cast wouldn't be valid 
either.) In other words, just change the first line to:

    enum emacs_reloc_type type = XFIXNUM (dump_pop (&lreloc));

This way, the last line can remain unchanged and there is no need for a 
cast.

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Stefan Kangas]

Pip Cet <pipcet@protonmail.com> writes:

> Using "break" instead of "return" is impossible:
>
> * "default: eassume (0);"  means you lose compiler warnings if the enum is
> expanded.  -Wswitch-enum generates the compiler warnings, but they're
> lost in a sea of false positives because sometimes we cannot switch over
> an enum exhaustively.
>
> * "default: emacs_abort ();" will generate code to handle the "impossible"
> case
>
> * Omitting the default statement will make GCC treat the fall-through case
> as reachable
>
> This is because this code:
>
> #include <stdio.h>
>
> enum three { A, B, C };
>
> int main (void)
> {
>   volatile enum three four = C + 1;
>   volatile const char *bad = 0;
>   switch (four) {
>   case A:
>   case B:
>   case C:
>     printf ("%s\n", "good");
>     break;
>   default:
>     printf ("%s\n", bad);
>   }
>   return 0;
> }
>
> is treated by GCC as valid code and the "bad" printf cannot be
> determined to be unreachable.

I have no reason to doubt that you are correct, but what are the
consequences for us, in concrete terms, that make you say that "using
'break' instead of 'return' is impossible"?  Given that most programs
run just fine despite the shortcomings and/or bugs you have identified
in GCC, I think that saying using the switch statement in the normal way
is "impossible" might be overstating the case.

My apologies if I'm missing the point that you're making here.

BTW, has this been reported to the GCC developers?

> Let's introduce macros for the different cases, and use the same macro
> in the same case, until GCC offers a fix.

I'm with Gerd here, and not too excited about the prospect of yet
another macro.  I'd rather do without the additional warning, and just
have it crash at runtime instead, like we do now.

But are we sure that installing my patch would make things worse?
If I'm reading Paul correctly, he's saying that a static check is
more reliable.

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Stefan Kangas]

Pip Cet <pipcet@protonmail.com> writes:

> svg_css_length_to_pixels doesn't handle the RSVG_UNIT_CH case.  I don't
> know at which version rsvg started to offer this unit; it's not

It seems to have been added in 2.58:

https://gnome.pages.gitlab.gnome.org/librsvg/Rsvg-2.0/enum.Unit.html

I will create a new bug report to track this.

> indicated in the header file.  The default case:
>
>     default:
>       /* We should never reach this.  */
>       value = 0;
>
> contains an incorrect comment (the header file warns that the enum is
> incomplete and will be expanded), and incorrect code (0 is not a good
> default value, and we should warn or return an error).

We would have detected this bug sooner with the below patch, no?

diff --git a/src/image.c b/src/image.c
index b8405d81111..7fda2684460 100644
--- a/src/image.c
+++ b/src/image.c
@@ -12008,9 +12008,6 @@ svg_css_length_to_pixels (RsvgLength length,
double dpi, int font_size)
 	 what size we make the image.  */
       value = 0;
       break;
-    default:
-      /* We should never reach this.  */
-      value = 0;
     }

   return value;

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Paul Eggert]

On 2025-01-20 09:54, Stefan Kangas wrote:
> If I'm reading Paul correctly, he's saying that a static check is
> more reliable.

Yes, because a static check doesn't rely on test cases that exercise the 
dynamic check appropriately. Typically we have no such test cases.

I see Pip's point. However, I don't think it's worth coming up with a 
new macro if GCC doesn't issue the proper diagnostics. Having an 
assertion that isn't checked is not gonna maintain well.

It sounds like it'd better to get GCC fixed first, to do what we want. 
Ideally GCC would come up with something where our existing macros 
suffice or even can be reduced in number (though the macro 
implementations might need changing).

bug#75451: scratch/igc: Enable CHECK_STRUCTS

[Pip Cet via Bug reports for GNU Emacs, the Swiss army knife of text editors]

"Stefan Kangas" <stefankangas@gmail.com> writes:

> Pip Cet <pipcet@protonmail.com> writes:
>
>> Using "break" instead of "return" is impossible:
>>
>> * "default: eassume (0);"  means you lose compiler warnings if the enum is
>> expanded.  -Wswitch-enum generates the compiler warnings, but they're
>> lost in a sea of false positives because sometimes we cannot switch over
>> an enum exhaustively.
>>
>> * "default: emacs_abort ();" will generate code to handle the "impossible"
>> case
>>
>> * Omitting the default statement will make GCC treat the fall-through case
>> as reachable
>>
>> This is because this code:
>>
>> #include <stdio.h>
>>
>> enum three { A, B, C };
>>
>> int main (void)
>> {
>>   volatile enum three four = C + 1;
>>   volatile const char *bad = 0;
>>   switch (four) {
>>   case A:
>>   case B:
>>   case C:
>>     printf ("%s\n", "good");
>>     break;
>>   default:
>>     printf ("%s\n", bad);
>>   }
>>   return 0;
>> }
>>
>> is treated by GCC as valid code and the "bad" printf cannot be
>> determined to be unreachable.
>
> I have no reason to doubt that you are correct, but what are the
> consequences for us, in concrete terms, that make you say that "using
> 'break' instead of 'return' is impossible"?  Given that most programs

I'm sorry, I meant that it's impossible to replace the version that
works with one that doesn't while maintaining the right set of compiler
warnings.  Of course the code will work fine no matter which option you
use: sometimes we'll get warnings about missed cases, sometimes we
won't; sometimes, an unnecessary extra case will be generated, sometimes
it won't.

> run just fine despite the shortcomings and/or bugs you have identified
> in GCC, I think that saying using the switch statement in the normal way
> is "impossible" might be overstating the case.

It certainly is overstating the case, and I'm sorry I wasn't clearer.

What is hard (not impossible) is to get the right warnings from GCC: if
you want your switch statement to be exhaustive, you shouldn't include a
default case; if you don't have a default case, the fall-through path
will be reachable, as far as GCC is concerned, and prompt additional
warnings; if you fix that by adding "default: eassume(0);", you lose the
exhaustiveness warning; if you fix that by adding -Wswitch=enum, you get
too many false positives for enums that aren't even controlled by Emacs;
if you fix that by adding #pragma statements around all the places that
depend on external enums which might change, you have set a lingering
trap for future developers, as #pragmas tend to move away from their
intended region and disable more warnings than you want them to.  (See
bug#71744 / commit 1282714da55cd4bbc1c7f2e49edeb43503427e5e for a bug
made harder to discover by a #pragma that no longer made sense.)

After all this, you still don't get any warnings if you use enum values
in the labels but the switch parameter has an int type: if a new glyph
type is added to enum glyph_type, x_draw_glyph_string won't warn about
it.

(Yes, dispextern.h should use ENUM_BF, will come up with a patch, but
GCC should *also* have warned about the code in x_draw_glyph_string
before that).

> My apologies if I'm missing the point that you're making here.

You're not.  I was getting overexcited and forgot to carefully check
whether pragmas can be made to work.  They're ugly, but right now it
looks like the answer is "yes, if you use ENUM_BF; if you don't, tough
luck".

Thanks for calling me out on this.

> BTW, has this been reported to the GCC developers?

I've only just become aware of how bad it is.  I'll report it in due
time, after searching for previous issues.  The switch ((int) x) { case
ENUM_A: ... } thing was easy enough to fix, and I have a vague feeling
that someone out there is mixing different enums in the same switch
statement and warning about that will help them.

A related issue I don't fully understand yet is why
extern int flag; int main (void)
{
  int ret; // uninitialized
  switch (flag)
  {
    case 0: ret = 1;
  }
  return ret;
}

doesn't appear to produce a warning with -Wuninitialized or
-Wmaybe-uninitialized.

(-Wnull-dereference is even worse: at -O1, gcc doesn't complain about

extern int *flag;
int main (void)
{
  int ret; // uninitialized
  switch (*flag)
  {
    case 0: return 1;
  }
  return ret + *(int *)0;
}

in any way that I can find, and returns 1 unconditionally, ignoring the
flag entirely.  I'll report that one first, I guess).

>> Let's introduce macros for the different cases, and use the same macro
>> in the same case, until GCC offers a fix.
>
> I'm with Gerd here, and not too excited about the prospect of yet
> another macro.  I'd rather do without the additional warning, and just
> have it crash at runtime instead, like we do now.

rsvg doesn't crash, it'll just fail to display the image, IIUC.

> But are we sure that installing my patch would make things worse?

It would make things better!  I disagreed with the "think carefully
about each enum switch and choose between options, then hardcode that
one option in the code so no one can ever see the bugs you decided not
to warn about" approach, particularly if it amounts to leaving
detectable bugs for others to find.

However we decide to do this, there are only two semantically different
valid enum switches: an exhaustive one, and one that cannot be
exhaustive because we don't control the enum, or because we do but it
has too many members for us to list.

If we decide that the former (which should be the much more common case)
doesn't require a default: label, your patch is fine; we'll just have to
make that case work with GCC so it warns about non-exhaustive switches,
and doesn't generate the new spurious warnings I'm seeing here:

pdumper.c:4561:6: warning: ‘mem_prot’ may be used uninitialized [-Wmaybe-uninitialized]
 4561 |   if (mem_prot != PROT_NONE)
      |      ^

> If I'm reading Paul correctly, he's saying that a static check is
> more reliable.

I'll have to reread.  With my GCC, I don't believe that's true: where
the old code will abort, the new code will produce spurious warnings in
some cases, and fall through and leave things uninitialized without
warning about them in others, IIUC.

Pip