FYI: fix PR 9708

FYI: fix PR 9708

[Tom Tromey]

I'm checking this in.

This fixes PR c++/9708.  The bug is that a method-scoped static variable
can't currently be printed by gdb.  This happens because the variable is
erroneously put into a namespace.

Built and regtested on x86-64 (compile farm).  New test case included.

Tom

2010-03-12  Tom Tromey  <tromey@redhat.com>

	PR c++/9708:
	* dwarf2read.c (die_needs_namespace) <DW_TAG_variable>: A variable
	in a lexical block does not need a namespace.
	(new_symbol) <DW_TAG_variable>: Put extern variables on
	list_in_scope in all cases.

2010-03-12  Tom Tromey  <tromey@redhat.com>

	PR c++/9708:
	* gdb.cp/m-static.exp: Add regression test.
	* gdb.cp/m-static.cc (method): New method.
	(main): Call it.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 53e2e1e..2d21edb 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -3204,6 +3204,8 @@ process_die (struct die_info *die, struct dwarf2_cu *cu)
 static int
 die_needs_namespace (struct die_info *die, struct dwarf2_cu *cu)
 {
+  struct attribute *attr;
+
   switch (die->tag)
     {
     case DW_TAG_namespace:
@@ -3231,11 +3233,17 @@ die_needs_namespace (struct die_info *die, struct dwarf2_cu *cu)
 				      spec_cu);
 	}
 
-      if (dwarf2_attr (die, DW_AT_external, cu)
-	  || die->parent->tag == DW_TAG_namespace)
-	return 1;
-
-      return 0;
+      attr = dwarf2_attr (die, DW_AT_external, cu);
+      if (attr == NULL && die->parent->tag != DW_TAG_namespace)
+	return 0;
+      /* A variable in a lexical block of some kind does not need a
+	 namespace, even though in C++ such variables may be external
+	 and have a mangled name.  */
+      if (die->parent->tag ==  DW_TAG_lexical_block
+	  || die->parent->tag ==  DW_TAG_try_block
+	  || die->parent->tag ==  DW_TAG_catch_block)
+	return 0;
+      return 1;
 
     default:
       return 0;
@@ -8413,7 +8421,15 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu)
 	      var_decode_location (attr, sym, cu);
 	      attr2 = dwarf2_attr (die, DW_AT_external, cu);
 	      if (attr2 && (DW_UNSND (attr2) != 0))
-		add_symbol_to_list (sym, &global_symbols);
+		{
+		  struct pending **list_to_add;
+
+		  /* A variable with DW_AT_external is never static,
+		     but it may be block-scoped.  */
+		  list_to_add = (cu->list_in_scope == &file_symbols
+				 ? &global_symbols : cu->list_in_scope);
+		  add_symbol_to_list (sym, list_to_add);
+		}
 	      else
 		add_symbol_to_list (sym, cu->list_in_scope);
 	    }
diff --git a/gdb/testsuite/gdb.cp/m-static.cc b/gdb/testsuite/gdb.cp/m-static.cc
index 2a0b61c..7f997ef 100644
--- a/gdb/testsuite/gdb.cp/m-static.cc
+++ b/gdb/testsuite/gdb.cp/m-static.cc
@@ -15,6 +15,12 @@ protected:
 
 public:
   gnu_obj_1(antiquities a, long l) {}
+
+  long method ()
+  {
+    static bool svar = true;
+    return key2;
+  }
 };
 
 const bool gnu_obj_1::test;
@@ -70,5 +76,8 @@ int main()
 
   test4.dummy = test4.elsewhere;
   test4.dummy = 0;
-  return test4.dummy;	// breakpoint: constructs-done
+
+  test1.method (); // breakpoint: constructs-done
+
+  return test4.dummy;
 }
diff --git a/gdb/testsuite/gdb.cp/m-static.exp b/gdb/testsuite/gdb.cp/m-static.exp
index f207462..7b4e0ca 100644
--- a/gdb/testsuite/gdb.cp/m-static.exp
+++ b/gdb/testsuite/gdb.cp/m-static.exp
@@ -132,5 +132,10 @@ gdb_test "print test4.nowhere" "field nowhere is nonexistent or has been optimis
 # that GDB's current behavior in such situations is either consistent
 # across platforms or optimal, so I'm not including one now.
 
+# Step into test1.method and examine the method-scoped static.
+# This is a regression test for PR 9708.
+gdb_test "step" "gnu_obj_1::method.*"
+gdb_test "print svar" " = true"
+
 gdb_exit
 return 0

Re: FYI: fix PR 9708

[Daniel Jacobowitz]

On Fri, Mar 12, 2010 at 05:25:05PM -0700, Tom Tromey wrote:
> +      attr = dwarf2_attr (die, DW_AT_external, cu);
> +      if (attr == NULL && die->parent->tag != DW_TAG_namespace)
> +	return 0;
> +      /* A variable in a lexical block of some kind does not need a
> +	 namespace, even though in C++ such variables may be external
> +	 and have a mangled name.  */
> +      if (die->parent->tag ==  DW_TAG_lexical_block
> +	  || die->parent->tag ==  DW_TAG_try_block
> +	  || die->parent->tag ==  DW_TAG_catch_block)
> +	return 0;
> +      return 1;

Why is this:

int f()
{
  static int x;
  ...
}

Different from this:

int f()
{
  if (foo)
    {
      static int x;
      ...
    }
}

Now one of those is going to get a namespace and the other isn't.
I'd have thought we would call them both f()::x - although clearly
that's not sufficient to uniquely identify them.  Should they
both be ! die_needs_namespace?

> +
> +  long method ()
> +  {
> +    static bool svar = true;
> +    return key2;
> +  }

Looking at your test case, I guess the answer is that you're relying on
GCC's extra DW_TAG_lexical_block emitted for all C++ functions; this
is not a DWARF requirement and other compilers omit it.

I'm sure I originally fixed some bug by giving these things the
physname of 'x'... but probably, whatever it was is also fixed by your
change to use list_in_scope.

-- 
Daniel Jacobowitz
CodeSourcery

Re: FYI: fix PR 9708

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <dan@codesourcery.com> writes:

[...]
Daniel> Now one of those is going to get a namespace and the other isn't.
Daniel> I'd have thought we would call them both f()::x - although clearly
Daniel> that's not sufficient to uniquely identify them.  Should they
Daniel> both be ! die_needs_namespace?

Yes, I think we should also omit DW_TAG_subprogram here.  I just didn't
think of it.  I can either just check it in, or if you have an easy way
to test it, let me know.  I've appended the patch.

Calling them f()::x might be interesting, but there are some caveats.

First, I am not sure the rest of gdb (I'm thinking the parser at least)
is set up to handle this.  AFAIK this would be a new feature.

Second, we'd probably have to insert two symbols here -- one for the
global name and one for the local name.  There are cases where gcc has
to give the static a synthetic linkage name, e.g.:

struct K {
  int m () {
    static bool themagicstatic = false;
    int x;
    {
      static bool themagicstatic = false;
      x = themagicstatic ? 23 : 24;
    }
    return x + themagicstatic ? 23 : 24;
  }
}

This leads to:

opsy. nm pr | grep themagicstatic
080497de V _ZZN1K1mEvE14themagicstatic
080497dd V _ZZN1K1mEvE14themagicstatic_0

.. so in the inner scope, we still need a symbol named "themagicstatic";
calling it "themagicstatic_0" would be confusing to the user.

Daniel> I'm sure I originally fixed some bug by giving these things the
Daniel> physname of 'x'... but probably, whatever it was is also fixed by your
Daniel> change to use list_in_scope.

Yeah, the die_needs_namespace change is needed to get the proper name
for the symbol in new_symbol.  Otherwise it ends up like
"K::themagicstatic", which is wrong.

Tom

*** dwarf2read.c.~1.366.~	2010-03-12 17:20:15.000000000 -0700
--- dwarf2read.c	2010-03-15 13:47:54.000000000 -0600
***************
*** 3241,3247 ****
  	 and have a mangled name.  */
        if (die->parent->tag ==  DW_TAG_lexical_block
  	  || die->parent->tag ==  DW_TAG_try_block
! 	  || die->parent->tag ==  DW_TAG_catch_block)
  	return 0;
        return 1;
  
--- 3241,3248 ----
  	 and have a mangled name.  */
        if (die->parent->tag ==  DW_TAG_lexical_block
  	  || die->parent->tag ==  DW_TAG_try_block
! 	  || die->parent->tag ==  DW_TAG_catch_block
! 	  || die->parent->tag == DW_TAG_subprogram)
  	return 0;
        return 1;
  

Re: FYI: fix PR 9708

[Daniel Jacobowitz]

On Mon, Mar 15, 2010 at 02:00:11PM -0600, Tom Tromey wrote:
> Yes, I think we should also omit DW_TAG_subprogram here.  I just didn't
> think of it.  I can either just check it in, or if you have an easy way
> to test it, let me know.  I've appended the patch.

The patch looks good to me; would you mind committing it?

I'm flushing out a bunch of fixed for the C parts of the GDB testsuite
with RealView; in a week or so, I think I'll be ready to start
seriously working on the C++ parts.  At that point I can be less
hypothetical.

> Calling them f()::x might be interesting, but there are some caveats.

The only reason I thought of this was:

drow@caradoc:~% cat a.c
int f()
{
  static int x;
  return x;
}
drow@caradoc:~% nm a.o | c++filt
0000000000000000 T f()
0000000000000000 b f()::x
                 U __gxx_personality_v0

i.e. that's what the physname would be if we demangled the linkage
name.

Other than that, I'm not attached to it.

> opsy. nm pr | grep themagicstatic
> 080497de V _ZZN1K1mEvE14themagicstatic
> 080497dd V _ZZN1K1mEvE14themagicstatic_0

For added amusement pipe that to c++filt.  They demangle the same.
Doesn't that seem like a bug?

-- 
Daniel Jacobowitz
CodeSourcery

Re: FYI: fix PR 9708

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <dan@codesourcery.com> writes:

Tom> Yes, I think we should also omit DW_TAG_subprogram here.  I just didn't
Tom> think of it.  I can either just check it in, or if you have an easy way
Tom> to test it, let me know.  I've appended the patch.

Daniel> The patch looks good to me; would you mind committing it?

I'll commit it shortly, here is the ChangeLog entry:

2010-03-15  Tom Tromey  <tromey@redhat.com>

	* dwarf2read.c (die_needs_namespace): Also return 0 for
	DW_TAG_subprogram.


Tom> opsy. nm pr | grep themagicstatic
Tom> 080497de V _ZZN1K1mEvE14themagicstatic
Tom> 080497dd V _ZZN1K1mEvE14themagicstatic_0

Daniel> For added amusement pipe that to c++filt.  They demangle the same.
Daniel> Doesn't that seem like a bug?

Yeah, though printing the _0 form would also be weird.
Strange situation.

Tom