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.)