GNU bug report logs - #41754
[feature/native-comp] Fix crash when loading lambdas from dumps with --enable-checking.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 41754 in the body.
You can then email your comments to 41754 AT debbugs.gnu.org in the normal way.

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

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

From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [feature/native-comp] Fix crash when loading lambdas from dumps with
 --enable-checking.
Date: Sun, 7 Jun 2020 16:08:19 -0300

Hi Andrea,

I have seen that a lambda may be loaded many times when loading a dump. Is this
expected? I don't know enough to tell. If it is, the attached patch prevents an
assertion failure when building with --enable-checking.

Nico.

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

From: Andrea Corallo <akrl <at> sdf.org>
To: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
Cc: 41754 <at> debbugs.gnu.org
Subject: Re: bug#41754: [feature/native-comp] Fix crash when loading lambdas
 from dumps with --enable-checking.
Date: Sun, 07 Jun 2020 19:28:06 +0000

Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:

> Hi Andrea,
>
> I have seen that a lambda may be loaded many times when loading a dump. Is this
> expected? I don't know enough to tell. If it is, the attached patch prevents an
> assertion failure when building with --enable-checking.
>
> Nico.

Hi Nico,

the patch is missing.  Are you referring to a check in
check_comp_unit_relocs?

  Andrea

-- 
akrl <at> sdf.org

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

From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
To: 41754 <at> debbugs.gnu.org
Subject: Re: bug#41754: [feature/native-comp] Fix crash when loading lambdas
 from dumps with --enable-checking.
Date: Sun, 7 Jun 2020 16:48:16 -0300

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

Oops,

Here it is.

Nico.

[0001-Don-t-crash-when-a-lambda-has-been-already-loaded.patch (application/octet-stream, attachment)]

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

From: Andrea Corallo <akrl <at> sdf.org>
To: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
Cc: 41754 <at> debbugs.gnu.org
Subject: Re: bug#41754: [feature/native-comp] Fix crash when loading lambdas
 from dumps with --enable-checking.
Date: Mon, 08 Jun 2020 15:42:12 +0000

Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:

> Hi Andrea,
>
> I have seen that a lambda may be loaded many times when loading a dump. Is this
> expected? I don't know enough to tell. If it is, the attached patch prevents an
> assertion failure when building with --enable-checking.
>
> Nico.

Hi,

had a look, this is a real bug.  Luckily except for failing the test
should not introduce instability in normal builds, still the test is
correct.

I'll come up with the fix once is tested.

Thanks

  Andrea

-- 
akrl <at> sdf.org

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

From: Andrea Corallo <akrl <at> sdf.org>
To: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
Cc: 41754 <at> debbugs.gnu.org
Subject: Re: bug#41754: [feature/native-comp] Fix crash when loading lambdas
 from dumps with --enable-checking.
Date: Mon, 08 Jun 2020 21:30:41 +0000

Hi,

with "4784bcc * Fix load logic for the reloading CU case (bug#41754)"
this issue should be fixed.

Reverting on my local branch "e38678b268 * Reduce the number of..." I
did boot-strapped it with and without --enable-checking.

Please have a run to confirm when you can.

Thanks

  Andrea

-- 
akrl <at> sdf.org

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

From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
To: Andrea Corallo <akrl <at> sdf.org>
Cc: 41754 <at> debbugs.gnu.org
Subject: Re: bug#41754: [feature/native-comp] Fix crash when loading lambdas
 from dumps with --enable-checking.
Date: Tue, 9 Jun 2020 08:54:45 -0300

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

Hi Andrea,

I can confirm that the issue with lambdas is fixed.

I have attached three bug fixes:

- Copy the suffixes before building the heap-based list in openp. I know this is
  not the solution I proposed in the bug report, but I couldn't adapt the code
  without increasing its complexity way too much for my liking. If you think
  this is not an appropriate solution I will come up with another one.

- Fix a simple bug caused by using cl-destructuring-bind incorrectly.

- Use a C array to keep the list of native compilation units. This one fixes the
  crashes when closing Emacs. Ideally I would figure out why iterating over
  a weak hash-table does not skip elements that were already GC'd, but I could
  not do it. I feel fixing this bug is very important, so I used a C array to
  keep the list of native compilation units.

Thanks, Nico.

El lun., 8 jun. 2020 a las 18:30, Andrea Corallo (<akrl <at> sdf.org>) escribió:
>
> Hi,
>
> with "4784bcc * Fix load logic for the reloading CU case (bug#41754)"
> this issue should be fixed.
>
> Reverting on my local branch "e38678b268 * Reduce the number of..." I
> did boot-strapped it with and without --enable-checking.
>
> Please have a run to confirm when you can.
>
> Thanks
>
>   Andrea
>
> --
> akrl <at> sdf.org

[0001-Fix-usage-of-cl-destructuring-bind-in-package-delete.patch (application/octet-stream, attachment)]

[0001-Copy-suffixes-passed-to-openp-to-avoid-GC-crashes.-F.patch (application/octet-stream, attachment)]

[0001-Use-a-C-array-to-keep-the-list-of-live-native-compil.patch (application/octet-stream, attachment)]

Message #25 received at 41754-done <at> debbugs.gnu.org (full text, mbox):

From: Andrea Corallo <akrl <at> sdf.org>
To: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
Cc: 41754-done <at> debbugs.gnu.org
Subject: Re: bug#41754: [feature/native-comp] Fix crash when loading lambdas
 from dumps with --enable-checking.
Date: Tue, 09 Jun 2020 14:07:37 +0000

Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:

> Hi Andrea,
>
> I can confirm that the issue with lambdas is fixed.

Good closing this.

I pushed the `package--delete-directory' fix.  Please post the patches
you are submitting in the thread you have discussed the related issue so
they can be effectively reviewed by people you have discussed with.

  Andrea
 
-- 
akrl <at> sdf.org

Message #28 received at 41754-done <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
To: Andrea Corallo <akrl <at> sdf.org>
Cc: 41754-done <at> debbugs.gnu.org
Subject: Re: bug#41754: [feature/native-comp] Fix crash when loading lambdas
 from dumps with --enable-checking.
Date: Tue, 9 Jun 2020 11:30:27 -0300

> I pushed the `package--delete-directory' fix.  Please post the patches
> you are submitting in the thread you have discussed the related issue so
> they can be effectively reviewed by people you have discussed with.

Sorry. I apologize. I thought the discussion in those threads was over, after
having identified the bug, that's why I sent them directly to you. I didn't mean
to sidestep anyone.

Nico.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
Cc: 41754 <at> debbugs.gnu.org, akrl <at> sdf.org
Subject: Re: bug#41754: [feature/native-comp] Fix crash when loading lambdas
 from dumps with --enable-checking.
Date: Tue, 09 Jun 2020 18:02:49 +0300

> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Tue, 9 Jun 2020 08:54:45 -0300
> Cc: 41754 <at> debbugs.gnu.org
> 
> - Copy the suffixes before building the heap-based list in openp. I know this is
>   not the solution I proposed in the bug report, but I couldn't adapt the code
>   without increasing its complexity way too much for my liking. If you think
>   this is not an appropriate solution I will come up with another one.

This conses a string for each extension each time through the loop,
doesn't it?  Is that really necessary?

Maybe we should take a step back and consider restructuring the code a
bit.  AFAIU, you cons the extended_suf list to be able to use the
FOR_* loops that manipulate lists, is that correct?  If so, could it
be that removing that constraint will lead to a more elegant and less
expensive code?  After all, all this function does is to append STR to
each directory in PATH, then try finding the resulting file with or
without one of the extensions in SUFFIXES.  Could we produce the file
name to probe without walking a single list?

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

From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 41754 <at> debbugs.gnu.org, Andrea Corallo <akrl <at> sdf.org>
Subject: Re: bug#41754: [feature/native-comp] Fix crash when loading lambdas
 from dumps with --enable-checking.
Date: Tue, 9 Jun 2020 12:11:15 -0300

> This conses a string for each extension each time through the loop,
> doesn't it?  Is that really necessary?

It does. I thought that keeping the code simple (aka using the FOR_* macros)
more important than saving a few memory allocations, especially when this
function accesses the filesystem, which should be much more expensive.

> AFAIU, you cons the extended_suf list to be able to use the
> FOR_* loops that manipulate lists, is that correct?

Exactly.

> If so, could it be that removing that constraint will lead to a more elegant
> and less expensive code? After all, all this function does is to append STR to
> each directory in PATH, then try finding the resulting file with or without one
> of the extensions in SUFFIXES. Could we produce the file name to probe without
> walking a single list?

I'll come up with a new version taking your suggestions into account.

Thanks, Nico.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
Cc: 41754 <at> debbugs.gnu.org, akrl <at> sdf.org
Subject: Re: bug#41754: [feature/native-comp] Fix crash when loading lambdas
 from dumps with --enable-checking.
Date: Tue, 09 Jun 2020 18:53:39 +0300

> From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
> Date: Tue, 9 Jun 2020 12:11:15 -0300
> Cc: Andrea Corallo <akrl <at> sdf.org>, 41754 <at> debbugs.gnu.org
> 
> > If so, could it be that removing that constraint will lead to a more elegant
> > and less expensive code? After all, all this function does is to append STR to
> > each directory in PATH, then try finding the resulting file with or without one
> > of the extensions in SUFFIXES. Could we produce the file name to probe without
> > walking a single list?
> 
> I'll come up with a new version taking your suggestions into account.

Please don't see what I wrote as a requirement.  Just take it into
consideration and see if it leads to something reasonable.  If you
feel that the result will not be significantly better, there's no need
to come up with yet another solution; I'd hate to ask you to do
redundant work unless my idea really happens to lead to an elegant
solution.

And thanks for your continued work on these matters, highly
appreciated.

Previous Next

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