bug#46256: [feature/native-comp] AOT eln files ignored if run from build tree

[Andrea Corallo]

Pip Cet <pipcet@gmail.com> writes:

> On Mon, Mar 8, 2021 at 5:54 AM Pip Cet <pipcet@gmail.com> wrote:
>> Note that this might not always work because of conservative GC.
>
> If it doesn't work, can you simply retry a few times? Eventually there
> shouldn't be references to the stale native_comp_unit on the stack.
>
> However, I think I've worked out why dynlib_close doesn't do its job:
>
> Fnative_elisp_load creates a comp unit, but, if the shared library has
> already been initialized, it doesn't set that comp unit's comp_unit
> variable to point to the new comp unit; instead, it will continue
> pointing to the first comp unit which still has it open.
>
> Then, the original comp unit is unloaded but not the new one created
> by Fnative_elisp_load. We call dynlib_close() once, but we called it
> twice before, leaving the shared library open and initialized.
>
> Then, we try to load the comp unit again, and follow the stale
> comp_unit variable pointing to the original comp unit.
>
> Fix should be as attached. Note the fix is, at worst, harmless (unless
> I messed up), so we should apply it anyway just because it's good not
> to leave stale pointers lying around even if we hope that the OS
> unmaps them at some point.
>
> Pip

Hi Pip,

thanks for the analysis, I'm not sure I followed 100% so I'll repeat to
make sure we are on the same page, please correct me in case.

IIUC (and make sense to me) the issue is that we are leaving two pointer
pointing to the same handle: One is in the CU_2 allocated by
'Fnative_elisp_load' and later discarded by 'load_comp_unit' when
reloading the same filename.  The other is the original CU_1 created the
first time this filename was loaded.

When CU_2 will be GC'ed because discarded we'll get the problem because
we'll dlclose the handle.  Is this correct?

In case isn't the attached curing the issue as well?

Thanks

  Andrea

PS I couldn't reproduce using the lisp reproducer both on my 64bit both
on my 32bit system (I left it looping for a while), is that reproducer
working for you?

diff --git a/src/alloc.c b/src/alloc.c
index af08336177..0bea10610f 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -3157,8 +3157,8 @@ cleanup_vector (struct Lisp_Vector *vector)
     {
       struct Lisp_Native_Comp_Unit *cu =
        PSEUDOVEC_STRUCT (vector, Lisp_Native_Comp_Unit);
-      eassert (cu->handle);
-      dynlib_close (cu->handle);
+      if (cu->handle)
+       dynlib_close (cu->handle);
     }
   else if (NATIVE_COMP_FLAG
           && PSEUDOVECTOR_TYPEP (&vector->header, PVEC_SUBR))
diff --git a/src/comp.c b/src/comp.c
index e6f672de25..abc3535dc6 100644
--- a/src/comp.c
+++ b/src/comp.c
@@ -4832,6 +4832,10 @@ load_comp_unit (struct Lisp_Native_Comp_Unit *comp_u, 
bool loading_dump,
        We must *never* mess with static pointers in an already loaded
        eln.  */
     {
+      /* Invalidate the handle for the CU we are leaving for garbage
+        collection.  */
+      comp_u->handle = NULL;
+      /* Swap CU for the old one.  */
       comp_u_lisp_obj = *saved_cu;
       comp_u = XNATIVE_COMP_UNIT (comp_u_lisp_obj);
       comp_u->loaded_once = true;