From: Andrea Corallo <akrl@sdf.org>
To: 41357@debbugs.gnu.org
Cc: Paul Eggert <eggert@cs.ucla.edu>
Subject: bug#41357: 28.0.50; GC may miss to mark calle safe register content
Date: Sun, 17 May 2020 12:42:48 +0000 [thread overview]
Message-ID: <xjfzha6ag1j.fsf@sdf.org> (raw)
[-- Attachment #1: Type: text/plain, Size: 2561 bytes --]
Hi all,
debugging the native compiler I've been chasing a bug in a configuration
where the .eln are compiled at speed 2 (-O2) and emacs-core is compiled
at -O0.
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. The value stays in r14
till we enter into 'flush_stack_call_func' where we have to push all
registers and identify the end of the stack for mark.
We correctly push callee safe register with __builtin_unwind_init () and
we identify the top (end) of the stack on my machine using
__builtin_frame_address (0).
Here I think raise the issue, __builtin_frame_address on GCC 7 and 10
for X86_64 is returning the base pointer and not the stack pointer [1].
As a consequence this is not including the callee safe registers that we
have just pushed.
In my case r14 gets pushed at address 0x7ffc47b95fa0 but in mark_stack
we are scanning the interval 0x7ffc47b95fb0 (end) 0x7ffc47b9a150
(bottom). This because __builtin_frame_address returned ebp
(0x7ffc47b95fb0 in this case).
The consequence is that the object originally referenced by r14 is never
marked and this leads to have it freed and to a crash.
I think we would be interested into obtaining the stack pointer and not
the base pointer, unfortunately what __builtin_frame_address does is
appears not really portable:
https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html
This bug is easy to observe in the native compiler with configurations
like this (speed2 for eln -O0 for core) but I believe can affect stock
Emacs too if any caller of flush_stack_call_func has a callee safe
register holding a reference to a live object not present into the
stack. This can get trickier especially with LTO enabled.
For now I'm testing the simple attached patch that seams to do the job
for me. It pushes the registers in 'flush_stack_call_func' and then
call 'flush_stack_call_func1' where now ebp must include the address
where those register got pushed.
I hope I'm not catastrophically wrong in this analysis, in case
I apologize for the noise.
Thanks
Andrea
[1] Reduced example. GCC7 -O0
void *
foo (void)
{
__builtin_unwind_init ();
return __builtin_frame_address (0);
}
foo:
push rbp
mov rbp, rsp
push r15
push r14
push r13
push r12
push rbx
mov rax, rbp
pop rbx
pop r12
pop r13
pop r14
pop r15
pop rbp
ret
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-Garbage-Collector-for-missing-calle-safe-registe.patch --]
[-- Type: text/x-diff, Size: 1875 bytes --]
From f8a5d8a4a95182e91037d444d6d1f51f7dc7e467 Mon Sep 17 00:00:00 2001
From: Andrea Corallo <akrl@sdf.org>
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 | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/alloc.c b/src/alloc.c
index f2b80fac88..1d2ee05481 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;
@@ -5043,6 +5041,13 @@ flush_stack_call_func (void (*func) (void *arg), void *arg)
eassert (current_thread == self);
}
+void
+flush_stack_call_func (void (*func) (void *arg), void *arg)
+{
+ __builtin_unwind_init ();
+ flush_stack_call_func1 (func, arg);
+}
+
/* Determine whether it is safe to access memory at address P. */
static int
valid_pointer_p (void *p)
--
2.17.1
next reply other threads:[~2020-05-17 12:42 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-17 12:42 Andrea Corallo [this message]
2020-05-17 15:36 ` bug#41357: 28.0.50; GC may miss to mark calle safe register content Eli Zaretskii
2020-05-17 16:40 ` Andrea Corallo
2020-05-17 16:46 ` Paul Eggert
2020-05-17 17:00 ` Pip Cet
2020-05-17 19:01 ` Paul Eggert
2020-05-17 19:19 ` Eli Zaretskii
2020-05-17 20:23 ` Andrea Corallo
2020-05-17 21:03 ` Paul Eggert
2020-05-17 17:04 ` Eli Zaretskii
2020-05-17 17:21 ` Andrea Corallo
2020-05-17 17:28 ` Eli Zaretskii
2020-05-17 21:27 ` Paul Eggert
2020-05-17 21:47 ` Andrea Corallo
2020-05-17 17:04 ` Eli Zaretskii
2020-05-17 17:08 ` Paul Eggert
2020-05-17 17:24 ` Eli Zaretskii
2020-05-17 19:05 ` Paul Eggert
2020-05-17 19:26 ` Eli Zaretskii
2020-05-17 19:46 ` Andrea Corallo
2020-05-17 21:21 ` Paul Eggert
2020-05-17 17:13 ` Andrea Corallo
2020-05-17 17:22 ` Eli Zaretskii
2020-05-17 17:45 ` Andrea Corallo
2020-05-17 17:57 ` Eli Zaretskii
2020-05-17 18:16 ` Andrea Corallo
2020-05-25 2:09 ` Tom Tromey
2020-05-25 8:37 ` Andrea Corallo
2020-05-28 22:08 ` Paul Eggert
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xjfzha6ag1j.fsf@sdf.org \
--to=akrl@sdf.org \
--cc=41357@debbugs.gnu.org \
--cc=eggert@cs.ucla.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.