unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#46988: 28.0.50; Documenting and verifying assumptions about C code not calling quit or GCing
@ 2021-03-07 13:47 Pip Cet
  2021-03-07 14:06 ` Pip Cet
  0 siblings, 1 reply; 14+ messages in thread
From: Pip Cet @ 2021-03-07 13:47 UTC (permalink / raw)
  To: 46988

This is a request for a debugging feature which would verify (at run
time or at compile time or both) that functions we know shouldn't call
GC don't call GC.

See https://lists.gnu.org/archive/html/emacs-devel/2021-03/msg00306.html

Proof-of-concept patch for a runtime check will be attached once this
has a bug number.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#46988: 28.0.50; Documenting and verifying assumptions about C code not calling quit or GCing
  2021-03-07 13:47 bug#46988: 28.0.50; Documenting and verifying assumptions about C code not calling quit or GCing Pip Cet
@ 2021-03-07 14:06 ` Pip Cet
  2021-03-08 19:42   ` Lars Ingebrigtsen
                     ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Pip Cet @ 2021-03-07 14:06 UTC (permalink / raw)
  To: 46988

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]

On Sun, Mar 7, 2021 at 1:48 PM Pip Cet <pipcet@gmail.com> wrote:
> Proof-of-concept patch for a runtime check will be attached once this
> has a bug number.

Patch attached. It assumes the standard stack growth direction, and
that __builtin_frame_address (0) is available and works. Uses GCC's
__attribute__ ((cleanup (...))).

My point here is that the technical implementation isn't the problem,
the question is whether we're disciplined enough to run with checking
enabled and react to bug reports about the fatal error being thrown.

Pip

[-- Attachment #2: 0001-Runtime-check-that-some-functions-don-t-GC-bug-46988.patch --]
[-- Type: text/x-patch, Size: 10752 bytes --]

From f5ad576ee0aa6c2b3975b0ca131aea4444c7e336 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Sun, 7 Mar 2021 13:59:32 +0000
Subject: [PATCH] Runtime check that some functions don't GC (bug#46988)

* src/alloc.c (mark_object): Mark as DONT_ALLOW_GC ().
(global_dont_allow_gc): New variable.
* src/eval.c (push_handler): Require an sp argument. All callers
changed.
(push_handler_nosignal): Require an sp argument. All callers changed.
Save sp argument in handler structure.
(unwind_to_catch): Handle dont_allow_gc stack.
(Fthrow): Mark as DONT_ALLOW_GC ().
* src/lisp.h (struct handler): Add sp field, for custom stack
unwinding.
(struct dont_allow_gc): New struct.
(dont_allow_gc_init): New function.
(dont_allow_gc_destroy): New function.
(DONT_ALLOW_GC): New macro.
(maybe_gc): Check whether GC is allowed, throw a fatal error if not.
---
 src/alloc.c        |  3 +++
 src/bytecode.c     |  3 ++-
 src/emacs-module.c |  2 +-
 src/eval.c         | 35 ++++++++++++++++++++++++-----------
 src/lisp.h         | 33 +++++++++++++++++++++++++++++++--
 src/thread.c       |  2 +-
 6 files changed, 62 insertions(+), 16 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index e72fc4c4332de..5fe62ef953117 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -6565,6 +6565,7 @@ mark_objects (Lisp_Object *obj, ptrdiff_t n)
 void
 mark_object (Lisp_Object arg)
 {
+  DONT_ALLOW_GC ();
   register Lisp_Object obj;
   void *po;
 #if GC_CHECK_MARKED_OBJECTS
@@ -7668,3 +7669,5 @@ syms_of_alloc (void)
   enum defined_HAVE_X_WINDOWS defined_HAVE_X_WINDOWS;
 } const EXTERNALLY_VISIBLE gdb_make_enums_visible = {0};
 #endif	/* __GNUC__ */
+
+struct dont_allow_gc *global_dont_allow_gc;
diff --git a/src/bytecode.c b/src/bytecode.c
index 4fd41acab856a..5a85fe81ae6da 100644
--- a/src/bytecode.c
+++ b/src/bytecode.c
@@ -776,7 +776,8 @@ #define DEFINE(name, value) LABEL (name) ,
 	  type = CONDITION_CASE;
 	pushhandler:
 	  {
-	    struct handler *c = push_handler (POP, type);
+	    struct handler *c = push_handler (POP, type,
+					      __builtin_frame_address (0));
 	    c->bytecode_dest = FETCH2;
 	    c->bytecode_top = top;
 
diff --git a/src/emacs-module.c b/src/emacs-module.c
index f8fb54c072823..63fe495b91a17 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -272,7 +272,7 @@ #define MODULE_HANDLE_NONLOCAL_EXIT(retval)                             \
   if (module_non_local_exit_check (env) != emacs_funcall_exit_return)	\
     return retval;							\
   struct handler *internal_handler =                                    \
-    push_handler_nosignal (Qt, CATCHER_ALL);                            \
+    push_handler_nosignal (Qt, CATCHER_ALL, __builtin_frame_address (0)); \
   if (!internal_handler)                                                \
     {									\
       module_out_of_memory (env);					\
diff --git a/src/eval.c b/src/eval.c
index ddaa8edd81706..b3964b51a1ed1 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -243,7 +243,7 @@ init_eval (void)
        which would otherwise leak every time we unwind back to top-level.   */
     handlerlist_sentinel = xzalloc (sizeof (struct handler));
     handlerlist = handlerlist_sentinel->nextfree = handlerlist_sentinel;
-    struct handler *c = push_handler (Qunbound, CATCHER);
+    struct handler *c = push_handler (Qunbound, CATCHER, __builtin_frame_address (0));
     eassert (c == handlerlist_sentinel);
     handlerlist_sentinel->nextfree = NULL;
     handlerlist_sentinel->next = NULL;
@@ -1178,7 +1178,7 @@ internal_catch (Lisp_Object tag,
 		Lisp_Object (*func) (Lisp_Object), Lisp_Object arg)
 {
   /* This structure is made part of the chain `catchlist'.  */
-  struct handler *c = push_handler (tag, CATCHER);
+  struct handler *c = push_handler (tag, CATCHER, __builtin_frame_address (0));
 
   if (EQ (tag, Qexit))
     minibuffer_quit_level = 0;
@@ -1258,6 +1258,9 @@ unwind_to_catch (struct handler *catch, enum nonlocal_exit type,
 
   lisp_eval_depth = catch->f_lisp_eval_depth;
 
+  void *sp = catch->sp;
+  while (global_dont_allow_gc && (void *)global_dont_allow_gc < sp)
+    global_dont_allow_gc = global_dont_allow_gc->prev;
   sys_longjmp (catch->jmp, 1);
 }
 
@@ -1267,6 +1270,7 @@ DEFUN ("throw", Fthrow, Sthrow, 2, 2, 0,
        attributes: noreturn)
   (register Lisp_Object tag, Lisp_Object value)
 {
+  DONT_ALLOW_GC ();
   struct handler *c;
 
   if (!NILP (tag))
@@ -1376,7 +1380,8 @@ internal_lisp_condition_case (Lisp_Object var, Lisp_Object bodyform,
       Lisp_Object condition = CONSP (clause) ? XCAR (clause) : Qnil;
       if (!CONSP (condition))
 	condition = list1 (condition);
-      struct handler *c = push_handler (condition, CONDITION_CASE);
+      struct handler *c = push_handler (condition, CONDITION_CASE,
+					__builtin_frame_address (0));
       if (sys_setjmp (c->jmp))
 	{
 	  Lisp_Object val = handlerlist->val;
@@ -1426,7 +1431,8 @@ internal_lisp_condition_case (Lisp_Object var, Lisp_Object bodyform,
 internal_condition_case (Lisp_Object (*bfun) (void), Lisp_Object handlers,
 			 Lisp_Object (*hfun) (Lisp_Object))
 {
-  struct handler *c = push_handler (handlers, CONDITION_CASE);
+  struct handler *c = push_handler (handlers, CONDITION_CASE,
+				    __builtin_frame_address (0));
   if (sys_setjmp (c->jmp))
     {
       Lisp_Object val = handlerlist->val;
@@ -1450,7 +1456,8 @@ internal_condition_case_1 (Lisp_Object (*bfun) (Lisp_Object), Lisp_Object arg,
 			   Lisp_Object handlers,
 			   Lisp_Object (*hfun) (Lisp_Object))
 {
-  struct handler *c = push_handler (handlers, CONDITION_CASE);
+  struct handler *c = push_handler (handlers, CONDITION_CASE,
+				    __builtin_frame_address (0));
   if (sys_setjmp (c->jmp))
     {
       Lisp_Object val = handlerlist->val;
@@ -1477,7 +1484,8 @@ internal_condition_case_2 (Lisp_Object (*bfun) (Lisp_Object, Lisp_Object),
 			   Lisp_Object handlers,
 			   Lisp_Object (*hfun) (Lisp_Object))
 {
-  struct handler *c = push_handler (handlers, CONDITION_CASE);
+  struct handler *c = push_handler (handlers, CONDITION_CASE,
+				    __builtin_frame_address (0));
   if (sys_setjmp (c->jmp))
     {
       Lisp_Object val = handlerlist->val;
@@ -1506,7 +1514,8 @@ internal_condition_case_n (Lisp_Object (*bfun) (ptrdiff_t, Lisp_Object *),
 						ptrdiff_t nargs,
 						Lisp_Object *args))
 {
-  struct handler *c = push_handler (handlers, CONDITION_CASE);
+  struct handler *c = push_handler (handlers, CONDITION_CASE,
+				    __builtin_frame_address (0));
   if (sys_setjmp (c->jmp))
     {
       Lisp_Object val = handlerlist->val;
@@ -1533,7 +1542,8 @@ internal_condition_case_n (Lisp_Object (*bfun) (ptrdiff_t, Lisp_Object *),
 internal_catch_all (Lisp_Object (*function) (void *), void *argument,
                     Lisp_Object (*handler) (enum nonlocal_exit, Lisp_Object))
 {
-  struct handler *c = push_handler_nosignal (Qt, CATCHER_ALL);
+  struct handler *c = push_handler_nosignal (Qt, CATCHER_ALL,
+					     __builtin_frame_address (0));
   if (c == NULL)
     return Qcatch_all_memory_full;
 
@@ -1555,16 +1565,18 @@ internal_catch_all (Lisp_Object (*function) (void *), void *argument,
 }
 
 struct handler *
-push_handler (Lisp_Object tag_ch_val, enum handlertype handlertype)
+push_handler (Lisp_Object tag_ch_val, enum handlertype handlertype,
+	      void *sp)
 {
-  struct handler *c = push_handler_nosignal (tag_ch_val, handlertype);
+  struct handler *c = push_handler_nosignal (tag_ch_val, handlertype, sp);
   if (!c)
     memory_full (sizeof *c);
   return c;
 }
 
 struct handler *
-push_handler_nosignal (Lisp_Object tag_ch_val, enum handlertype handlertype)
+push_handler_nosignal (Lisp_Object tag_ch_val, enum handlertype handlertype,
+		       void *sp)
 {
   struct handler *CACHEABLE c = handlerlist->nextfree;
   if (!c)
@@ -1585,6 +1597,7 @@ push_handler_nosignal (Lisp_Object tag_ch_val, enum handlertype handlertype)
   c->pdlcount = SPECPDL_INDEX ();
   c->poll_suppress_count = poll_suppress_count;
   c->interrupt_input_blocked = interrupt_input_blocked;
+  c->sp = sp;
   handlerlist = c;
   return c;
 }
diff --git a/src/lisp.h b/src/lisp.h
index b95f389b89024..9d94376b5b897 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3259,6 +3259,7 @@ SPECPDL_INDEX (void)
   ptrdiff_t pdlcount;
   int poll_suppress_count;
   int interrupt_input_blocked;
+  void *sp;
 };
 
 extern Lisp_Object memory_signal_data;
@@ -4143,8 +4144,9 @@ xsignal (Lisp_Object error_symbol, Lisp_Object data)
     (Lisp_Object (*) (ptrdiff_t, Lisp_Object *), ptrdiff_t, Lisp_Object *,
      Lisp_Object, Lisp_Object (*) (Lisp_Object, ptrdiff_t, Lisp_Object *));
 extern Lisp_Object internal_catch_all (Lisp_Object (*) (void *), void *, Lisp_Object (*) (enum nonlocal_exit, Lisp_Object));
-extern struct handler *push_handler (Lisp_Object, enum handlertype);
-extern struct handler *push_handler_nosignal (Lisp_Object, enum handlertype);
+extern struct handler *push_handler (Lisp_Object, enum handlertype, void *);
+extern struct handler *push_handler_nosignal (Lisp_Object, enum handlertype,
+					      void *);
 extern void specbind (Lisp_Object, Lisp_Object);
 extern void record_unwind_protect (void (*) (Lisp_Object), Lisp_Object);
 extern void record_unwind_protect_array (Lisp_Object *, ptrdiff_t);
@@ -5050,9 +5052,36 @@ #define FOR_EACH_ALIST_VALUE(head_var, list_var, value_var)		\
 
 /* Check whether it's time for GC, and run it if so.  */
 
+/* Do not wrap into do { } while (0). */
+
+struct dont_allow_gc;
+struct dont_allow_gc
+{
+  struct dont_allow_gc *prev;
+};
+
+extern struct dont_allow_gc *global_dont_allow_gc;
+
+INLINE void
+dont_allow_gc_init (struct dont_allow_gc *dag)
+{
+  dag->prev = global_dont_allow_gc;
+  global_dont_allow_gc = dag;
+}
+
+INLINE void
+dont_allow_gc_destroy (struct dont_allow_gc *dag)
+{
+  global_dont_allow_gc = dag->prev;
+}
+
+#define DONT_ALLOW_GC() struct dont_allow_gc __attribute__ ((cleanup (dont_allow_gc_destroy))) dont_allow_gc; dont_allow_gc_init (&dont_allow_gc)
+
 INLINE void
 maybe_gc (void)
 {
+  if (global_dont_allow_gc)
+    fatal ("GC disallowed");
   if (consing_until_gc < 0)
     maybe_garbage_collect ();
 }
diff --git a/src/thread.c b/src/thread.c
index f74f611148647..6f5deb4101032 100644
--- a/src/thread.c
+++ b/src/thread.c
@@ -748,7 +748,7 @@ run_thread (void *state)
      which would otherwise leak every time we unwind back to top-level.   */
   handlerlist_sentinel = xzalloc (sizeof (struct handler));
   handlerlist = handlerlist_sentinel->nextfree = handlerlist_sentinel;
-  struct handler *c = push_handler (Qunbound, CATCHER);
+  struct handler *c = push_handler (Qunbound, CATCHER, __builtin_frame_address (0));
   eassert (c == handlerlist_sentinel);
   handlerlist_sentinel->nextfree = NULL;
   handlerlist_sentinel->next = NULL;
-- 
2.30.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#46988: 28.0.50; Documenting and verifying assumptions about C code not calling quit or GCing
  2021-03-07 14:06 ` Pip Cet
@ 2021-03-08 19:42   ` Lars Ingebrigtsen
  2021-03-08 19:57     ` Pip Cet
  2021-03-10 18:28   ` Matt Armstrong
  2022-06-20  1:41   ` Lars Ingebrigtsen
  2 siblings, 1 reply; 14+ messages in thread
From: Lars Ingebrigtsen @ 2021-03-08 19:42 UTC (permalink / raw)
  To: Pip Cet; +Cc: 46988

Pip Cet <pipcet@gmail.com> writes:

> Patch attached. It assumes the standard stack growth direction, and
> that __builtin_frame_address (0) is available and works. Uses GCC's
> __attribute__ ((cleanup (...))).
>
> My point here is that the technical implementation isn't the problem,
> the question is whether we're disciplined enough to run with checking
> enabled and react to bug reports about the fatal error being thrown.

This is something that comes up again and again, so having
infrastructure to getting feedback faster on this stuff sounds like a
good idea to me.

Even better would be to have build-time warnings, but I guess that's
pretty much impossible?

On the other hand, even if this just gives us run-time feedback, I guess
the test suite would give some coverage here...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#46988: 28.0.50; Documenting and verifying assumptions about C code not calling quit or GCing
  2021-03-08 19:42   ` Lars Ingebrigtsen
@ 2021-03-08 19:57     ` Pip Cet
  2021-03-09 14:05       ` Lars Ingebrigtsen
  0 siblings, 1 reply; 14+ messages in thread
From: Pip Cet @ 2021-03-08 19:57 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 46988

On Mon, Mar 8, 2021 at 7:42 PM Lars Ingebrigtsen <larsi@gnus.org> wrote:
> Pip Cet <pipcet@gmail.com> writes:
>
> > Patch attached. It assumes the standard stack growth direction, and
> > that __builtin_frame_address (0) is available and works. Uses GCC's
> > __attribute__ ((cleanup (...))).
> >
> > My point here is that the technical implementation isn't the problem,
> > the question is whether we're disciplined enough to run with checking
> > enabled and react to bug reports about the fatal error being thrown.
>
> This is something that comes up again and again, so having
> infrastructure to getting feedback faster on this stuff sounds like a
> good idea to me.
>
> Even better would be to have build-time warnings, but I guess that's
> pretty much impossible?

It would be very easy if we could categorize our functions into "don't
call this unless you can accept GC" and "I won't GC", but there's a
third category, "depending on my arguments, I may or may not call GC"
(and, of course, a fourth category, "uncategorized").

It's easy to warn about category 1 functions being called from
category 2 functions, but category 3 ruins everything.

I'm very impressed with gcc's -fanalyzer (in conjunction with -flto)
If you have 20 GB of RAM and 15 minutes to spare, you can get it to
find a few places in the Emacs sources that really do look suspicious
and should be fixed to more obviously not dereference NULL.

Essentially, it does a lot of hard work trying to prove your code is
okay; but, if it can't, it will warn even though it might be (and, in
the case of Emacs, is) wrong.

So "all" we'd have to do is to teach it about some category 1
functions, some category 2 functions, and have it prove there's no
valid control flow from the second to the first. If it can't, we just
might have to accept that our arguments are not obvious enough, and
make them explicit enough for the analyzer to understand.

But I haven't figured out how to do that, yet. My suspicion is the
analyzer works on local variables and ignores the state of global
variables, and our state vars would have to be global.

And I realize I'm poking a hornet's nest, but what this really is is
dynamic scope, which might be easier for the analyzer to grok than
general global variables... hmm.

In summary, I think we can install the run-time check, and when the
analyzer is ready, we'll automatically get compile-time warnings! Even
though it might require a terabyte of RAM!

I also like the way we can build specbind-free dynamic C bindings
using stack structures, global variables, __attribute__((cleanup)),
and some code in unwind_to_catch, but of course that's a GCC extension
and not something we can do outside the realm of debugging.

Pip





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#46988: 28.0.50; Documenting and verifying assumptions about C code not calling quit or GCing
  2021-03-08 19:57     ` Pip Cet
@ 2021-03-09 14:05       ` Lars Ingebrigtsen
  0 siblings, 0 replies; 14+ messages in thread
From: Lars Ingebrigtsen @ 2021-03-09 14:05 UTC (permalink / raw)
  To: Pip Cet; +Cc: 46988

Pip Cet <pipcet@gmail.com> writes:

> In summary, I think we can install the run-time check, and when the
> analyzer is ready, we'll automatically get compile-time warnings! Even
> though it might require a terabyte of RAM!

Makes sense to me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#46988: 28.0.50; Documenting and verifying assumptions about C code not calling quit or GCing
  2021-03-07 14:06 ` Pip Cet
  2021-03-08 19:42   ` Lars Ingebrigtsen
@ 2021-03-10 18:28   ` Matt Armstrong
  2021-03-10 19:09     ` Pip Cet
  2022-06-20  1:41   ` Lars Ingebrigtsen
  2 siblings, 1 reply; 14+ messages in thread
From: Matt Armstrong @ 2021-03-10 18:28 UTC (permalink / raw)
  To: Pip Cet, 46988

Pip Cet <pipcet@gmail.com> writes:

Hey Pip, just minor comments from me.

(I assume you already plan to put the use of the cleanup attribute
behind conditional macros for portability)

Why a linked list/stack implementation?  How about a global count var
that is incremented, decremented, and asserted zero?

You wrote that this impl depends on the stack direction, but I can't
figure out why.  If it is indeed the case, add a comment explaining
this?

As a macro name, I think something like ASSERT_NO_GC_IN_SCOPE would be
clearer.

Signaling that this is a "magical" scope based construct is useful
because this sort of thing is so unusual in C.  The first thing I looked
for was an "END_SCOPE" macro and started scratching my head.

I'd also use a clear that indicates a debug time check (ASSERT,
CRASH_IF, etc.)

"Don't allow" states an invariant but does not clearly indicate a
consequence or other intent.  It could imply something as polite as "GC
is disabled for this scope".

For the C level stuff, maybe call it gc_forbidden_scope?


> + /* Do not wrap into do { } while (0). */

Move the comment next to the #define.  Ideally, don't issue a command
for the next programmer but instead explain why the code is the way is.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#46988: 28.0.50; Documenting and verifying assumptions about C code not calling quit or GCing
  2021-03-10 18:28   ` Matt Armstrong
@ 2021-03-10 19:09     ` Pip Cet
  2021-03-11 23:17       ` Matt Armstrong
  0 siblings, 1 reply; 14+ messages in thread
From: Pip Cet @ 2021-03-10 19:09 UTC (permalink / raw)
  To: Matt Armstrong; +Cc: 46988

On Wed, Mar 10, 2021 at 6:28 PM Matt Armstrong <matt@rfc20.org> wrote:
> Pip Cet <pipcet@gmail.com> writes:
>
> Hey Pip, just minor comments from me.

Thanks (I'm responding to my email in a LIFO fashion after receiving a
new computer)!

> (I assume you already plan to put the use of the cleanup attribute
> behind conditional macros for portability)

Yes, absolutely.

> Why a linked list/stack implementation?  How about a global count var
> that is incremented, decremented, and asserted zero?

Emacs' specpdl implementation is independent of, and probably
predates, the widespread availability of C exceptions. In fact, as it
is written today, C exceptions will not work in Emacs C code (neither
will C++ exceptions), because Emacs uses setjmp / longjmp instead, and
unwinds the stack by itself. Since I was too lazy to fix it to use
compatible unwinding routines on supported platforms, I simply created
a linked list on the stack, unwinding it in unwind_to_catch.

> You wrote that this impl depends on the stack direction, but I can't
> figure out why.  If it is indeed the case, add a comment explaining
> this?

The unwind_to_catch code checks how far up the stack it has to go by
comparing the stack pointer to the address of a local variable. You're
absolutely right about the comment.

> As a macro name, I think something like ASSERT_NO_GC_IN_SCOPE would be
> clearer.

Thanks! You're quite right, I chose that as a placeholder, and
explicitly mentioning the scope is a really good idea.

> Signaling that this is a "magical" scope based construct is useful
> because this sort of thing is so unusual in C.  The first thing I looked
> for was an "END_SCOPE" macro and started scratching my head.
>
> I'd also use a clear that indicates a debug time check (ASSERT,
> CRASH_IF, etc.)
>
> "Don't allow" states an invariant but does not clearly indicate a
> consequence or other intent.  It could imply something as polite as "GC
> is disabled for this scope".
>
> For the C level stuff, maybe call it gc_forbidden_scope?

I like your proposed names!

> > + /* Do not wrap into do { } while (0). */
>
> Move the comment next to the #define.  Ideally, don't issue a command
> for the next programmer but instead explain why the code is the way is.

Yes, you're right. It's impolite at best. I'll try to avoid that in future.

Thanks again
Pip





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#46988: 28.0.50; Documenting and verifying assumptions about C code not calling quit or GCing
  2021-03-10 19:09     ` Pip Cet
@ 2021-03-11 23:17       ` Matt Armstrong
  0 siblings, 0 replies; 14+ messages in thread
From: Matt Armstrong @ 2021-03-11 23:17 UTC (permalink / raw)
  To: Pip Cet; +Cc: 46988

Pip Cet <pipcet@gmail.com> writes:

> On Wed, Mar 10, 2021 at 6:28 PM Matt Armstrong <matt@rfc20.org> wrote:
>> Why a linked list/stack implementation?  How about a global count var
>> that is incremented, decremented, and asserted zero?
>
> Emacs' specpdl implementation is independent of, and probably
> predates, the widespread availability of C exceptions. In fact, as it
> is written today, C exceptions will not work in Emacs C code (neither
> will C++ exceptions), because Emacs uses setjmp / longjmp instead, and
> unwinds the stack by itself. Since I was too lazy to fix it to use
> compatible unwinding routines on supported platforms, I simply created
> a linked list on the stack, unwinding it in unwind_to_catch.
>
>> You wrote that this impl depends on the stack direction, but I can't
>> figure out why.  If it is indeed the case, add a comment explaining
>> this?
>
> The unwind_to_catch code checks how far up the stack it has to go by
> comparing the stack pointer to the address of a local variable. You're
> absolutely right about the comment.

Ah, makes perfect sense.  Thanks for this and your other responses.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#46988: 28.0.50; Documenting and verifying assumptions about C code not calling quit or GCing
  2021-03-07 14:06 ` Pip Cet
  2021-03-08 19:42   ` Lars Ingebrigtsen
  2021-03-10 18:28   ` Matt Armstrong
@ 2022-06-20  1:41   ` Lars Ingebrigtsen
  2022-06-20 11:47     ` Eli Zaretskii
  2 siblings, 1 reply; 14+ messages in thread
From: Lars Ingebrigtsen @ 2022-06-20  1:41 UTC (permalink / raw)
  To: Pip Cet; +Cc: 46988, Mattias Engdegård, Stefan Monnier

Pip Cet <pipcet@gmail.com> writes:

> Patch attached. It assumes the standard stack growth direction, and
> that __builtin_frame_address (0) is available and works. Uses GCC's
> __attribute__ ((cleanup (...))).
>
> My point here is that the technical implementation isn't the problem,
> the question is whether we're disciplined enough to run with checking
> enabled and react to bug reports about the fatal error being thrown.

I've respun the patch for the current trunk, and I wonder whether
anybody has any comments here (so I've added Stefan and Mattias to the
CCs).

I think if we add this, it should be enabled only if the build is
configured with --enable-checking.

diff --git a/src/alloc.c b/src/alloc.c
index 55e18ecd77..276267ef10 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -7019,6 +7019,7 @@ #define CHECK_ALLOCATED_AND_LIVE_SYMBOL()		((void) 0)
 void
 mark_object (Lisp_Object obj)
 {
+  DONT_ALLOW_GC ();
   ptrdiff_t sp = mark_stk.sp;
   mark_stack_push_value (obj);
   process_mark_stack (sp);
@@ -7921,3 +7922,5 @@ syms_of_alloc (void)
   enum defined_HAVE_PGTK defined_HAVE_PGTK;
 } const EXTERNALLY_VISIBLE gdb_make_enums_visible = {0};
 #endif	/* __GNUC__ */
+
+struct dont_allow_gc *global_dont_allow_gc;
diff --git a/src/bytecode.c b/src/bytecode.c
index fa068e1ec6..6d3b3fdb98 100644
--- a/src/bytecode.c
+++ b/src/bytecode.c
@@ -958,7 +958,8 @@ #define DEFINE(name, value) [name] = &&insn_ ## name,
 	  type = CONDITION_CASE;
 	pushhandler:
 	  {
-	    struct handler *c = push_handler (POP, type);
+	    struct handler *c = push_handler (POP, type,
+					      __builtin_frame_address (0));
 	    c->bytecode_dest = FETCH2;
 	    c->bytecode_top = top;
 
diff --git a/src/emacs-module.c b/src/emacs-module.c
index 1c392d65df..87d9fe070a 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -272,7 +272,7 @@ #define MODULE_HANDLE_NONLOCAL_EXIT(retval)                             \
   if (module_non_local_exit_check (env) != emacs_funcall_exit_return)	\
     return retval;							\
   struct handler *internal_handler =                                    \
-    push_handler_nosignal (Qt, CATCHER_ALL);                            \
+    push_handler_nosignal (Qt, CATCHER_ALL, __builtin_frame_address (0)); \
   if (!internal_handler)                                                \
     {									\
       module_out_of_memory (env);					\
diff --git a/src/eval.c b/src/eval.c
index 346dff8bdc..f04b814c0e 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -236,7 +236,7 @@ init_eval (void)
        which would otherwise leak every time we unwind back to top-level.   */
     handlerlist_sentinel = xzalloc (sizeof (struct handler));
     handlerlist = handlerlist_sentinel->nextfree = handlerlist_sentinel;
-    struct handler *c = push_handler (Qunbound, CATCHER);
+    struct handler *c = push_handler (Qunbound, CATCHER, __builtin_frame_address (0));
     eassert (c == handlerlist_sentinel);
     handlerlist_sentinel->nextfree = NULL;
     handlerlist_sentinel->next = NULL;
@@ -1200,7 +1200,7 @@ internal_catch (Lisp_Object tag,
 		Lisp_Object (*func) (Lisp_Object), Lisp_Object arg)
 {
   /* This structure is made part of the chain `catchlist'.  */
-  struct handler *c = push_handler (tag, CATCHER);
+  struct handler *c = push_handler (tag, CATCHER, __builtin_frame_address (0));
 
   /* Call FUNC.  */
   if (! sys_setjmp (c->jmp))
@@ -1274,6 +1274,9 @@ unwind_to_catch (struct handler *catch, enum nonlocal_exit type,
   lisp_eval_depth = catch->f_lisp_eval_depth;
   set_act_rec (current_thread, catch->act_rec);
 
+  void *sp = catch->sp;
+  while (global_dont_allow_gc && (void *)global_dont_allow_gc < sp)
+    global_dont_allow_gc = global_dont_allow_gc->prev;
   sys_longjmp (catch->jmp, 1);
 }
 
@@ -1283,6 +1286,7 @@ DEFUN ("throw", Fthrow, Sthrow, 2, 2, 0,
        attributes: noreturn)
   (register Lisp_Object tag, Lisp_Object value)
 {
+  DONT_ALLOW_GC ();
   struct handler *c;
 
   if (!NILP (tag))
@@ -1405,7 +1409,8 @@ internal_lisp_condition_case (Lisp_Object var, Lisp_Object bodyform,
       Lisp_Object condition = CONSP (clause) ? XCAR (clause) : Qnil;
       if (!CONSP (condition))
 	condition = list1 (condition);
-      struct handler *c = push_handler (condition, CONDITION_CASE);
+      struct handler *c = push_handler (condition, CONDITION_CASE,
+					__builtin_frame_address (0));
       if (sys_setjmp (c->jmp))
 	{
 	  Lisp_Object val = handlerlist->val;
@@ -1472,7 +1477,8 @@ internal_lisp_condition_case (Lisp_Object var, Lisp_Object bodyform,
 internal_condition_case (Lisp_Object (*bfun) (void), Lisp_Object handlers,
 			 Lisp_Object (*hfun) (Lisp_Object))
 {
-  struct handler *c = push_handler (handlers, CONDITION_CASE);
+  struct handler *c = push_handler (handlers, CONDITION_CASE,
+				    __builtin_frame_address (0));
   if (sys_setjmp (c->jmp))
     {
       Lisp_Object val = handlerlist->val;
@@ -1496,7 +1502,8 @@ internal_condition_case_1 (Lisp_Object (*bfun) (Lisp_Object), Lisp_Object arg,
 			   Lisp_Object handlers,
 			   Lisp_Object (*hfun) (Lisp_Object))
 {
-  struct handler *c = push_handler (handlers, CONDITION_CASE);
+  struct handler *c = push_handler (handlers, CONDITION_CASE,
+				    __builtin_frame_address (0));
   if (sys_setjmp (c->jmp))
     {
       Lisp_Object val = handlerlist->val;
@@ -1523,7 +1530,8 @@ internal_condition_case_2 (Lisp_Object (*bfun) (Lisp_Object, Lisp_Object),
 			   Lisp_Object handlers,
 			   Lisp_Object (*hfun) (Lisp_Object))
 {
-  struct handler *c = push_handler (handlers, CONDITION_CASE);
+  struct handler *c = push_handler (handlers, CONDITION_CASE,
+				    __builtin_frame_address (0));
   if (sys_setjmp (c->jmp))
     {
       Lisp_Object val = handlerlist->val;
@@ -1552,7 +1560,8 @@ internal_condition_case_n (Lisp_Object (*bfun) (ptrdiff_t, Lisp_Object *),
 						ptrdiff_t nargs,
 						Lisp_Object *args))
 {
-  struct handler *c = push_handler (handlers, CONDITION_CASE);
+  struct handler *c = push_handler (handlers, CONDITION_CASE,
+				    __builtin_frame_address (0));
   if (sys_setjmp (c->jmp))
     {
       Lisp_Object val = handlerlist->val;
@@ -1579,7 +1588,8 @@ internal_condition_case_n (Lisp_Object (*bfun) (ptrdiff_t, Lisp_Object *),
 internal_catch_all (Lisp_Object (*function) (void *), void *argument,
                     Lisp_Object (*handler) (enum nonlocal_exit, Lisp_Object))
 {
-  struct handler *c = push_handler_nosignal (Qt, CATCHER_ALL);
+  struct handler *c = push_handler_nosignal (Qt, CATCHER_ALL,
+					     __builtin_frame_address (0));
   if (c == NULL)
     return Qcatch_all_memory_full;
 
@@ -1601,16 +1611,18 @@ internal_catch_all (Lisp_Object (*function) (void *), void *argument,
 }
 
 struct handler *
-push_handler (Lisp_Object tag_ch_val, enum handlertype handlertype)
+push_handler (Lisp_Object tag_ch_val, enum handlertype handlertype,
+	      void *sp)
 {
-  struct handler *c = push_handler_nosignal (tag_ch_val, handlertype);
+  struct handler *c = push_handler_nosignal (tag_ch_val, handlertype, sp);
   if (!c)
     memory_full (sizeof *c);
   return c;
 }
 
 struct handler *
-push_handler_nosignal (Lisp_Object tag_ch_val, enum handlertype handlertype)
+push_handler_nosignal (Lisp_Object tag_ch_val, enum handlertype handlertype,
+		       void *sp)
 {
   struct handler *CACHEABLE c = handlerlist->nextfree;
   if (!c)
@@ -1635,6 +1647,7 @@ push_handler_nosignal (Lisp_Object tag_ch_val, enum handlertype handlertype)
 #ifdef HAVE_X_WINDOWS
   c->x_error_handler_depth = x_error_message_count;
 #endif
+  c->sp = sp;
   handlerlist = c;
   return c;
 }
diff --git a/src/lisp.h b/src/lisp.h
index 05b0754ff6..f15abb4519 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3649,6 +3649,7 @@ record_in_backtrace (Lisp_Object function, Lisp_Object *args, ptrdiff_t nargs)
 #ifdef HAVE_X_WINDOWS
   int x_error_handler_depth;
 #endif
+  void *sp;
 };
 
 extern Lisp_Object memory_signal_data;
@@ -4560,9 +4561,10 @@ xsignal (Lisp_Object error_symbol, Lisp_Object data)
     (Lisp_Object (*) (ptrdiff_t, Lisp_Object *), ptrdiff_t, Lisp_Object *,
      Lisp_Object, Lisp_Object (*) (Lisp_Object, ptrdiff_t, Lisp_Object *));
 extern Lisp_Object internal_catch_all (Lisp_Object (*) (void *), void *, Lisp_Object (*) (enum nonlocal_exit, Lisp_Object));
-extern struct handler *push_handler (Lisp_Object, enum handlertype)
+extern struct handler *push_handler (Lisp_Object, enum handlertype, void *)
   ATTRIBUTE_RETURNS_NONNULL;
-extern struct handler *push_handler_nosignal (Lisp_Object, enum handlertype);
+extern struct handler *push_handler_nosignal (Lisp_Object, enum handlertype,
+					      void *);
 extern void specbind (Lisp_Object, Lisp_Object);
 extern void record_unwind_protect (void (*) (Lisp_Object), Lisp_Object);
 extern void record_unwind_protect_array (Lisp_Object *, ptrdiff_t);
@@ -5541,9 +5543,36 @@ #define FOR_EACH_ALIST_VALUE(head_var, list_var, value_var)		\
 
 /* Check whether it's time for GC, and run it if so.  */
 
+/* Do not wrap into do { } while (0). */
+
+struct dont_allow_gc;
+struct dont_allow_gc
+{
+  struct dont_allow_gc *prev;
+};
+
+extern struct dont_allow_gc *global_dont_allow_gc;
+
+INLINE void
+dont_allow_gc_init (struct dont_allow_gc *dag)
+{
+  dag->prev = global_dont_allow_gc;
+  global_dont_allow_gc = dag;
+}
+
+INLINE void
+dont_allow_gc_destroy (struct dont_allow_gc *dag)
+{
+  global_dont_allow_gc = dag->prev;
+}
+
+#define DONT_ALLOW_GC() struct dont_allow_gc __attribute__ ((cleanup (dont_allow_gc_destroy))) dont_allow_gc; dont_allow_gc_init (&dont_allow_gc)
+
 INLINE void
 maybe_gc (void)
 {
+  if (global_dont_allow_gc)
+    fatal ("GC disallowed");
   if (consing_until_gc < 0)
     maybe_garbage_collect ();
 }
diff --git a/src/thread.c b/src/thread.c
index 626d14aad0..e172785a64 100644
--- a/src/thread.c
+++ b/src/thread.c
@@ -779,7 +779,7 @@ run_thread (void *state)
      which would otherwise leak every time we unwind back to top-level.   */
   handlerlist_sentinel = xzalloc (sizeof (struct handler));
   handlerlist = handlerlist_sentinel->nextfree = handlerlist_sentinel;
-  struct handler *c = push_handler (Qunbound, CATCHER);
+  struct handler *c = push_handler (Qunbound, CATCHER, __builtin_frame_address (0));
   eassert (c == handlerlist_sentinel);
   handlerlist_sentinel->nextfree = NULL;
   handlerlist_sentinel->next = NULL;


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#46988: 28.0.50; Documenting and verifying assumptions about C code not calling quit or GCing
  2022-06-20  1:41   ` Lars Ingebrigtsen
@ 2022-06-20 11:47     ` Eli Zaretskii
  2022-06-23 15:56       ` Pip Cet
  0 siblings, 1 reply; 14+ messages in thread
From: Eli Zaretskii @ 2022-06-20 11:47 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 46988, mattiase, monnier, pipcet

> Cc: 46988@debbugs.gnu.org,
>  Mattias Engdegård <mattiase@acm.org>,
>  Stefan Monnier <monnier@iro.umontreal.ca>
> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 20 Jun 2022 03:41:42 +0200
> 
> Pip Cet <pipcet@gmail.com> writes:
> 
> > Patch attached. It assumes the standard stack growth direction, and
> > that __builtin_frame_address (0) is available and works. Uses GCC's
> > __attribute__ ((cleanup (...))).
> >
> > My point here is that the technical implementation isn't the problem,
> > the question is whether we're disciplined enough to run with checking
> > enabled and react to bug reports about the fatal error being thrown.
> 
> I've respun the patch for the current trunk, and I wonder whether
> anybody has any comments here (so I've added Stefan and Mattias to the
> CCs).
> 
> I think if we add this, it should be enabled only if the build is
> configured with --enable-checking.

Yes, I agree.

In addition, AFAIU this uses some GCC-specific stuff without providing
for other compilers that don't support the same literal features.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#46988: 28.0.50; Documenting and verifying assumptions about C code not calling quit or GCing
  2022-06-20 11:47     ` Eli Zaretskii
@ 2022-06-23 15:56       ` Pip Cet
  2022-06-23 16:08         ` Eli Zaretskii
  0 siblings, 1 reply; 14+ messages in thread
From: Pip Cet @ 2022-06-23 15:56 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 46988, Mattias, Lars Ingebrigtsen, Stefan Monnier

On Mon, Jun 20, 2022 at 11:48 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > I think if we add this, it should be enabled only if the build is
> > configured with --enable-checking.
>
> Yes, I agree.

So do I.

> In addition, AFAIU this uses some GCC-specific stuff without providing
> for other compilers that don't support the same literal features.

I'm not sure to what extent clang emulates those same features, but
it's certainly not standard C.

Pip





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#46988: 28.0.50; Documenting and verifying assumptions about C code not calling quit or GCing
  2022-06-23 15:56       ` Pip Cet
@ 2022-06-23 16:08         ` Eli Zaretskii
  2022-06-23 16:20           ` Mattias Engdegård
  0 siblings, 1 reply; 14+ messages in thread
From: Eli Zaretskii @ 2022-06-23 16:08 UTC (permalink / raw)
  To: Pip Cet; +Cc: 46988, mattiase, larsi, monnier

> From: Pip Cet <pipcet@gmail.com>
> Date: Thu, 23 Jun 2022 15:56:49 +0000
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, 46988@debbugs.gnu.org, Mattias <mattiase@acm.org>, 
> 	Stefan Monnier <monnier@iro.umontreal.ca>
> 
> > In addition, AFAIU this uses some GCC-specific stuff without providing
> > for other compilers that don't support the same literal features.
> 
> I'm not sure to what extent clang emulates those same features, but
> it's certainly not standard C.

Clang at least attempts to be GCC-compliant.  But there are other C
compilers we want to support which don't do even that.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#46988: 28.0.50; Documenting and verifying assumptions about C code not calling quit or GCing
  2022-06-23 16:08         ` Eli Zaretskii
@ 2022-06-23 16:20           ` Mattias Engdegård
  2022-06-23 16:35             ` Eli Zaretskii
  0 siblings, 1 reply; 14+ messages in thread
From: Mattias Engdegård @ 2022-06-23 16:20 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 46988, larsi, monnier, Pip Cet

23 juni 2022 kl. 18.08 skrev Eli Zaretskii <eliz@gnu.org>:

> Clang at least attempts to be GCC-compliant.  But there are other C
> compilers we want to support which don't do even that.

Quite true. I don't think we should bother supporting them (at least not right away); just disable the feature for them.

The feature looks sound in principle and probably useful. Matt Armstrong had some good comments on the surface (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=46988#20) to make it more readable.

Maybe it would be useful to generalise it to other dynamic checks, such as 'no consing', 'no lisp', 'no bytecode', 'no regexp', 'no signalling'?






^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#46988: 28.0.50; Documenting and verifying assumptions about C code not calling quit or GCing
  2022-06-23 16:20           ` Mattias Engdegård
@ 2022-06-23 16:35             ` Eli Zaretskii
  0 siblings, 0 replies; 14+ messages in thread
From: Eli Zaretskii @ 2022-06-23 16:35 UTC (permalink / raw)
  To: Mattias Engdegård; +Cc: 46988, larsi, monnier, pipcet

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Thu, 23 Jun 2022 18:20:09 +0200
> Cc: Pip Cet <pipcet@gmail.com>, larsi@gnus.org, 46988@debbugs.gnu.org,
>         monnier@iro.umontreal.ca
> 
> 23 juni 2022 kl. 18.08 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > Clang at least attempts to be GCC-compliant.  But there are other C
> > compilers we want to support which don't do even that.
> 
> Quite true. I don't think we should bother supporting them (at least not right away); just disable the feature for them.

Yes, that's what I meant: we shouldn't break compilation with those,
but we don't have to do more.





^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2022-06-23 16:35 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-07 13:47 bug#46988: 28.0.50; Documenting and verifying assumptions about C code not calling quit or GCing Pip Cet
2021-03-07 14:06 ` Pip Cet
2021-03-08 19:42   ` Lars Ingebrigtsen
2021-03-08 19:57     ` Pip Cet
2021-03-09 14:05       ` Lars Ingebrigtsen
2021-03-10 18:28   ` Matt Armstrong
2021-03-10 19:09     ` Pip Cet
2021-03-11 23:17       ` Matt Armstrong
2022-06-20  1:41   ` Lars Ingebrigtsen
2022-06-20 11:47     ` Eli Zaretskii
2022-06-23 15:56       ` Pip Cet
2022-06-23 16:08         ` Eli Zaretskii
2022-06-23 16:20           ` Mattias Engdegård
2022-06-23 16:35             ` Eli Zaretskii

Code repositories for project(s) associated with this inbox:

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).