[RFA] Ensure result of make_cleanup is never NULL.

[RFA] Ensure result of make_cleanup is never NULL.

[Doug Evans]

Hi.

This is a followup patch to:
http://sourceware.org/ml/gdb-patches/2012-04/msg00395.html
[tweaked as indicated here:
http://sourceware.org/ml/gdb-patches/2012-04/msg00396.html]

It does two main things:
- moves definition of struct cleanup into cleanups.c
- ensures result of make_cleanup is never NULL

There's more than one way to ensure the result of make_cleanup is never NULL.
This way seems simple enough.

Regression tested on amd64-linux.

Ok to check in?

2012-04-15  Doug Evans  <dje@sebabeach.org>

	* cleanups.h (struct cleanup): Move to cleanups.c.
	(make_cleanup_dtor_ftype): New typedef.
	(make_cleanup_dtor): Use it.
	(ALL_CLEANUPS): Replace with ...
	(all_cleanups): ... this.  Declare.  All uses updated.
	* utils.c (CLEANUP_FENCEPOST): Define.
	(cleanup_chain, final_cleanup_chain): Initialize to CLEANUP_FENCEPOST.
	(make_my_cleanup2): Assert result is non-NULL.
	(all_cleanups): New function.
	(save_my_cleanups): Initialize new chain to CLEANUP_FENCEPOST instead
	of NULL.

--- cleanups.h=	2012-04-15 18:45:20.138903764 -0700
+++ cleanups.h	2012-04-15 18:37:45.916900087 -0700
@@ -19,29 +19,6 @@
 #ifndef CLEANUPS_H
 #define CLEANUPS_H
 
-/* The cleanup list records things that have to be undone
-   if an error happens (descriptors to be closed, memory to be freed, etc.)
-   Each link in the chain records a function to call and an
-   argument to give it.
-
-   Use make_cleanup to add an element to the cleanup chain.
-   Use do_cleanups to do all cleanup actions back to a given
-   point in the chain.  Use discard_cleanups to remove cleanups
-   from the chain back to a given point, not doing them.
-
-   If the argument is pointer to allocated memory, then you need
-   to additionally set the 'free_arg' member to a function that will
-   free that memory.  This function will be called both when the cleanup
-   is executed and when it's discarded.  */
-
-struct cleanup
-  {
-    struct cleanup *next;
-    void (*function) (void *);
-    void (*free_arg) (void *);
-    void *arg;
-  };
-
 /* NOTE: cagney/2000-03-04: This typedef is strictly for the
    make_cleanup function declarations below.  Do not use this typedef
    as a cast when passing functions into the make_cleanup() code.
@@ -49,21 +26,25 @@ struct cleanup
    Calling a f(char*) function with f(void*) is non-portable.  */
 typedef void (make_cleanup_ftype) (void *);
 
+/* Function type for the dtor in make_cleanup_dtor.  */
+typedef void (make_cleanup_dtor_ftype) (void *);
+
 /* WARNING: The result of the "make cleanup" routines is not the intuitive
    choice of being a handle on the just-created cleanup.  Instead it is an
    opaque handle of the cleanup mechanism and represents all cleanups created
-   from that point onwards.  */
+   from that point onwards.
+   The result is guaranteed to be non-NULL though.  */
 
 extern struct cleanup *make_cleanup (make_cleanup_ftype *, void *);
 
 extern struct cleanup *make_cleanup_dtor (make_cleanup_ftype *, void *,
-					  void (*dtor) (void *));
+					  make_cleanup_dtor_ftype *);
 
 extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void *);
 
 /* A special value to pass to do_cleanups and do_final_cleanups
    to tell them to do all cleanups.  */
-#define	ALL_CLEANUPS	((struct cleanup *)0)
+extern struct cleanup *all_cleanups (void);
 
 extern void do_cleanups (struct cleanup *);
 extern void do_final_cleanups (struct cleanup *);
--- cleanups.c=	2012-04-15 11:20:33.999687721 -0700
+++ cleanups.c	2012-04-15 18:46:51.397904493 -0700
@@ -18,15 +18,43 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "gdb_assert.h"
+
+/* The cleanup list records things that have to be undone
+   if an error happens (descriptors to be closed, memory to be freed, etc.)
+   Each link in the chain records a function to call and an
+   argument to give it.
+
+   Use make_cleanup to add an element to the cleanup chain.
+   Use do_cleanups to do all cleanup actions back to a given
+   point in the chain.  Use discard_cleanups to remove cleanups
+   from the chain back to a given point, not doing them.
+
+   If the argument is pointer to allocated memory, then you need
+   to additionally set the 'free_arg' member to a function that will
+   free that memory.  This function will be called both when the cleanup
+   is executed and when it's discarded.  */
+
+struct cleanup
+  {
+    struct cleanup *next;
+    void (*function) (void *);
+    void (*free_arg) (void *);
+    void *arg;
+  };
+
+/* A fencepost used to mark the end of a cleanup chain.
+   The value is chosen to be non-NULL so that make_cleanup never returns NULL,
+   and cause a segv if dereferenced.  */
+#define CLEANUP_FENCEPOST ((struct cleanup *) 1)
 
 /* Chain of cleanup actions established with make_cleanup,
    to be executed if an error happens.  */
+static struct cleanup *cleanup_chain = CLEANUP_FENCEPOST;
 
-/* Cleaned up after a failed command.  */
-static struct cleanup *cleanup_chain;
-
-/* Cleaned up when gdb exits.  */
-static struct cleanup *final_cleanup_chain;
+/* Chain of cleanup actions established with make_final_cleanup,
+   to be executed when gdb exits.  */
+static struct cleanup *final_cleanup_chain = CLEANUP_FENCEPOST;
 
 static struct cleanup *
 make_my_cleanup2 (struct cleanup **pmy_chain, make_cleanup_ftype *function,
@@ -42,6 +70,7 @@ make_my_cleanup2 (struct cleanup **pmy_c
   new->arg = arg;
   *pmy_chain = new;
 
+  gdb_assert (old_chain != NULL);
   return old_chain;
 }
 
@@ -98,6 +127,15 @@ do_my_cleanups (struct cleanup **pmy_cha
     }
 }
 
+/* Return a value that can be passed to do_cleanups, do_final_cleanups to
+   indicate perform all cleanups.  */
+
+struct cleanup *
+all_cleanups (void)
+{
+  return CLEANUP_FENCEPOST;
+}
+
 /* Discard cleanups and do the actions they describe
    until we get back to the point OLD_CHAIN in the cleanup_chain.  */
 
@@ -154,7 +192,7 @@ save_my_cleanups (struct cleanup **pmy_c
 {
   struct cleanup *old_chain = *pmy_chain;
 
-  *pmy_chain = 0;
+  *pmy_chain = CLEANUP_FENCEPOST;
   return old_chain;
 }
 
Index: exceptions.c
===================================================================
RCS file: /cvs/src/src/gdb/exceptions.c,v
retrieving revision 1.50
diff -u -p -r1.50 exceptions.c
--- exceptions.c	4 Jan 2012 08:17:01 -0000	1.50
+++ exceptions.c	16 Apr 2012 01:54:30 -0000
@@ -224,7 +224,7 @@ throw_exception (struct gdb_exception ex
   quit_flag = 0;
   immediate_quit = 0;
 
-  do_cleanups (ALL_CLEANUPS);
+  do_cleanups (all_cleanups ());
 
   /* Jump to the containing catch_errors() call, communicating REASON
      to that call via setjmp's return value.  Note that REASON can't
Index: main.c
===================================================================
RCS file: /cvs/src/src/gdb/main.c,v
retrieving revision 1.104
diff -u -p -r1.104 main.c
--- main.c	19 Mar 2012 18:19:24 -0000	1.104
+++ main.c	16 Apr 2012 01:54:30 -0000
@@ -230,7 +230,7 @@ captured_command_loop (void *data)
      are not that well behaved.  do_cleanups should either be replaced
      with a do_cleanups call (to cover the problem) or an assertion
      check to detect bad FUNCs code.  */
-  do_cleanups (ALL_CLEANUPS);
+  do_cleanups (all_cleanups ());
   /* If the command_loop returned, normally (rather than threw an
      error) we try to quit.  If the quit is aborted, catch_errors()
      which called this catch the signal and restart the command
Index: top.c
===================================================================
RCS file: /cvs/src/src/gdb/top.c,v
retrieving revision 1.214
diff -u -p -r1.214 top.c
--- top.c	1 Mar 2012 19:30:20 -0000	1.214
+++ top.c	16 Apr 2012 01:54:30 -0000
@@ -1297,8 +1297,9 @@ quit_target (void *arg)
   if (write_history_p && history_filename)
     write_history (history_filename);
 
-  do_final_cleanups (ALL_CLEANUPS);    /* Do any final cleanups before
-					  exiting.  */
+  /* Do any final cleanups before exiting.  */
+  do_final_cleanups (all_cleanups ());
+
   return 0;
 }
 

Re: [RFA] Ensure result of make_cleanup is never NULL.

[Pedro Alves]

This looks good to me.

On 04/16/2012 03:06 AM, Doug Evans wrote:
> +/* A fencepost used to mark the end of a cleanup chain.
> +   The value is chosen to be non-NULL so that make_cleanup never returns NULL,
> +   and cause a segv if dereferenced.  */
> +#define CLEANUP_FENCEPOST ((struct cleanup *) 1)

I'd mildly prefer -1.  Easier to spot as being special with the
debugger; as a general principle for these things, it's an address less likely
to end be created by mistake (thinking of &foo->a yieling a pointer to a
low address when foo is NULL); and it is just more customary.

-- 
Pedro Alves

Re: [RFA] Ensure result of make_cleanup is never NULL.

[Joel Brobecker]

> +/* A fencepost used to mark the end of a cleanup chain.
> +   The value is chosen to be non-NULL so that make_cleanup never returns NULL,
> +   and cause a segv if dereferenced.  */
> +#define CLEANUP_FENCEPOST ((struct cleanup *) 1)

Just a tiny idea, not really important, but JIC: Could we use an enum
intead of a define so that GDB prints "CLEANUP_FENCEPOST" rather than
a numeric value when we print a cleanup pointer that's the fencepost?

-- 
Joel

Re: [RFA] Ensure result of make_cleanup is never NULL.

[Jan Kratochvil]

On Mon, 16 Apr 2012 16:40:11 +0200, Joel Brobecker wrote:
> Just a tiny idea, not really important, but JIC: Could we use an enum
> intead of a define so that GDB prints "CLEANUP_FENCEPOST" rather than
> a numeric value when we print a cleanup pointer that's the fencepost?

It will not work, it is a pointer, not enum.

Maybe one could add something to gdb-gdb.py but it also did not work for me.


Jan

Re: [RFA] Ensure result of make_cleanup is never NULL.

[Pedro Alves]

On 04/16/2012 03:40 PM, Joel Brobecker wrote:

>> +/* A fencepost used to mark the end of a cleanup chain.
>> +   The value is chosen to be non-NULL so that make_cleanup never returns NULL,
>> +   and cause a segv if dereferenced.  */
>> +#define CLEANUP_FENCEPOST ((struct cleanup *) 1)
> 
> Just a tiny idea, not really important, but JIC: Could we use an enum
> intead of a define so that GDB prints "CLEANUP_FENCEPOST" rather than
> a numeric value when we print a cleanup pointer that's the fencepost?


No, because what you'll be printing will have type struct cleanup pointer, not
whatever enum it was cast from.

I see at least two ways to get something like that:

- a gdb specific pretty printer for cleanups.

- Make the sentinel a real object:

   static struct cleanup sentinel_cleanup;
   #define CLEANUP_FENCEPOST &sentinel_cleanup

  And get Tromey's "set print symbol" patch in, which IIRC/IIUC, the
  latest version makes GDB print the symbol name corresponding
  to addresses by default.  Then gdb would print something like:

   (gdb) p old_chain
   $1 = 0xfoobar <sentinel_cleanup>

-- 
Pedro Alves

Re: [RFA] Ensure result of make_cleanup is never NULL.

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro>   And get Tromey's "set print symbol" patch in, which IIRC/IIUC, the
Pedro>   latest version makes GDB print the symbol name corresponding
Pedro>   to addresses by default.  Then gdb would print something like:

Pedro>    (gdb) p old_chain
Pedro>    $1 = 0xfoobar <sentinel_cleanup>

Yeah, that is what it would do.

Tom

Re: [RFA] Ensure result of make_cleanup is never NULL.

[Doug Evans]

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

On Mon, Apr 16, 2012 at 8:21 AM, Doug Evans <dje@google.com> wrote:
>
> On Apr 16, 2012 7:58 AM, "Pedro Alves" <palves@redhat.com> wrote:
>>
>> On 04/16/2012 03:40 PM, Joel Brobecker wrote:
>>
>> >> +/* A fencepost used to mark the end of a cleanup chain.
>> >> +   The value is chosen to be non-NULL so that make_cleanup never
>> >> returns NULL,
>> >> +   and cause a segv if dereferenced.  */
>> >> +#define CLEANUP_FENCEPOST ((struct cleanup *) 1)
>> >
>> > Just a tiny idea, not really important, but JIC: Could we use an enum
>> > intead of a define so that GDB prints "CLEANUP_FENCEPOST" rather than
>> > a numeric value when we print a cleanup pointer that's the fencepost?
>>
>>
>> No, because what you'll be printing will have type struct cleanup pointer,
>> not
>> whatever enum it was cast from.
>>
>> I see at least two ways to get something like that:
>>
>> - a gdb specific pretty printer for cleanups.
>>
>> - Make the sentinel a real object:
>>
>>   static struct cleanup sentinel_cleanup;
>>   #define CLEANUP_FENCEPOST &sentinel_cleanup
>>
>>  And get Tromey's "set print symbol" patch in, which IIRC/IIUC, the
>>  latest version makes GDB print the symbol name corresponding
>>  to addresses by default.  Then gdb would print something like:
>>
>>   (gdb) p old_chain
>>   $1 = 0xfoobar <sentinel_cleanup>
>>
>> --
>> Pedro Alves
>
> That was my other plan.  I can do that instead.

How about this?


2012-04-17  Doug Evans  <dje@google.com>

        * cleanups.h (struct cleanup): Move to cleanups.c.
        (make_cleanup_dtor_ftype): New typedef.
        (make_cleanup_dtor): Use it.
        (ALL_CLEANUPS): Replace with ...
        (all_cleanups): ... this.  Declare.  All uses updated.
        * cleanups.c (cleanup_sentinel): New static global.
        (CLEANUP_SENTINEL): Define.
        (cleanup_chain, final_cleanup_chain): Initialize to CLEANUP_SENTINEL.
        (make_my_cleanup2): Assert result is non-NULL.
        (all_cleanups): New function.
        (save_my_cleanups): Initialize new chain to CLEANUP_SENTINEL instead
        of NULL.

[-- Attachment #2: gdb-120417-cleanups-sentinel-1.patch.txt --]
[-- Type: text/plain, Size: 8221 bytes --]

2012-04-17  Doug Evans  <dje@google.com>

	* cleanups.h (struct cleanup): Move to cleanups.c.
	(make_cleanup_dtor_ftype): New typedef.
	(make_cleanup_dtor): Use it.
	(ALL_CLEANUPS): Replace with ...
	(all_cleanups): ... this.  Declare.  All uses updated.
	* cleanups.c: #include "gdb_assert.h".
	(cleanup_sentinel): New static global.
	(CLEANUP_SENTINEL): Define.
	(cleanup_chain, final_cleanup_chain): Initialize to CLEANUP_SENTINEL.
	(make_my_cleanup2): Assert result is non-NULL.
	(all_cleanups): New function.
	(save_my_cleanups): Initialize new chain to CLEANUP_SENTINEL instead
	of NULL.

Index: cleanups.c
===================================================================
RCS file: /cvs/src/src/gdb/cleanups.c,v
retrieving revision 1.2
diff -u -p -r1.2 cleanups.c
--- cleanups.c	17 Apr 2012 21:24:47 -0000	1.2
+++ cleanups.c	17 Apr 2012 22:46:10 -0000
@@ -18,15 +18,50 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "gdb_assert.h"
+
+/* The cleanup list records things that have to be undone
+   if an error happens (descriptors to be closed, memory to be freed, etc.)
+   Each link in the chain records a function to call and an
+   argument to give it.
+
+   Use make_cleanup to add an element to the cleanup chain.
+   Use do_cleanups to do all cleanup actions back to a given
+   point in the chain.  Use discard_cleanups to remove cleanups
+   from the chain back to a given point, not doing them.
+
+   If the argument is pointer to allocated memory, then you need
+   to additionally set the 'free_arg' member to a function that will
+   free that memory.  This function will be called both when the cleanup
+   is executed and when it's discarded.  */
+
+struct cleanup
+{
+  struct cleanup *next;
+  void (*function) (void *);
+  void (*free_arg) (void *);
+  void *arg;
+};
+
+/* Used to mark the end of a cleanup chain.
+   The value is chosen so that it:
+   - is non-NULL so that make_cleanup never returns NULL,
+   - causes a segv if dereferenced
+     [though this won't catch errors that a value of, say,
+     ((struct cleanup *) -1) will]
+   - displays as something useful when printed in gdb.  */
+static struct cleanup cleanup_sentinel;
+
+/* Handy macro to use when referring to cleanup_sentinel.  */
+#define CLEANUP_SENTINEL (&cleanup_sentinel)
 
 /* Chain of cleanup actions established with make_cleanup,
    to be executed if an error happens.  */
+static struct cleanup *cleanup_chain = CLEANUP_SENTINEL;
 
-/* Cleaned up after a failed command.  */
-static struct cleanup *cleanup_chain;
-
-/* Cleaned up when gdb exits.  */
-static struct cleanup *final_cleanup_chain;
+/* Chain of cleanup actions established with make_final_cleanup,
+   to be executed when gdb exits.  */
+static struct cleanup *final_cleanup_chain = CLEANUP_SENTINEL;
 
 /* Main worker routine to create a cleanup.
    PMY_CHAIN is a pointer to either cleanup_chain or final_cleanup_chain.
@@ -51,6 +86,7 @@ make_my_cleanup2 (struct cleanup **pmy_c
   new->arg = arg;
   *pmy_chain = new;
 
+  gdb_assert (old_chain != NULL);
   return old_chain;
 }
 
@@ -120,6 +156,15 @@ do_my_cleanups (struct cleanup **pmy_cha
     }
 }
 
+/* Return a value that can be passed to do_cleanups, do_final_cleanups to
+   indicate perform all cleanups.  */
+
+struct cleanup *
+all_cleanups (void)
+{
+  return CLEANUP_SENTINEL;
+}
+
 /* Discard cleanups and do the actions they describe
    until we get back to the point OLD_CHAIN in the cleanup_chain.  */
 
@@ -185,7 +230,7 @@ save_my_cleanups (struct cleanup **pmy_c
 {
   struct cleanup *old_chain = *pmy_chain;
 
-  *pmy_chain = 0;
+  *pmy_chain = CLEANUP_SENTINEL;
   return old_chain;
 }
 
Index: cleanups.h
===================================================================
RCS file: /cvs/src/src/gdb/cleanups.h,v
retrieving revision 1.2
diff -u -p -r1.2 cleanups.h
--- cleanups.h	17 Apr 2012 21:24:47 -0000	1.2
+++ cleanups.h	17 Apr 2012 22:46:10 -0000
@@ -19,28 +19,8 @@
 #ifndef CLEANUPS_H
 #define CLEANUPS_H
 
-/* The cleanup list records things that have to be undone
-   if an error happens (descriptors to be closed, memory to be freed, etc.)
-   Each link in the chain records a function to call and an
-   argument to give it.
-
-   Use make_cleanup to add an element to the cleanup chain.
-   Use do_cleanups to do all cleanup actions back to a given
-   point in the chain.  Use discard_cleanups to remove cleanups
-   from the chain back to a given point, not doing them.
-
-   If the argument is pointer to allocated memory, then you need
-   to additionally set the 'free_arg' member to a function that will
-   free that memory.  This function will be called both when the cleanup
-   is executed and when it's discarded.  */
-
-struct cleanup
-  {
-    struct cleanup *next;
-    void (*function) (void *);
-    void (*free_arg) (void *);
-    void *arg;
-  };
+/* Outside of cleanups.c, this is an opaque type.  */
+struct cleanup;
 
 /* NOTE: cagney/2000-03-04: This typedef is strictly for the
    make_cleanup function declarations below.  Do not use this typedef
@@ -49,21 +29,25 @@ struct cleanup
    Calling a f(char*) function with f(void*) is non-portable.  */
 typedef void (make_cleanup_ftype) (void *);
 
+/* Function type for the dtor in make_cleanup_dtor.  */
+typedef void (make_cleanup_dtor_ftype) (void *);
+
 /* WARNING: The result of the "make cleanup" routines is not the intuitive
    choice of being a handle on the just-created cleanup.  Instead it is an
    opaque handle of the cleanup mechanism and represents all cleanups created
-   from that point onwards.  */
+   from that point onwards.
+   The result is guaranteed to be non-NULL though.  */
 
 extern struct cleanup *make_cleanup (make_cleanup_ftype *, void *);
 
 extern struct cleanup *make_cleanup_dtor (make_cleanup_ftype *, void *,
-					  void (*dtor) (void *));
+					  make_cleanup_dtor_ftype *);
 
 extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void *);
 
 /* A special value to pass to do_cleanups and do_final_cleanups
    to tell them to do all cleanups.  */
-#define	ALL_CLEANUPS	((struct cleanup *)0)
+extern struct cleanup *all_cleanups (void);
 
 extern void do_cleanups (struct cleanup *);
 extern void do_final_cleanups (struct cleanup *);
Index: exceptions.c
===================================================================
RCS file: /cvs/src/src/gdb/exceptions.c,v
retrieving revision 1.50
diff -u -p -r1.50 exceptions.c
--- exceptions.c	4 Jan 2012 08:17:01 -0000	1.50
+++ exceptions.c	17 Apr 2012 22:46:10 -0000
@@ -224,7 +224,7 @@ throw_exception (struct gdb_exception ex
   quit_flag = 0;
   immediate_quit = 0;
 
-  do_cleanups (ALL_CLEANUPS);
+  do_cleanups (all_cleanups ());
 
   /* Jump to the containing catch_errors() call, communicating REASON
      to that call via setjmp's return value.  Note that REASON can't
Index: main.c
===================================================================
RCS file: /cvs/src/src/gdb/main.c,v
retrieving revision 1.108
diff -u -p -r1.108 main.c
--- main.c	17 Apr 2012 15:56:21 -0000	1.108
+++ main.c	17 Apr 2012 22:46:10 -0000
@@ -231,7 +231,7 @@ captured_command_loop (void *data)
      are not that well behaved.  do_cleanups should either be replaced
      with a do_cleanups call (to cover the problem) or an assertion
      check to detect bad FUNCs code.  */
-  do_cleanups (ALL_CLEANUPS);
+  do_cleanups (all_cleanups ());
   /* If the command_loop returned, normally (rather than threw an
      error) we try to quit.  If the quit is aborted, catch_errors()
      which called this catch the signal and restart the command
Index: top.c
===================================================================
RCS file: /cvs/src/src/gdb/top.c,v
retrieving revision 1.214
diff -u -p -r1.214 top.c
--- top.c	1 Mar 2012 19:30:20 -0000	1.214
+++ top.c	17 Apr 2012 22:46:10 -0000
@@ -1297,8 +1297,9 @@ quit_target (void *arg)
   if (write_history_p && history_filename)
     write_history (history_filename);
 
-  do_final_cleanups (ALL_CLEANUPS);    /* Do any final cleanups before
-					  exiting.  */
+  /* Do any final cleanups before exiting.  */
+  do_final_cleanups (all_cleanups ());
+
   return 0;
 }
 

Re: [RFA] Ensure result of make_cleanup is never NULL.

[Doug Evans]

On Tue, Apr 17, 2012 at 4:00 PM, Doug Evans <dje@google.com> wrote:
> On Mon, Apr 16, 2012 at 8:21 AM, Doug Evans <dje@google.com> wrote:
>>
>> On Apr 16, 2012 7:58 AM, "Pedro Alves" <palves@redhat.com> wrote:
>>>
>>> On 04/16/2012 03:40 PM, Joel Brobecker wrote:
>>>
>>> >> +/* A fencepost used to mark the end of a cleanup chain.
>>> >> +   The value is chosen to be non-NULL so that make_cleanup never
>>> >> returns NULL,
>>> >> +   and cause a segv if dereferenced.  */
>>> >> +#define CLEANUP_FENCEPOST ((struct cleanup *) 1)
>>> >
>>> > Just a tiny idea, not really important, but JIC: Could we use an enum
>>> > intead of a define so that GDB prints "CLEANUP_FENCEPOST" rather than
>>> > a numeric value when we print a cleanup pointer that's the fencepost?
>>>
>>>
>>> No, because what you'll be printing will have type struct cleanup pointer,
>>> not
>>> whatever enum it was cast from.
>>>
>>> I see at least two ways to get something like that:
>>>
>>> - a gdb specific pretty printer for cleanups.
>>>
>>> - Make the sentinel a real object:
>>>
>>>   static struct cleanup sentinel_cleanup;
>>>   #define CLEANUP_FENCEPOST &sentinel_cleanup
>>>
>>>  And get Tromey's "set print symbol" patch in, which IIRC/IIUC, the
>>>  latest version makes GDB print the symbol name corresponding
>>>  to addresses by default.  Then gdb would print something like:
>>>
>>>   (gdb) p old_chain
>>>   $1 = 0xfoobar <sentinel_cleanup>
>>>
>>> --
>>> Pedro Alves
>>
>> That was my other plan.  I can do that instead.
>
> How about this?
>
>
> 2012-04-17  Doug Evans  <dje@google.com>
>
>        * cleanups.h (struct cleanup): Move to cleanups.c.
>        (make_cleanup_dtor_ftype): New typedef.
>        (make_cleanup_dtor): Use it.
>        (ALL_CLEANUPS): Replace with ...
>        (all_cleanups): ... this.  Declare.  All uses updated.
>        * cleanups.c (cleanup_sentinel): New static global.
>        (CLEANUP_SENTINEL): Define.
>        (cleanup_chain, final_cleanup_chain): Initialize to CLEANUP_SENTINEL.
>        (make_my_cleanup2): Assert result is non-NULL.
>        (all_cleanups): New function.
>        (save_my_cleanups): Initialize new chain to CLEANUP_SENTINEL instead
>        of NULL.

btw, I thought about making cleanup_sentinel const here, for a bit of
extra robustness.

Re: [RFA] Ensure result of make_cleanup is never NULL.

[Joel Brobecker]

> btw, I thought about making cleanup_sentinel const here, for a bit of
> extra robustness.

Sounds like a good idea. And also, I was wondering if it made sense
to initialize the sentinel struct to `{ 0 }', even if we do not
actually access the contents of that specific struct instance?

-- 
Joel

Re: [RFA] Ensure result of make_cleanup is never NULL.

[Pedro Alves]

On 04/18/2012 02:05 AM, Joel Brobecker wrote:

>> btw, I thought about making cleanup_sentinel const here, for a bit of
>> extra robustness.
> 
> Sounds like a good idea. And also, I was wondering if it made sense
> to initialize the sentinel struct to `{ 0 }', even if we do not
> actually access the contents of that specific struct instance?


Not really necessary.  C does that implicitly to all uninitialized global variables.

-- 
Pedro Alves

Re: [RFA] Ensure result of make_cleanup is never NULL.

[Pedro Alves]

On 04/18/2012 12:00 AM, Doug Evans wrote:

> On Mon, Apr 16, 2012 at 8:21 AM, Doug Evans <dje@google.com> wrote:
>> On Apr 16, 2012 7:58 AM, "Pedro Alves" <palves@redhat.com> wrote:
>>>
>>> - Make the sentinel a real object:
>>>
>> That was my other plan.  I can do that instead.
> 
> How about this?


Looks good to me.

-- 
Pedro Alves

Re: [RFA] Ensure result of make_cleanup is never NULL.

[Pedro Alves]

On 04/18/2012 01:43 AM, Doug Evans wrote:

> btw, I thought about making cleanup_sentinel const here, for a bit of
> extra robustness.


Won't that mean you'd need to cast the constness out in most, if
not all places that use it?

-- 
Pedro Alves

Re: [RFA] Ensure result of make_cleanup is never NULL.

[Doug Evans]

On Wed, Apr 18, 2012 at 2:26 AM, Pedro Alves <palves@redhat.com> wrote:
> On 04/18/2012 01:43 AM, Doug Evans wrote:
>
>> btw, I thought about making cleanup_sentinel const here, for a bit of
>> extra robustness.
>
>
> Won't that mean you'd need to cast the constness out in most, if
> not all places that use it?

--- cleanups.c= 2012-04-17 12:02:38.000000000 -0700
+++ cleanups.c  2012-04-18 06:59:46.000000000 -0700
@@ -50,10 +50,10 @@ struct cleanup
      [though this won't catch errors that a value of, say,
      ((struct cleanup *) -1) will]
    - displays as something useful when printed in gdb.  */
-static struct cleanup cleanup_sentinel;
+static const struct cleanup cleanup_sentinel;

 /* Handy macro to use when referring to cleanup_sentinel.  */
-#define CLEANUP_SENTINEL (&cleanup_sentinel)
+#define CLEANUP_SENTINEL ((struct cleanup *) &cleanup_sentinel)

 /* Chain of cleanup actions established with make_cleanup,
    to be executed if an error happens.  */

Re: [RFA] Ensure result of make_cleanup is never NULL.

[Pedro Alves]

On 04/18/2012 03:02 PM, Doug Evans wrote:

> On Wed, Apr 18, 2012 at 2:26 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 04/18/2012 01:43 AM, Doug Evans wrote:
>>
>>> btw, I thought about making cleanup_sentinel const here, for a bit of
>>> extra robustness.
>>
>>
>> Won't that mean you'd need to cast the constness out in most, if
>> not all places that use it?


Well, aren't all places using the sentinel, using it throught the macro?
What does this make more robust?

> 
> --- cleanups.c= 2012-04-17 12:02:38.000000000 -0700
> +++ cleanups.c  2012-04-18 06:59:46.000000000 -0700
> @@ -50,10 +50,10 @@ struct cleanup
>       [though this won't catch errors that a value of, say,
>       ((struct cleanup *) -1) will]
>     - displays as something useful when printed in gdb.  */
> -static struct cleanup cleanup_sentinel;
> +static const struct cleanup cleanup_sentinel;
> 
>  /* Handy macro to use when referring to cleanup_sentinel.  */
> -#define CLEANUP_SENTINEL (&cleanup_sentinel)
> +#define CLEANUP_SENTINEL ((struct cleanup *) &cleanup_sentinel)
> 
>  /* Chain of cleanup actions established with make_cleanup,
>     to be executed if an error happens.  */



-- 
Pedro Alves

Re: [RFA] Ensure result of make_cleanup is never NULL.

[Doug Evans]

On Wed, Apr 18, 2012 at 7:23 AM, Pedro Alves <palves@redhat.com> wrote:
> On 04/18/2012 03:02 PM, Doug Evans wrote:
>
>> On Wed, Apr 18, 2012 at 2:26 AM, Pedro Alves <palves@redhat.com> wrote:
>>> On 04/18/2012 01:43 AM, Doug Evans wrote:
>>>
>>>> btw, I thought about making cleanup_sentinel const here, for a bit of
>>>> extra robustness.
>>>
>>>
>>> Won't that mean you'd need to cast the constness out in most, if
>>> not all places that use it?
>
>
> Well, aren't all places using the sentinel, using it throught the macro?
> What does this make more robust?


Any attempts to modify the sentinel will segv.


>>
>> --- cleanups.c= 2012-04-17 12:02:38.000000000 -0700
>> +++ cleanups.c  2012-04-18 06:59:46.000000000 -0700
>> @@ -50,10 +50,10 @@ struct cleanup
>>       [though this won't catch errors that a value of, say,
>>       ((struct cleanup *) -1) will]
>>     - displays as something useful when printed in gdb.  */
>> -static struct cleanup cleanup_sentinel;
>> +static const struct cleanup cleanup_sentinel;
>>
>>  /* Handy macro to use when referring to cleanup_sentinel.  */
>> -#define CLEANUP_SENTINEL (&cleanup_sentinel)
>> +#define CLEANUP_SENTINEL ((struct cleanup *) &cleanup_sentinel)
>>
>>  /* Chain of cleanup actions established with make_cleanup,
>>     to be executed if an error happens.  */

Re: [RFA] Ensure result of make_cleanup is never NULL.

[Pedro Alves]

On 04/18/2012 03:31 PM, Doug Evans wrote:

> On Wed, Apr 18, 2012 at 7:23 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 04/18/2012 03:02 PM, Doug Evans wrote:
>>
>>> On Wed, Apr 18, 2012 at 2:26 AM, Pedro Alves <palves@redhat.com> wrote:
>>>> On 04/18/2012 01:43 AM, Doug Evans wrote:
>>>>
>>>>> btw, I thought about making cleanup_sentinel const here, for a bit of
>>>>> extra robustness.
>>>>
>>>>
>>>> Won't that mean you'd need to cast the constness out in most, if
>>>> not all places that use it?
>>
>>
>> Well, aren't all places using the sentinel, using it throught the macro?
>> What does this make more robust?
> 
> 
> Any attempts to modify the sentinel will segv.


Ah.  Nothing like explaining things clearly.  Good idea then.  :-)

-- 
Pedro Alves

Re: [RFA] Ensure result of make_cleanup is never NULL.

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Well, aren't all places using the sentinel, using it throught the macro?
Pedro> What does this make more robust?

If you initialize the object it can end up in a read-only section, so
writes to it will SEGV.

In this particular case I don't see that this would help a lot, since
the object is just a sentinel.  But, it wouldn't hurt and maybe it would
catch some really unusual bug.

Tom

Re: [RFA] Ensure result of make_cleanup is never NULL.

[Doug Evans]

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

On Wed, Apr 18, 2012 at 7:36 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
> Pedro> Well, aren't all places using the sentinel, using it throught the macro?
> Pedro> What does this make more robust?
>
> If you initialize the object it can end up in a read-only section, so
> writes to it will SEGV.
>
> In this particular case I don't see that this would help a lot, since
> the object is just a sentinel.  But, it wouldn't hurt and maybe it would
> catch some really unusual bug.

Hi.  fyi, here is what I committed.

I changed the name cleanup_sentinel -> sentinel_cleanup because I went
to print the value in gdb and spelled it the latter way and when that
failed I cursed because I was darn sure I had spelled it correctly.
That told me I liked the latter spelling better. :-)   Anyways ...

2012-04-19  Doug Evans  <dje@google.com>

        * cleanups.h (struct cleanup): Move to cleanups.c.
        (make_cleanup_dtor_ftype): New typedef.
        (make_cleanup_dtor): Use it.
        (ALL_CLEANUPS): Replace with ...
        (all_cleanups): ... this.  Declare.  All uses updated.
        * cleanups.c: #include "gdb_assert.h".
        (sentinel_cleanup): New static global.
        (SENTINEL_CLEANUP): Define.
        (cleanup_chain, final_cleanup_chain): Initialize to SENTINEL_CLEANUP.
        (make_my_cleanup2): Assert result is non-NULL.
        (all_cleanups): New function.
        (save_my_cleanups): Initialize new chain to SENTINEL_CLEANUP instead
        of NULL.

[-- Attachment #2: gdb-120419-cleanups-3b.patch.txt --]
[-- Type: text/plain, Size: 8427 bytes --]

2012-04-19  Doug Evans  <dje@google.com>

	* cleanups.h (struct cleanup): Move to cleanups.c.
	(make_cleanup_dtor_ftype): New typedef.
	(make_cleanup_dtor): Use it.
	(ALL_CLEANUPS): Replace with ...
	(all_cleanups): ... this.  Declare.  All uses updated.
	* cleanups.c: #include "gdb_assert.h".
	(sentinel_cleanup): New static global.
	(SENTINEL_CLEANUP): Define.
	(cleanup_chain, final_cleanup_chain): Initialize to SENTINEL_CLEANUP.
	(make_my_cleanup2): Assert result is non-NULL.
	(all_cleanups): New function.
	(save_my_cleanups): Initialize new chain to SENTINEL_CLEANUP instead
	of NULL.

Index: cleanups.c
===================================================================
RCS file: /cvs/src/src/gdb/cleanups.c,v
retrieving revision 1.2
diff -u -p -r1.2 cleanups.c
--- cleanups.c	17 Apr 2012 21:24:47 -0000	1.2
+++ cleanups.c	19 Apr 2012 17:52:44 -0000
@@ -18,15 +18,53 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "gdb_assert.h"
+
+/* The cleanup list records things that have to be undone
+   if an error happens (descriptors to be closed, memory to be freed, etc.)
+   Each link in the chain records a function to call and an
+   argument to give it.
+
+   Use make_cleanup to add an element to the cleanup chain.
+   Use do_cleanups to do all cleanup actions back to a given
+   point in the chain.  Use discard_cleanups to remove cleanups
+   from the chain back to a given point, not doing them.
+
+   If the argument is pointer to allocated memory, then you need
+   to additionally set the 'free_arg' member to a function that will
+   free that memory.  This function will be called both when the cleanup
+   is executed and when it's discarded.  */
+
+struct cleanup
+{
+  struct cleanup *next;
+  void (*function) (void *);
+  void (*free_arg) (void *);
+  void *arg;
+};
+
+/* Used to mark the end of a cleanup chain.
+   The value is chosen so that it:
+   - is non-NULL so that make_cleanup never returns NULL,
+   - causes a segv if dereferenced
+     [though this won't catch errors that a value of, say,
+     ((struct cleanup *) -1) will]
+   - displays as something useful when printed in gdb.
+   This is const for a bit of extra robustness.
+   It is initialized to coax gcc into putting it into .rodata.
+   All fields are initialized to survive -Wextra.  */
+static const struct cleanup sentinel_cleanup = { 0, 0, 0, 0 };
+
+/* Handy macro to use when referring to sentinel_cleanup.  */
+#define SENTINEL_CLEANUP ((struct cleanup *) &sentinel_cleanup)
 
 /* Chain of cleanup actions established with make_cleanup,
    to be executed if an error happens.  */
+static struct cleanup *cleanup_chain = SENTINEL_CLEANUP;
 
-/* Cleaned up after a failed command.  */
-static struct cleanup *cleanup_chain;
-
-/* Cleaned up when gdb exits.  */
-static struct cleanup *final_cleanup_chain;
+/* Chain of cleanup actions established with make_final_cleanup,
+   to be executed when gdb exits.  */
+static struct cleanup *final_cleanup_chain = SENTINEL_CLEANUP;
 
 /* Main worker routine to create a cleanup.
    PMY_CHAIN is a pointer to either cleanup_chain or final_cleanup_chain.
@@ -51,6 +89,7 @@ make_my_cleanup2 (struct cleanup **pmy_c
   new->arg = arg;
   *pmy_chain = new;
 
+  gdb_assert (old_chain != NULL);
   return old_chain;
 }
 
@@ -120,6 +159,15 @@ do_my_cleanups (struct cleanup **pmy_cha
     }
 }
 
+/* Return a value that can be passed to do_cleanups, do_final_cleanups to
+   indicate perform all cleanups.  */
+
+struct cleanup *
+all_cleanups (void)
+{
+  return SENTINEL_CLEANUP;
+}
+
 /* Discard cleanups and do the actions they describe
    until we get back to the point OLD_CHAIN in the cleanup_chain.  */
 
@@ -185,7 +233,7 @@ save_my_cleanups (struct cleanup **pmy_c
 {
   struct cleanup *old_chain = *pmy_chain;
 
-  *pmy_chain = 0;
+  *pmy_chain = SENTINEL_CLEANUP;
   return old_chain;
 }
 
Index: cleanups.h
===================================================================
RCS file: /cvs/src/src/gdb/cleanups.h,v
retrieving revision 1.2
diff -u -p -r1.2 cleanups.h
--- cleanups.h	17 Apr 2012 21:24:47 -0000	1.2
+++ cleanups.h	19 Apr 2012 17:52:44 -0000
@@ -19,28 +19,8 @@
 #ifndef CLEANUPS_H
 #define CLEANUPS_H
 
-/* The cleanup list records things that have to be undone
-   if an error happens (descriptors to be closed, memory to be freed, etc.)
-   Each link in the chain records a function to call and an
-   argument to give it.
-
-   Use make_cleanup to add an element to the cleanup chain.
-   Use do_cleanups to do all cleanup actions back to a given
-   point in the chain.  Use discard_cleanups to remove cleanups
-   from the chain back to a given point, not doing them.
-
-   If the argument is pointer to allocated memory, then you need
-   to additionally set the 'free_arg' member to a function that will
-   free that memory.  This function will be called both when the cleanup
-   is executed and when it's discarded.  */
-
-struct cleanup
-  {
-    struct cleanup *next;
-    void (*function) (void *);
-    void (*free_arg) (void *);
-    void *arg;
-  };
+/* Outside of cleanups.c, this is an opaque type.  */
+struct cleanup;
 
 /* NOTE: cagney/2000-03-04: This typedef is strictly for the
    make_cleanup function declarations below.  Do not use this typedef
@@ -49,21 +29,25 @@ struct cleanup
    Calling a f(char*) function with f(void*) is non-portable.  */
 typedef void (make_cleanup_ftype) (void *);
 
+/* Function type for the dtor in make_cleanup_dtor.  */
+typedef void (make_cleanup_dtor_ftype) (void *);
+
 /* WARNING: The result of the "make cleanup" routines is not the intuitive
    choice of being a handle on the just-created cleanup.  Instead it is an
    opaque handle of the cleanup mechanism and represents all cleanups created
-   from that point onwards.  */
+   from that point onwards.
+   The result is guaranteed to be non-NULL though.  */
 
 extern struct cleanup *make_cleanup (make_cleanup_ftype *, void *);
 
 extern struct cleanup *make_cleanup_dtor (make_cleanup_ftype *, void *,
-					  void (*dtor) (void *));
+					  make_cleanup_dtor_ftype *);
 
 extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void *);
 
 /* A special value to pass to do_cleanups and do_final_cleanups
    to tell them to do all cleanups.  */
-#define	ALL_CLEANUPS	((struct cleanup *)0)
+extern struct cleanup *all_cleanups (void);
 
 extern void do_cleanups (struct cleanup *);
 extern void do_final_cleanups (struct cleanup *);
Index: exceptions.c
===================================================================
RCS file: /cvs/src/src/gdb/exceptions.c,v
retrieving revision 1.50
diff -u -p -r1.50 exceptions.c
--- exceptions.c	4 Jan 2012 08:17:01 -0000	1.50
+++ exceptions.c	19 Apr 2012 17:52:44 -0000
@@ -224,7 +224,7 @@ throw_exception (struct gdb_exception ex
   quit_flag = 0;
   immediate_quit = 0;
 
-  do_cleanups (ALL_CLEANUPS);
+  do_cleanups (all_cleanups ());
 
   /* Jump to the containing catch_errors() call, communicating REASON
      to that call via setjmp's return value.  Note that REASON can't
Index: main.c
===================================================================
RCS file: /cvs/src/src/gdb/main.c,v
retrieving revision 1.108
diff -u -p -r1.108 main.c
--- main.c	17 Apr 2012 15:56:21 -0000	1.108
+++ main.c	19 Apr 2012 17:52:44 -0000
@@ -231,7 +231,7 @@ captured_command_loop (void *data)
      are not that well behaved.  do_cleanups should either be replaced
      with a do_cleanups call (to cover the problem) or an assertion
      check to detect bad FUNCs code.  */
-  do_cleanups (ALL_CLEANUPS);
+  do_cleanups (all_cleanups ());
   /* If the command_loop returned, normally (rather than threw an
      error) we try to quit.  If the quit is aborted, catch_errors()
      which called this catch the signal and restart the command
Index: top.c
===================================================================
RCS file: /cvs/src/src/gdb/top.c,v
retrieving revision 1.214
diff -u -p -r1.214 top.c
--- top.c	1 Mar 2012 19:30:20 -0000	1.214
+++ top.c	19 Apr 2012 17:52:44 -0000
@@ -1297,8 +1297,9 @@ quit_target (void *arg)
   if (write_history_p && history_filename)
     write_history (history_filename);
 
-  do_final_cleanups (ALL_CLEANUPS);    /* Do any final cleanups before
-					  exiting.  */
+  /* Do any final cleanups before exiting.  */
+  do_final_cleanups (all_cleanups ());
+
   return 0;
 }