PATCH: Initialize tmp_obstack

PATCH: Initialize tmp_obstack

[H. J. Lu]

Gcc 4.1 warns

cc1: warnings being treated as errors
/export/gnu/src/gdb/gdb/gdb/p-valprint.c: In function
‘pascal_object_print_value_fields’:
/export/gnu/src/gdb/gdb/gdb/p-valprint.c:756: warning:
‘tmp_obstack.alloc_failed’ may be used uninitialized in this function

This patch fixes those.

H.J.
----
2006-12-02  H.J. Lu  <hongjiu.lu@intel.com>

	* cp-valprint.c (cp_print_value_fields): Initialize tmp_obstack.
	(cp_print_value): Likewise
	* p-valprint.c (pascal_object_print_value_fields): Likewise
	(pascal_object_print_value): Likewise

--- gdb/cp-valprint.c.init	2005-12-17 14:33:59.000000000 -0800
+++ gdb/cp-valprint.c	2006-12-02 10:23:21.000000000 -0800
@@ -265,7 +265,7 @@ cp_print_value_fields (struct type *type
 		       struct type **dont_print_vb,int dont_print_statmem)
 {
   int i, len, n_baseclasses;
-  struct obstack tmp_obstack;
+  struct obstack tmp_obstack = { 0 };
   char *last_dont_print = obstack_next_free (&dont_print_statmem_obstack);
   int fields_seen = 0;
 
@@ -524,7 +524,7 @@ cp_print_value (struct type *type, struc
 		struct ui_file *stream, int format, int recurse,
 		enum val_prettyprint pretty, struct type **dont_print_vb)
 {
-  struct obstack tmp_obstack;
+  struct obstack tmp_obstack = { 0 };
   struct type **last_dont_print
     = (struct type **) obstack_next_free (&dont_print_vb_obstack);
   int i, n_baseclasses = TYPE_N_BASECLASSES (type);
--- gdb/p-valprint.c.init	2006-12-02 09:50:54.000000000 -0800
+++ gdb/p-valprint.c	2006-12-02 10:22:35.000000000 -0800
@@ -753,7 +753,7 @@ pascal_object_print_value_fields (struct
 				  int dont_print_statmem)
 {
   int i, len, n_baseclasses;
-  struct obstack tmp_obstack;
+  struct obstack tmp_obstack = { 0 };
   char *last_dont_print = obstack_next_free (&dont_print_statmem_obstack);
 
   CHECK_TYPEDEF (type);
@@ -922,7 +922,7 @@ pascal_object_print_value (struct type *
 			   enum val_prettyprint pretty,
 			   struct type **dont_print_vb)
 {
-  struct obstack tmp_obstack;
+  struct obstack tmp_obstack = { 0 };
   struct type **last_dont_print
   = (struct type **) obstack_next_free (&dont_print_vb_obstack);
   int i, n_baseclasses = TYPE_N_BASECLASSES (type);

Re: PATCH: Initialize tmp_obstack

[Daniel Jacobowitz]

On Sat, Dec 02, 2006 at 10:27:12AM -0800, H. J. Lu wrote:
> Gcc 4.1 warns

Mine doesn't, but I never did figure out why some compilers do and
others don't.

> 2006-12-02  H.J. Lu  <hongjiu.lu@intel.com>
> 
> 	* cp-valprint.c (cp_print_value_fields): Initialize tmp_obstack.
> 	(cp_print_value): Likewise
> 	* p-valprint.c (pascal_object_print_value_fields): Likewise
> 	(pascal_object_print_value): Likewise

OK to commit - let's use this version.  Thanks.  I think it's fine for
the 4.2 branch too.

-- 
Daniel Jacobowitz
CodeSourcery

Re: PATCH: Initialize tmp_obstack

[H. J. Lu]

On Tue, Dec 05, 2006 at 03:40:03PM -0500, Daniel Jacobowitz wrote:
> On Sat, Dec 02, 2006 at 10:27:12AM -0800, H. J. Lu wrote:
> > Gcc 4.1 warns
> 
> Mine doesn't, but I never did figure out why some compilers do and
> others don't.
> 
> > 2006-12-02  H.J. Lu  <hongjiu.lu@intel.com>
> > 
> > 	* cp-valprint.c (cp_print_value_fields): Initialize tmp_obstack.
> > 	(cp_print_value): Likewise
> > 	* p-valprint.c (pascal_object_print_value_fields): Likewise
> > 	(pascal_object_print_value): Likewise
> 
> OK to commit - let's use this version.  Thanks.  I think it's fine for
> the 4.2 branch too.

I assume that you meant gdb 6.5 branch.


H.J.

Re: PATCH: Initialize tmp_obstack

[Daniel Jacobowitz]

On Tue, Dec 05, 2006 at 12:56:38PM -0800, H. J. Lu wrote:
> I assume that you meant gdb 6.5 branch.

Actually I meant 6.6.  Sorry.  You checked it in to the right place,
anyway.

-- 
Daniel Jacobowitz
CodeSourcery

Re: PATCH: Initialize tmp_obstack

[Daniel Jacobowitz]

On Tue, Dec 05, 2006 at 10:48:40PM +0100, Mark Kettenis wrote:
> Sorry, yes, this was meant to go to the list.  My proposal is not to "fix"
> this at all.  It's a GCC bug, that's presumably fixed, and we'll ship
> with -Werror turned off by default anyway.

Well, there seem to be enough systems with affected GCC that HJ, Jim,
and I all hit it - Ubuntu Edgy is one such.

We could discuss this till we're blue in the face without getting
anywhere; the uninitialized warnings are not reliable in face of this
sort of pattern (isn't it a classical example of why uninitialized
warnings are hard?).

I think we're being overly optimistic if we expect GCC to get it right
all the time.  In fact, I asked a bunch of GCC developers about it on
IRC and the best response I got was "then you are screwed".  Or Diego's
explanation:

> We don't explicitly try to handle it.  it's mostly side-effect of
> various optimizations.  some times it's jump-threading, others it's
> PRE, others it's CCP, others it's VRP.

> that irritates the hell out of me.  we depend on optimizations for
> this warning.

I know that's a widely held position.

-- 
Daniel Jacobowitz
CodeSourcery

Re: PATCH: Initialize tmp_obstack

[Daniel Jacobowitz]

On Tue, Dec 05, 2006 at 04:56:39PM -0500, Daniel Jacobowitz wrote:
> I think we're being overly optimistic if we expect GCC to get it right
> all the time.  In fact, I asked a bunch of GCC developers about it on
> IRC and the best response I got was "then you are screwed".  Or Diego's
> explanation:
> 
> > We don't explicitly try to handle it.  it's mostly side-effect of
> > various optimizations.  some times it's jump-threading, others it's
> > PRE, others it's CCP, others it's VRP.
> 
> > that irritates the hell out of me.  we depend on optimizations for
> > this warning.
> 
> I know that's a widely held position.

For the record, his followup makes that come out a bit differently:

> I would love for someone to tackle this particular warning in
> an explicit and deterministic way.

-- 
Daniel Jacobowitz
CodeSourcery

Re: PATCH: Initialize tmp_obstack

[Jim Blandy]

Daniel Jacobowitz <drow@false.org> writes:
> On Tue, Dec 05, 2006 at 04:56:39PM -0500, Daniel Jacobowitz wrote:
>> I think we're being overly optimistic if we expect GCC to get it right
>> all the time.  In fact, I asked a bunch of GCC developers about it on
>> IRC and the best response I got was "then you are screwed".  Or Diego's
>> explanation:
>> 
>> > We don't explicitly try to handle it.  it's mostly side-effect of
>> > various optimizations.  some times it's jump-threading, others it's
>> > PRE, others it's CCP, others it's VRP.
>> 
>> > that irritates the hell out of me.  we depend on optimizations for
>> > this warning.
>> 
>> I know that's a widely held position.
>
> For the record, his followup makes that come out a bit differently:
>
>> I would love for someone to tackle this particular warning in
>> an explicit and deterministic way.

Solving it correctly is Turing-complete; I wonder what Diego had in
mind.

One alternative I've seen is Java's "definite assignment" rule: the
language spec actually dictates a (not entirely dumb but not too hot
either) flow analysis that is used to decide (if I remember right)
whether you must initialize a varable or not.  It's not The C Way to
require initialization, but GCC could simply fix a particular
algorithm for the sake of producing that warning.

Re: PATCH: Initialize tmp_obstack

[Daniel Jacobowitz]

Since there's disagreement about this patch, I have reverted it.  We
can put it back in when there's consensus.

-- 
Daniel Jacobowitz
CodeSourcery

Re: PATCH: Initialize tmp_obstack

[Daniel Jacobowitz]

On Tue, Dec 05, 2006 at 06:58:48PM -0500, Daniel Jacobowitz wrote:
> Since there's disagreement about this patch, I have reverted it.  We
> can put it back in when there's consensus.

Unfortunately, when I reverted it the discussion stopped dead without
advancing towards consensus.

What do we do about this warning?

-- 
Daniel Jacobowitz
CodeSourcery

Re: PATCH: Initialize tmp_obstack

[Daniel Jacobowitz]

On Thu, Dec 07, 2006 at 09:40:18AM -0500, Daniel Jacobowitz wrote:
> On Tue, Dec 05, 2006 at 06:58:48PM -0500, Daniel Jacobowitz wrote:
> > Since there's disagreement about this patch, I have reverted it.  We
> > can put it back in when there's consensus.
> 
> Unfortunately, when I reverted it the discussion stopped dead without
> advancing towards consensus.
> 
> What do we do about this warning?

No one has replied.  While I agree that GCC is not being very helpful
here, I don't anticipate a reliable compiler fix, and it's really
beginning to frustrate me that I can't use -Werror on my laptop
(I'm travelling).

Mark, how strongly do you object to HJ's workaround?  If it's
unacceptable, will you volunteer to simplify the affected functions
to avoid the warning?

-- 
Daniel Jacobowitz
CodeSourcery

Re: PATCH: Initialize tmp_obstack

[Daniel Jacobowitz]

On Sun, Dec 24, 2006 at 11:00:32PM -0500, Daniel Jacobowitz wrote:
> On Thu, Dec 07, 2006 at 09:40:18AM -0500, Daniel Jacobowitz wrote:
> > On Tue, Dec 05, 2006 at 06:58:48PM -0500, Daniel Jacobowitz wrote:
> > > Since there's disagreement about this patch, I have reverted it.  We
> > > can put it back in when there's consensus.
> > 
> > Unfortunately, when I reverted it the discussion stopped dead without
> > advancing towards consensus.
> > 
> > What do we do about this warning?
> 
> No one has replied.  While I agree that GCC is not being very helpful
> here, I don't anticipate a reliable compiler fix, and it's really
> beginning to frustrate me that I can't use -Werror on my laptop
> (I'm travelling).
> 
> Mark, how strongly do you object to HJ's workaround?  If it's
> unacceptable, will you volunteer to simplify the affected functions
> to avoid the warning?

I'm travelling again - excuse spotty response this week, please.  But
that means I'm hacking on GDB on my laptop again, which is affected by
this problem.

How can we avoid the uninitialized warnings?

-- 
Daniel Jacobowitz
CodeSourcery

Re: PATCH: Initialize tmp_obstack

[Mark Kettenis]

> Date: Tue, 16 Jan 2007 01:59:38 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Sun, Dec 24, 2006 at 11:00:32PM -0500, Daniel Jacobowitz wrote:
> > On Thu, Dec 07, 2006 at 09:40:18AM -0500, Daniel Jacobowitz wrote:
> > > On Tue, Dec 05, 2006 at 06:58:48PM -0500, Daniel Jacobowitz wrote:
> > > > Since there's disagreement about this patch, I have reverted it.  We
> > > > can put it back in when there's consensus.
> > > 
> > > Unfortunately, when I reverted it the discussion stopped dead without
> > > advancing towards consensus.
> > > 
> > > What do we do about this warning?
> > 
> > No one has replied.  While I agree that GCC is not being very helpful
> > here, I don't anticipate a reliable compiler fix, and it's really
> > beginning to frustrate me that I can't use -Werror on my laptop
> > (I'm travelling).
> > 
> > Mark, how strongly do you object to HJ's workaround?  If it's
> > unacceptable, will you volunteer to simplify the affected functions
> > to avoid the warning?
> 
> I'm travelling again - excuse spotty response this week, please.  But
> that means I'm hacking on GDB on my laptop again, which is affected by
> this problem.
> 
> How can we avoid the uninitialized warnings?

I've looked at the code, I couldn't see an easy way.  I couldn't find
an easy way because the code is actually very hard to understand.  I
really think the code should be rewritten such that it is easier to
understand.

Mark

Re: PATCH: Initialize tmp_obstack

[Mark Kettenis]

> Date: Tue, 16 Jan 2007 01:59:38 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Sun, Dec 24, 2006 at 11:00:32PM -0500, Daniel Jacobowitz wrote:
>
> I'm travelling again - excuse spotty response this week, please.  But
> that means I'm hacking on GDB on my laptop again, which is affected by
> this problem.
> 
> How can we avoid the uninitialized warnings?

I had another look and come up with the attached.  By always
initializing tmp_obstack the code is less optimal, but at least I can
convince myself the code is safe now.  Unfortunately I can't test
whether this fixes the GCC 4.1 warnings.

Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	* cp-valprint.c (cp_print_value_fields, cp_print_value): Always
	initialize tmp_obstack.

Index: cp-valprint.c
===================================================================
RCS file: /cvs/src/src/gdb/cp-valprint.c,v
retrieving revision 1.47
diff -u -p -r1.47 cp-valprint.c
--- cp-valprint.c 9 Jan 2007 17:58:50 -0000 1.47
+++ cp-valprint.c 21 Jan 2007 12:44:23 -0000
@@ -167,7 +167,6 @@ cp_print_value_fields (struct type *type
 		       struct type **dont_print_vb,int dont_print_statmem)
 {
   int i, len, n_baseclasses;
-  struct obstack tmp_obstack;
   char *last_dont_print = obstack_next_free (&dont_print_statmem_obstack);
   int fields_seen = 0;
 
@@ -197,12 +196,13 @@ cp_print_value_fields (struct type *type
     fprintf_filtered (stream, "<No data fields>");
   else
     {
+      struct obstack tmp_obstack = dont_print_statmem_obstack;
+
       if (dont_print_statmem == 0)
 	{
 	  /* If we're at top level, carve out a completely fresh
 	     chunk of the obstack and use that until this particular
 	     invocation returns.  */
-	  tmp_obstack = dont_print_statmem_obstack;
 	  obstack_finish (&dont_print_statmem_obstack);
 	}
 
@@ -426,9 +426,9 @@ cp_print_value (struct type *type, struc
 		struct ui_file *stream, int format, int recurse,
 		enum val_prettyprint pretty, struct type **dont_print_vb)
 {
-  struct obstack tmp_obstack;
   struct type **last_dont_print
     = (struct type **) obstack_next_free (&dont_print_vb_obstack);
+  struct obstack tmp_obstack = dont_print_vb_obstack;
   int i, n_baseclasses = TYPE_N_BASECLASSES (type);
   int thisoffset;
   struct type *thistype;
@@ -438,7 +438,6 @@ cp_print_value (struct type *type, struc
       /* If we're at top level, carve out a completely fresh
          chunk of the obstack and use that until this particular
          invocation returns.  */
-      tmp_obstack = dont_print_vb_obstack;
       /* Bump up the high-water mark.  Now alpha is omega.  */
       obstack_finish (&dont_print_vb_obstack);
     }

Re: PATCH: Initialize tmp_obstack

[Daniel Jacobowitz]

On Sun, Jan 21, 2007 at 02:07:14PM +0100, Mark Kettenis wrote:
> I had another look and come up with the attached.  By always
> initializing tmp_obstack the code is less optimal, but at least I can
> convince myself the code is safe now.  Unfortunately I can't test
> whether this fixes the GCC 4.1 warnings.

Thanks!  I can test it - I'll let you know this afternoon.

p-valprint.c has the same problem, but the correct fix there is going
to be to delete some code.  Pascal copied a lot from C/C++ but it
doesn't actually have static members to the best of my knowledge.
I see some discussion of them as planned for GNU Pascal, but no
implementations.

-- 
Daniel Jacobowitz
CodeSourcery

Re: PATCH: Initialize tmp_obstack

[Daniel Jacobowitz]

On Sun, Jan 21, 2007 at 10:59:41AM -0500, Daniel Jacobowitz wrote:
> On Sun, Jan 21, 2007 at 02:07:14PM +0100, Mark Kettenis wrote:
> > I had another look and come up with the attached.  By always
> > initializing tmp_obstack the code is less optimal, but at least I can
> > convince myself the code is safe now.  Unfortunately I can't test
> > whether this fixes the GCC 4.1 warnings.
> 
> Thanks!  I can test it - I'll let you know this afternoon.

Works fine.

> p-valprint.c has the same problem, but the correct fix there is going
> to be to delete some code.  Pascal copied a lot from C/C++ but it
> doesn't actually have static members to the best of my knowledge.
> I see some discussion of them as planned for GNU Pascal, but no
> implementations.

But it was easier to copy your fix into p-valprint.c than to untangle
which parts of this Pascal really needs and which parts it just copied.
Wish we had some test coverage for pascal!

See attached; cp-valprint.c parts unchanged, p-valprint.c parts copied
from that and compile tested only.

-- 
Daniel Jacobowitz
CodeSourcery

from  Mark Kettenis  <kettenis@gnu.org>

	* cp-valprint.c (cp_print_value_fields, cp_print_value): Always
	initialize tmp_obstack.
	* p-valprint.c (pascal_object_print_value_fields)
	(pascal_object_print_value): Likewise.

---
 gdb/cp-valprint.c |    7 +++----
 gdb/p-valprint.c  |    6 ++----
 2 files changed, 5 insertions(+), 8 deletions(-)

Index: src/gdb/cp-valprint.c
===================================================================
--- src.orig/gdb/cp-valprint.c	2007-01-21 12:17:05.000000000 -0500
+++ src/gdb/cp-valprint.c	2007-01-21 12:17:09.000000000 -0500
@@ -167,7 +167,6 @@ cp_print_value_fields (struct type *type
 		       struct type **dont_print_vb,int dont_print_statmem)
 {
   int i, len, n_baseclasses;
-  struct obstack tmp_obstack;
   char *last_dont_print = obstack_next_free (&dont_print_statmem_obstack);
   int fields_seen = 0;
 
@@ -197,12 +196,13 @@ cp_print_value_fields (struct type *type
     fprintf_filtered (stream, "<No data fields>");
   else
     {
+      struct obstack tmp_obstack = dont_print_statmem_obstack;
+
       if (dont_print_statmem == 0)
 	{
 	  /* If we're at top level, carve out a completely fresh
 	     chunk of the obstack and use that until this particular
 	     invocation returns.  */
-	  tmp_obstack = dont_print_statmem_obstack;
 	  obstack_finish (&dont_print_statmem_obstack);
 	}
 
@@ -426,9 +426,9 @@ cp_print_value (struct type *type, struc
 		struct ui_file *stream, int format, int recurse,
 		enum val_prettyprint pretty, struct type **dont_print_vb)
 {
-  struct obstack tmp_obstack;
   struct type **last_dont_print
     = (struct type **) obstack_next_free (&dont_print_vb_obstack);
+  struct obstack tmp_obstack = dont_print_vb_obstack;
   int i, n_baseclasses = TYPE_N_BASECLASSES (type);
   int thisoffset;
   struct type *thistype;
@@ -438,7 +438,6 @@ cp_print_value (struct type *type, struc
       /* If we're at top level, carve out a completely fresh
          chunk of the obstack and use that until this particular
          invocation returns.  */
-      tmp_obstack = dont_print_vb_obstack;
       /* Bump up the high-water mark.  Now alpha is omega.  */
       obstack_finish (&dont_print_vb_obstack);
     }
Index: src/gdb/p-valprint.c
===================================================================
--- src.orig/gdb/p-valprint.c	2007-01-21 12:17:07.000000000 -0500
+++ src/gdb/p-valprint.c	2007-01-21 12:20:40.000000000 -0500
@@ -641,7 +641,6 @@ pascal_object_print_value_fields (struct
 				  int dont_print_statmem)
 {
   int i, len, n_baseclasses;
-  struct obstack tmp_obstack;
   char *last_dont_print = obstack_next_free (&dont_print_statmem_obstack);
 
   CHECK_TYPEDEF (type);
@@ -660,6 +659,7 @@ pascal_object_print_value_fields (struct
     fprintf_filtered (stream, "<No data fields>");
   else
     {
+      struct obstack tmp_obstack = dont_print_statmem_obstack;
       int fields_seen = 0;
 
       if (dont_print_statmem == 0)
@@ -667,7 +667,6 @@ pascal_object_print_value_fields (struct
 	  /* If we're at top level, carve out a completely fresh
 	     chunk of the obstack and use that until this particular
 	     invocation returns.  */
-	  tmp_obstack = dont_print_statmem_obstack;
 	  obstack_finish (&dont_print_statmem_obstack);
 	}
 
@@ -810,9 +809,9 @@ pascal_object_print_value (struct type *
 			   enum val_prettyprint pretty,
 			   struct type **dont_print_vb)
 {
-  struct obstack tmp_obstack;
   struct type **last_dont_print
   = (struct type **) obstack_next_free (&dont_print_vb_obstack);
+  struct obstack tmp_obstack = dont_print_vb_obstack;
   int i, n_baseclasses = TYPE_N_BASECLASSES (type);
 
   if (dont_print_vb == 0)
@@ -820,7 +819,6 @@ pascal_object_print_value (struct type *
       /* If we're at top level, carve out a completely fresh
          chunk of the obstack and use that until this particular
          invocation returns.  */
-      tmp_obstack = dont_print_vb_obstack;
       /* Bump up the high-water mark.  Now alpha is omega.  */
       obstack_finish (&dont_print_vb_obstack);
     }

Re: PATCH: Initialize tmp_obstack

[Daniel Jacobowitz]

On Sun, Jan 21, 2007 at 12:24:26PM -0500, Daniel Jacobowitz wrote:
> from  Mark Kettenis  <kettenis@gnu.org>
> 
> 	* cp-valprint.c (cp_print_value_fields, cp_print_value): Always
> 	initialize tmp_obstack.
> 	* p-valprint.c (pascal_object_print_value_fields)
> 	(pascal_object_print_value): Likewise.

I've belatedly checked this in.

-- 
Daniel Jacobowitz
CodeSourcery