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

Previous Next

To reply to this bug, email your comments to 78812 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Helmut Eller <eller.helmut <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: feature/igc: [PATCH] Make lface_id_to_name an exact root
Date: Tue, 17 Jun 2025 09:56:23 +0200

[Message part 1 (text/plain, inline)]

This is a proposal to make the variable lface_id_to_name an exact root.

[0001-Make-lface_id_to_name-an-exact-root.patch (text/x-diff, attachment)]

Message #8 received at 78812 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Helmut Eller <eller.helmut <at> gmail.com>
Cc: 78812 <at> debbugs.gnu.org
Subject: Re: bug#78812: feature/igc: [PATCH] Make lface_id_to_name an exact
 root
Date: Tue, 17 Jun 2025 15:25:18 +0000

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

> This is a proposal to make the variable lface_id_to_name an exact root.

Thank you!

I'll apply this as it is, but I'd suggest one change, which I'd like to
quickly run by you:

Some of the code in igc_xpalloc_lisp_objs_exact handles item_size !=
word_size, and some doesn't.  Would you prefer to remove the parameter
or change the code to handle multi-word items, which might be useful for
arrays of arrays of Lisp_Object?

I think we should add label arguments to all igc*alloc* functions.  With
your patch, we don't see the lface-id-to-name root until the first time
it's been resized, and it would make more sense to see it right away.

There's the general problem of memcpy atomicity, but I don't have a
solution for that; the complicated code in xpalloc_exact is still
insufficient, so we might as well remove it.

While the ordinary memcpy function in glibc will always copy large
chunks, some debug builds use byte-based replacements, and then we have
a potential problem.  This is particularly annoying because
stress-testing such builds by triggering collections from a background
thread might see false positive segfaults.  I haven't seen one in
practice, but I use such tools very rarely.

Thanks again
Pip

Message #11 received at 78812 <at> debbugs.gnu.org (full text, mbox):

From: Helmut Eller <eller.helmut <at> gmail.com>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 78812 <at> debbugs.gnu.org
Subject: Re: bug#78812: feature/igc: [PATCH] Make lface_id_to_name an exact
 root
Date: Wed, 18 Jun 2025 10:45:41 +0200

[Message part 1 (text/plain, inline)]

On Tue, Jun 17 2025, Pip Cet wrote:

> "Helmut Eller" <eller.helmut <at> gmail.com> writes:
>
>> This is a proposal to make the variable lface_id_to_name an exact root.
>
> Thank you!
>
> I'll apply this as it is, but I'd suggest one change, which I'd like to
> quickly run by you:
>
> Some of the code in igc_xpalloc_lisp_objs_exact handles item_size !=
> word_size, and some doesn't.  Would you prefer to remove the parameter
> or change the code to handle multi-word items, which might be useful for
> arrays of arrays of Lisp_Object?

Let's remove it.  The item_size is redundant here.  See the amended
patch.  I also added the missing xfree :-).

> I think we should add label arguments to all igc*alloc* functions.  With
> your patch, we don't see the lface-id-to-name root until the first time
> it's been resized, and it would make more sense to see it right away.

I added that as a separate patch.

> There's the general problem of memcpy atomicity, but I don't have a
> solution for that; the complicated code in xpalloc_exact is still
> insufficient, so we might as well remove it.
>
> While the ordinary memcpy function in glibc will always copy large
> chunks, some debug builds use byte-based replacements, and then we have
> a potential problem.  This is particularly annoying because
> stress-testing such builds by triggering collections from a background
> thread might see false positive segfaults.  I haven't seen one in
> practice, but I use such tools very rarely.

Fixing all problematic uses of memcpy would be quite tricky.  Two
obvious cases are larger_vector and copy_hash_table.  I suppose that
memcpy is also a reason why software write barriers would be a lot of
work.

[0001-Make-lface_id_to_name-an-exact-root.patch (text/x-diff, attachment)]

[0002-Require-an-explicit-label-for-igc_xalloc_lisp_objs_e.patch (text/x-diff, attachment)]

Message #14 received at 78812 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Helmut Eller <eller.helmut <at> gmail.com>
Cc: 78812 <at> debbugs.gnu.org
Subject: Re: bug#78812: feature/igc: [PATCH] Make lface_id_to_name an exact
 root
Date: Wed, 18 Jun 2025 12:27:01 +0000

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

> On Tue, Jun 17 2025, Pip Cet wrote:
>
>> "Helmut Eller" <eller.helmut <at> gmail.com> writes:
>>
>>> This is a proposal to make the variable lface_id_to_name an exact root.
>>
>> Thank you!
>>
>> I'll apply this as it is, but I'd suggest one change, which I'd like to
>> quickly run by you:
>>
>> Some of the code in igc_xpalloc_lisp_objs_exact handles item_size !=
>> word_size, and some doesn't.  Would you prefer to remove the parameter
>> or change the code to handle multi-word items, which might be useful for
>> arrays of arrays of Lisp_Object?
>
> Let's remove it.  The item_size is redundant here.  See the amended
> patch.  I also added the missing xfree :-).

Thanks.  I missed that one, and I also missed that memcpy (ptr, NULL, 0)
is undefined behavior, see bug#78824.

However, the new patch looks good on both counts.

>> I think we should add label arguments to all igc*alloc* functions.  With
>> your patch, we don't see the lface-id-to-name root until the first time
>> it's been resized, and it would make more sense to see it right away.
>
> I added that as a separate patch.

Thanks!  LGTM (not that that means much, see above).

>> There's the general problem of memcpy atomicity, but I don't have a
>> solution for that; the complicated code in xpalloc_exact is still
>> insufficient, so we might as well remove it.
>>
>> While the ordinary memcpy function in glibc will always copy large
>> chunks, some debug builds use byte-based replacements, and then we have
>> a potential problem.  This is particularly annoying because
>> stress-testing such builds by triggering collections from a background
>> thread might see false positive segfaults.  I haven't seen one in
>> practice, but I use such tools very rarely.
>
> Fixing all problematic uses of memcpy would be quite tricky.  Two
> obvious cases are larger_vector and copy_hash_table.  I suppose that
> memcpy is also a reason why software write barriers would be a lot of
> work.

So let's use memcpy for now, where there's no obvious way to do it using
a loop (and performance isn't important enough to use memcpy, of course).

I'll apply the new patch once we've confirmed that bug#78824 was the
zero-byte memcpy issue, if that's okay?

Pip

Previous Next

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