Subject: Re: 28.0.50; GC may miss to mark calle safe register content

From: Andrea Corallo <[email-masked]>
Date: Sun, 17 May 2020 16:40:09 +0000

Eli Zaretskii <[email-masked]> writes:

>> From: Andrea Corallo <[email-masked]>
>> Cc: Paul Eggert <[email-masked]>, Eli Zaretskii <[email-masked]>
>> Date: Sun, 17 May 2020 12:42:48 +0000
>>
>> What is going on is that in a .eln in a function A a Lisp_Object is
>> hold in a register (r14).  Function A is calling other functions into
>> emacs-core till Garbage Collection is triggered.
>>
>> Being emacs-core compiled with -O0 GCC is not selecting any callee safe
>> register and therefore these gets never pushed.
>
> Isn't this something for the infrastructure of calling
> natively-compiled Lisp to solve?  The Emacs C code isn't prepared for
> calling optimized C code when it calls Lisp, and I don't think it's
> right for us to assume that, because it will make Emacs slower.  If
> the natively-compiled Lisp needs some setup to be compatible with GC,
> I think the calling framework should set that up.

Hi Eli,

I think this is a real bug that we have in the codebase (emacs-27
included).

Usually it works because having many big functions with high register
pressure that gets activated before reaching 'flush_stack_call_func'
statistically callee saved regs are very likely to be pushed.

But nothing prevents a caller of 'flush_stack_call_func' to store a lisp
object into a callee saved regs and trigger the bug.  This obviously
depends on the compiler, flags used etc, things we have no control over.

This could be also an explanation of instability for LTO configurations.
Given that callers of 'flush_stack_call_func' are more likely to be
inlined the exposed surface becomes considerably higher.

This bug should be also more likely to be observable if C files are
compiled with a mix of -O0 and -O2.

I think we should honor calling convention and make sure we garbage
collect also the content of callee saved registers, BTW I guess that's
the reason why we call '__builtin_unwind_init' isn't?

If we are concerned about performance the attached the attached patch
should be zero performance overhead.

Regards

  Andrea

--
[email-masked]
>From ef923e029d5192b67ff7a0aede75a19c2c5327d4 Mon Sep 17 00:00:00 2001
From: Andrea Corallo <[email-masked]>
Date: Sun, 17 May 2020 13:23:59 +0100
Subject: [PATCH] * Fix Garbage Collector for missing calle safe registers
 content

	* src/alloc.c (SET_STACK_TOP_ADDRESS): Do not call
	__builtin_unwind_init.
	(flush_stack_call_func1): Rename from 'flush_stack_call_func'.
	(flush_stack_call_func): New function to spill all registers
	before calling 'flush_stack_call_func1'.  This to make sure the
	top of the stack identified includes those registers.
---
 src/alloc.c | 4 +---
 src/lisp.h  | 9 ++++++++-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index f2b80fac88..d6ba4d9790 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -4952,12 +4952,10 @@ test_setjmp (void)
 #ifdef HAVE___BUILTIN_UNWIND_INIT
 # define SET_STACK_TOP_ADDRESS(p)	\
    stacktop_sentry sentry;		\
-   __builtin_unwind_init ();		\
    *(p) = NEAR_STACK_TOP (&sentry)
 #else
 # define SET_STACK_TOP_ADDRESS(p)		\
    stacktop_sentry sentry;			\
-   __builtin_unwind_init ();			\
    test_setjmp ();				\
    sys_setjmp (sentry.j);			\
    *(p) = NEAR_STACK_TOP (&sentry + (stack_bottom < &sentry.c))
@@ -5033,7 +5031,7 @@ mark_stack (char const *bottom, char const *end)
    from FUNC.  */
 
 NO_INLINE void
-flush_stack_call_func (void (*func) (void *arg), void *arg)
+flush_stack_call_func1 (void (*func) (void *arg), void *arg)
 {
   void *end;
   struct thread_state *self = current_thread;
diff --git a/src/lisp.h b/src/lisp.h
index 66a86ddadf..c7ce3918fc 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3819,7 +3819,14 @@ verify (FLT_RADIX == 2 || FLT_RADIX == 16);
 extern void alloc_unexec_post (void);
 extern void mark_maybe_objects (Lisp_Object const *, ptrdiff_t);
 extern void mark_stack (char const *, char const *);
-extern void flush_stack_call_func (void (*func) (void *arg), void *arg);
+
+INLINE void
+flush_stack_call_func (void (*func) (void *arg), void *arg)
+{
+  __builtin_unwind_init ();
+  flush_stack_call_func1 (func, arg);
+}
+
 extern void garbage_collect (void);
 extern void maybe_garbage_collect (void);
 extern const char *pending_malloc_warning;
-- 
2.17.1