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