GNU bug report logs - #78686
feature/igc: [PATCH] Make charset_table an exact root

Previous Next

Full log

View this message in rfc822 format

From: Pip Cet <pipcet <at> protonmail.com>
To: Helmut Eller <eller.helmut <at> gmail.com>
Cc: 78686 <at> debbugs.gnu.org
Subject: bug#78686: feature/igc: [PATCH] Make charset_table an exact root
Date: Wed, 04 Jun 2025 13:10:12 +0000

"Helmut Eller" <eller.helmut <at> gmail.com> writes:

> Here is a proposal to move charset_table out of the GC heap and make it
> an exact root area instead.

> Previously, the charset_table was located in various places:

> 1. in charset_table_init during bootstrap (ambiguous root)
> 2. on the GC heap (loaded from the dump; pinned)
> 3. on the malloc heap when it needed to grow (ambiguous root)
>
> Now, the charset table is copied back to charset_table_init after
> loading the dump and it is scanned as an exact root area.

Thanks.  Seems like the wrong approach to me:

This removes the case we want, which is case 2, by restoring the case we
don't want, by adding new and larger unprotected roots.  Shouldn't we
simply make it so if igc is in use, we only ever use case 2?

This code doesn't handle charset_table growing above the initial size,
then dumping.  We should attempt to support unusual dumps like that.

There is a warning about making charset_table_init[] large enough so it
doesn't grow during bootstrapping, but I think that comment is obsolete
and refers to unexec problems.  It should be fine now, I think, but
there really is no reason for an optimization that applies only to
temacs, anyway.

At the very least, this deserves an emacs_abort (not an igc_assert which
might be disabled) and a comment explaining why it's not usually hit: a
silent failure and crash is a bad way of indicating to a user they need
to bump the initial charset_table_size.

Scanning only to charset_used, rather than to the end of the array, is a
questionable optimization: scanning a small number of Qnils is better
than relying on what's potentially another thread making memory
modifications so they become visible in the right order.

charset_table should move to the cold section of the dump if it's no
longer expected to be used after restoring the dump.

I don't think "clone_charset_table" is a good name for the only thing
this function does, which is to restore a dumped charset table, making
the dumped copy unsafe to use in the process.

I don't understand why it needs to assert the arena is parked.  That
simply makes future changes more difficult.

So why not just turn IGC_OBJ_DUMPED_CHARSET_TABLE into
IGC_OBJ_CHARSET_TABLE, remove some igc_asserts, and avoid roots
altogether, ensuring that root_create_charset_table is called only in
temacs, never in pdumped emacs?

Also, I think removing pinned_objects_in_dump is a separate change.

Are you looking at kbd_buffer next?  That's a very large ambiguous root,
but a little tricky, not least due to the GCC bug.

Pip

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.