Re: [continuation args 2/2] Make continuation args not leak

[Pedro Alves <pedro@codesourcery.com>]

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

A Saturday 12 July 2008 21:23:41, Ulrich Weigand wrote:
> Pedro Alves wrote:
> > 	Rewrite continuations internals on top of cleanups and plug
> > 	continuation arguments leaks.
>
> This breaks the build due to violation of the C aliasing rules:
>
> /home/uweigand/fsf/gdb-head/gdb/utils.c: In function 'add_continuation':
> /home/uweigand/fsf/gdb-head/gdb/utils.c:479: warning: dereferencing
> type-punned pointer will break strict-aliasing rules
...

>
> > +  struct cleanup **as_cleanup_p = (struct cleanup **) &cmd_continuation;
>
> This may cause a "struct cleanup *" to alias with a "struct continuation
> *", which is not allowed according to the C standard.
>

Sorry for that.  Missed building at > -O0 to catch these things.

I thought that since the type is not defined, this would not
be a problem, as you can't dereference through cmd_continuation.

> Why do we still have a "struct continuation" (as nowhere-defined type)?
> Shouldn't this just use "struct cleanup" throughout?

Being a cleanup is an implementation detail, I didn't want to make
that fact public.

This fixes the issue for me by making the inheritance explicit.

Tested on x86_64-unknown-linux-gnu async mode.

OK?

-- 
Pedro Alves

[-- Attachment #2: continuations_alias.diff --]
[-- Type: text/x-diff, Size: 3897 bytes --]

2008-07-12  Pedro Alves  <pedro@codesourcery.com>

	* utils.c (struct continuation): Define as inheriting struct
	cleanup.
	(add_continuation, do_all_continuations)
	(discard_all_continuations, add_intermediate_continuation)
	(do_all_intermediate_continuations)
	(discard_all_intermediate_continuations): Adjust.

---
 gdb/utils.c |   37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

Index: src/gdb/utils.c
===================================================================
--- src.orig/gdb/utils.c	2008-07-12 22:50:56.000000000 +0100
+++ src/gdb/utils.c	2008-07-12 23:25:40.000000000 +0100
@@ -470,19 +470,29 @@ null_cleanup (void *arg)
 {
 }
 
+/* Continuations are implemented as cleanups internally.  Inherit from
+   cleanups.  */
+struct continuation
+{
+  struct cleanup base;
+};
+
 /* Add a continuation to the continuation list, the global list
-   cmd_continuation. The new continuation will be added at the front.*/
+   cmd_continuation. The new continuation will be added at the
+   front.  */
 void
 add_continuation (void (*continuation_hook) (void *), void *args,
 		  void (*continuation_free_args) (void *))
 {
-  struct cleanup **as_cleanup_p = (struct cleanup **) &cmd_continuation;
+  struct cleanup *as_cleanup = &cmd_continuation->base;
   make_cleanup_ftype *continuation_hook_fn = continuation_hook;
 
-  make_my_cleanup2 (as_cleanup_p,
+  make_my_cleanup2 (&as_cleanup,
 		    continuation_hook_fn,
 		    args,
 		    continuation_free_args);
+
+  cmd_continuation = (struct continuation *) as_cleanup;
 }
 
 /* Walk down the cmd_continuation list, and execute all the
@@ -503,7 +513,7 @@ do_all_continuations (void)
      effect of invoking the continuations and the processing of the
      preexisting continuations will not be affected.  */
 
-  continuation_ptr = (struct cleanup *) cmd_continuation;
+  continuation_ptr = &cmd_continuation->base;
   cmd_continuation = NULL;
 
   /* Work now on the list we have set aside.  */
@@ -515,8 +525,9 @@ do_all_continuations (void)
 void
 discard_all_continuations (void)
 {
-  struct cleanup **continuation_ptr = (struct cleanup **) &cmd_continuation;
-  discard_my_cleanups (continuation_ptr, NULL);
+  struct cleanup *continuation_ptr = &cmd_continuation->base;
+  discard_my_cleanups (&continuation_ptr, NULL);
+  cmd_continuation = NULL;
 }
 
 /* Add a continuation to the continuation list, the global list
@@ -527,13 +538,15 @@ add_intermediate_continuation (void (*co
 			       (void *), void *args,
 			       void (*continuation_free_args) (void *))
 {
-  struct cleanup **as_cleanup_p = (struct cleanup **) &intermediate_continuation;
+  struct cleanup *as_cleanup = &intermediate_continuation->base;
   make_cleanup_ftype *continuation_hook_fn = continuation_hook;
 
-  make_my_cleanup2 (as_cleanup_p,
+  make_my_cleanup2 (&as_cleanup,
 		    continuation_hook_fn,
 		    args,
 		    continuation_free_args);
+
+  intermediate_continuation = (struct continuation *) as_cleanup;
 }
 
 /* Walk down the cmd_continuation list, and execute all the
@@ -554,7 +567,7 @@ do_all_intermediate_continuations (void)
      effect of invoking the continuations and the processing of the
      preexisting continuations will not be affected.  */
 
-  continuation_ptr = (struct cleanup *) intermediate_continuation;
+  continuation_ptr = &intermediate_continuation->base;
   intermediate_continuation = NULL;
 
   /* Work now on the list we have set aside.  */
@@ -566,9 +579,9 @@ do_all_intermediate_continuations (void)
 void
 discard_all_intermediate_continuations (void)
 {
-  struct cleanup **continuation_ptr
-    = (struct cleanup **) &intermediate_continuation;
-  discard_my_cleanups (continuation_ptr, NULL);
+  struct cleanup *continuation_ptr = &intermediate_continuation->base;
+  discard_my_cleanups (&continuation_ptr, NULL);
+  continuation_ptr = NULL;
 }
 \f