* bug#75451: scratch/igc: Enable CHECK_STRUCTS @ 2025-01-09 3:57 Stefan Kangas 2025-01-09 5:09 ` Gerd Möllmann 0 siblings, 1 reply; 6+ messages in thread From: Stefan Kangas @ 2025-01-09 3:57 UTC (permalink / raw) To: 75451; +Cc: Gerd Moellmann 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" ^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#75451: scratch/igc: Enable CHECK_STRUCTS 2025-01-09 3:57 bug#75451: scratch/igc: Enable CHECK_STRUCTS Stefan Kangas @ 2025-01-09 5:09 ` Gerd Möllmann 2025-01-09 7:20 ` Stefan Kangas 0 siblings, 1 reply; 6+ messages in thread From: Gerd Möllmann @ 2025-01-09 5:09 UTC (permalink / raw) To: Stefan Kangas; +Cc: Gerd Moellmann, 75451 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.) ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#75451: scratch/igc: Enable CHECK_STRUCTS 2025-01-09 5:09 ` Gerd Möllmann @ 2025-01-09 7:20 ` Stefan Kangas 2025-01-09 7:25 ` Gerd Möllmann 0 siblings, 1 reply; 6+ messages in thread From: Stefan Kangas @ 2025-01-09 7:20 UTC (permalink / raw) To: Gerd Möllmann; +Cc: Gerd Moellmann, 75451-done 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#75451: scratch/igc: Enable CHECK_STRUCTS 2025-01-09 7:20 ` Stefan Kangas @ 2025-01-09 7:25 ` Gerd Möllmann 2025-01-09 10:09 ` Pip Cet via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 6+ messages in thread From: Gerd Möllmann @ 2025-01-09 7:25 UTC (permalink / raw) To: Stefan Kangas; +Cc: Gerd Moellmann, 75451-done 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! ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#75451: scratch/igc: Enable CHECK_STRUCTS 2025-01-09 7:25 ` Gerd Möllmann @ 2025-01-09 10:09 ` Pip Cet via Bug reports for GNU Emacs, the Swiss army knife of text editors 2025-01-09 12:44 ` Stefan Kangas 0 siblings, 1 reply; 6+ messages in thread From: Pip Cet via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2025-01-09 10:09 UTC (permalink / raw) To: Gerd Möllmann; +Cc: Gerd Moellmann, Stefan Kangas, 75451-done 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#75451: scratch/igc: Enable CHECK_STRUCTS 2025-01-09 10:09 ` Pip Cet via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2025-01-09 12:44 ` Stefan Kangas 0 siblings, 0 replies; 6+ messages in thread From: Stefan Kangas @ 2025-01-09 12:44 UTC (permalink / raw) To: Pip Cet, Gerd Möllmann Cc: Gerd Moellmann, Andrea Corallo, Eli Zaretskii, 75451-done 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.) ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-01-09 12:44 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-09 3:57 bug#75451: scratch/igc: Enable CHECK_STRUCTS Stefan Kangas 2025-01-09 5:09 ` Gerd Möllmann 2025-01-09 7:20 ` Stefan Kangas 2025-01-09 7:25 ` Gerd Möllmann 2025-01-09 10:09 ` Pip Cet via Bug reports for GNU Emacs, the Swiss army knife of text editors 2025-01-09 12:44 ` Stefan Kangas
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.