Re: [RFA] Fix PR gdb/11702, printing of static const member variables

[Doug Evans <dje@google.com>]

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

On Mon, Jun 28, 2010 at 4:02 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Sun, 27 Jun 2010 20:24:42 +0200, Doug Evans wrote:
>> btw, the dwarf4 standard, as I read it, says static member variables are
>> identified by having DW_AT_external.  [4.1 Data Object Entries]
>> However, dwarf2_add_field is calling die_is_declaration.
>
> echo 'class C { static const float i = 1; } c;'|g++ -c -o 1.o -Wall -g -x c++ -
>    < c>   DW_AT_producer    : (indirect string, offset: 0x11): GNU C++ 4.6.0 20100628 (experimental)
>  <2><33>: Abbrev Number: 3 (DW_TAG_member)
>    <34>   DW_AT_name        : i
>    <38>   DW_AT_type        : <0x45>
>    <3c>   DW_AT_external    : 1
>    <3d>   DW_AT_accessibility: 3       (private)
>    <3e>   DW_AT_declaration : 1
>    <3f>   DW_AT_const_value : 4 byte block: 0 0 80 3f
>
> Isn't it primarily a bug in GCC?  There is no other DIE for `i' and it is
> a complete definition so there is no place for DW_AT_declaration there.
>
> Just such GCC change will be incompatible with existing GDBs, maybe to make
> the GCC change only for -gdwarf-4 upwards which is incompatible with older
> GDBs anyway?

I guess one can think of it as two separate issues: What does gcc emit
and what does gdb look for.
I was *only* addressing the latter.
You are correct that if gcc stopped emitting DW_AT_declaration
(assuming that's the correct thing to do), then enabling it via some
flag in order to not break debugging with older debuggers would be
reasonable (although if the context is just static const member
variables - they're completely broken in gdb 7.1 anyway, and perhaps
all the older gdb's that we care about).
It may be too late to add such a change to -gdwarf-4, I'm not sure.

[And, for completeness sake, there's the issue of needing to consider
what other compilers gdb is used with.]

>> This seems wrong, but I didn't address this here, other than to add
>> a note pointing this out (because some new code in new_symbol should match).
>
> I agree it is an issue for a different patch.
>
>
>> Also, I added a case to new_symbol to handle DW_TAG_member.
>> gcc uses DW_TAG_variable but the correct way AIUI is DW_TAG_member,
>
> 4.4.5 20100627 uses DW_TAG_variable but 4.5.1 20100627 uses DW_TAG_member.
>
>
> Otherwise - for the second patch - there are some needless whitespace changes.
> Also I would test also `float' const field there using LOC_CONST_BYTES; it
> works but just to test it, based on your IRC comments about LOC_CONST_BYTES.

I have committed the following.
I'll update the pr shortly.

2010-06-29  Doug Evans  <dje@google.com>

        PR c++/11702
        * NEWS: Add entry.
        * dwarf2read.c (dwarf2_add_field): If DW_AT_const_value is present,
        create a symbol for the field and record the value.
        (new_symbol): Handle DW_TAG_member.
        * gdbtypes.c (field_is_static): Remove FIXME.
        * symtab.c (search_symbols): When searching for VARIABLES_DOMAIN,
        only ignore LOC_CONST symbols that are enums.

        testsuite/
        Test PR c++/11702.
        * gdb.cp/m-static.exp: Add testcase.
        * gdb.cp/m-static.h (gnu_obj_4): Add initialized static const member.

[-- Attachment #2: gdb-100629-static-const-member-3.patch.txt --]
[-- Type: text/plain, Size: 7896 bytes --]

2010-06-29  Doug Evans  <dje@google.com>

	PR c++/11702
	* NEWS: Add entry.
	* dwarf2read.c (dwarf2_add_field): If DW_AT_const_value is present,
	create a symbol for the field and record the value.
	(new_symbol): Handle DW_TAG_member.
	* gdbtypes.c (field_is_static): Remove FIXME.
	* symtab.c (search_symbols): When searching for VARIABLES_DOMAIN,
	only ignore LOC_CONST symbols that are enums.

	testsuite/
	Test PR c++/11702.
	* gdb.cp/m-static.exp: Add testcase.
	* gdb.cp/m-static.h (gnu_obj_4): Add initialized static const member.

Index: NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.388
diff -u -p -r1.388 NEWS
--- NEWS	28 Jun 2010 21:16:02 -0000	1.388
+++ NEWS	29 Jun 2010 16:45:56 -0000
@@ -32,6 +32,11 @@
   GDB now also supports proper overload resolution for all the previously
   mentioned flavors of operators.
 
+  ** static const class members
+
+  Printing of static const class members that are initialized in the
+  class definition has been fixed.
+
 * Windows Thread Information Block access.
 
   On Windows targets, GDB now supports displaying the Windows Thread
Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.407
diff -u -p -r1.407 dwarf2read.c
--- dwarf2read.c	29 Jun 2010 16:35:28 -0000	1.407
+++ dwarf2read.c	29 Jun 2010 16:45:56 -0000
@@ -4528,6 +4528,11 @@ dwarf2_add_field (struct field_info *fip
 
   fp = &new_field->field;
 
+  /* NOTE: According to the dwarf standard, static data members are
+     indicated by having DW_AT_external.
+     The check here for ! die_is_declaration is historical.
+     This test is replicated in new_symbol.  */
+
   if (die->tag == DW_TAG_member && ! die_is_declaration (die, cu))
     {
       /* Data member other than a C++ static data member.  */
@@ -4643,6 +4648,14 @@ dwarf2_add_field (struct field_info *fip
       if (fieldname == NULL)
 	return;
 
+      attr = dwarf2_attr (die, DW_AT_const_value, cu);
+      if (attr)
+	{
+	  /* A static const member, not much different than an enum as far as
+	     we're concerned, except that we can support more types.  */
+	  new_symbol (die, NULL, cu);
+	}
+
       /* Get physical name.  */
       physname = (char *) dwarf2_physname (fieldname, die, cu);
 
@@ -8824,6 +8837,7 @@ new_symbol (struct die_info *die, struct
 	     BLOCK_FUNCTION from the blockvector.  */
 	  break;
 	case DW_TAG_variable:
+	case DW_TAG_member:
 	  /* Compilation with minimal debug info may result in variables
 	     with missing type entries. Change the misleading `void' type
 	     to something sensible.  */
@@ -8832,6 +8846,17 @@ new_symbol (struct die_info *die, struct
 	      = objfile_type (objfile)->nodebug_data_symbol;
 
 	  attr = dwarf2_attr (die, DW_AT_const_value, cu);
+	  /* In the case of DW_TAG_member, we should only be called for
+	     static const members.  */
+	  if (die->tag == DW_TAG_member)
+	    {
+	      /* NOTE: This test seems wrong according to the dwarf standard.
+		 static data members are represented by DW_AT_external.
+		 However, dwarf2_add_field is currently calling
+		 die_is_declaration to check, so we do the same.  */
+	      gdb_assert (die_is_declaration (die, cu));
+	      gdb_assert (attr);
+	    }
 	  if (attr)
 	    {
 	      dwarf2_const_value (attr, sym, cu);
Index: gdbtypes.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbtypes.c,v
retrieving revision 1.193
diff -u -p -r1.193 gdbtypes.c
--- gdbtypes.c	28 Jun 2010 20:12:52 -0000	1.193
+++ gdbtypes.c	29 Jun 2010 16:45:56 -0000
@@ -2512,9 +2512,7 @@ field_is_static (struct field *f)
      to the address of the enclosing struct.  It would be nice to
      have a dedicated flag that would be set for static fields when
      the type is being created.  But in practice, checking the field
-     loc_kind should give us an accurate answer (at least as long as
-     we assume that DWARF block locations are not going to be used
-     for static fields).  FIXME?  */
+     loc_kind should give us an accurate answer.  */
   return (FIELD_LOC_KIND (*f) == FIELD_LOC_KIND_PHYSNAME
 	  || FIELD_LOC_KIND (*f) == FIELD_LOC_KIND_PHYSADDR);
 }
Index: symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.239
diff -u -p -r1.239 symtab.c
--- symtab.c	28 Jun 2010 20:35:52 -0000	1.239
+++ symtab.c	29 Jun 2010 16:45:56 -0000
@@ -3057,10 +3057,15 @@ search_symbols (char *regexp, domain_enu
 	      if (file_matches (real_symtab->filename, files, nfiles)
 		  && ((regexp == NULL
 		       || re_exec (SYMBOL_NATURAL_NAME (sym)) != 0)
-		      && ((kind == VARIABLES_DOMAIN && SYMBOL_CLASS (sym) != LOC_TYPEDEF
+		      && ((kind == VARIABLES_DOMAIN
+			   && SYMBOL_CLASS (sym) != LOC_TYPEDEF
 			   && SYMBOL_CLASS (sym) != LOC_UNRESOLVED
 			   && SYMBOL_CLASS (sym) != LOC_BLOCK
-			   && SYMBOL_CLASS (sym) != LOC_CONST)
+			   /* LOC_CONST can be used for more than just enums,
+			      e.g., c++ static const members.
+			      We only want to skip enums here.  */
+			   && !(SYMBOL_CLASS (sym) == LOC_CONST
+				&& TYPE_CODE (SYMBOL_TYPE (sym)) == TYPE_CODE_ENUM))
 			  || (kind == FUNCTIONS_DOMAIN && SYMBOL_CLASS (sym) == LOC_BLOCK)
 			  || (kind == TYPES_DOMAIN && SYMBOL_CLASS (sym) == LOC_TYPEDEF))))
 		{
Index: testsuite/gdb.cp/m-static.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/m-static.exp,v
retrieving revision 1.12
diff -u -p -r1.12 m-static.exp
--- testsuite/gdb.cp/m-static.exp	27 Jun 2010 17:19:54 -0000	1.12
+++ testsuite/gdb.cp/m-static.exp	29 Jun 2010 16:45:56 -0000
@@ -56,12 +56,14 @@ gdb_start
 gdb_reinitialize_dir $srcdir/$subdir
 gdb_load ${binfile}
 
-
 if ![runto_main] then {
     perror "couldn't run to breakpoint"
     continue
 }
 
+get_debug_format
+set non_dwarf [expr ! [test_debug_format "DWARF 2"]]
+
 # First, run to after we've constructed all the objects:
 
 gdb_breakpoint [gdb_get_line_number "constructs-done"]
@@ -125,6 +127,16 @@ gdb_test "print test4.elsewhere" "\\$\[0
 # static const int that nobody initializes.  From PR gdb/635.
 gdb_test "print test4.nowhere" "field nowhere is nonexistent or has been optimized out" "static const int initialized nowhere"
 
+# static const initialized in the class definition, PR gdb/11702.
+if { $non_dwarf } { setup_xfail *-*-* }
+gdb_test "print test4.everywhere" "\\$\[0-9\].* = 317" "static const int initialized in class definition"
+if { $non_dwarf } { setup_xfail *-*-* }
+gdb_test "print test4.somewhere" "\\$\[0-9\].* = 3.14\[0-9\]*" "static const float initialized in class definition"
+
+# Also make sure static const members can be found via "info var".
+if { $non_dwarf } { setup_xfail *-*-* }
+gdb_test "info variable everywhere" "File .*/m-static\[.\]h.*const int gnu_obj_4::everywhere;" "info variable everywhere"
+
 # Perhaps at some point test4 should also include a test for a static
 # const int that was initialized in the header file.  But I'm not sure
 # that GDB's current behavior in such situations is either consistent
Index: testsuite/gdb.cp/m-static.h
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/m-static.h,v
retrieving revision 1.2
diff -u -p -r1.2 m-static.h
--- testsuite/gdb.cp/m-static.h	5 May 2006 18:04:09 -0000	1.2
+++ testsuite/gdb.cp/m-static.h	29 Jun 2010 16:45:56 -0000
@@ -5,8 +5,8 @@ class gnu_obj_4
  public:
   static const int elsewhere;
   static const int nowhere;
-  // At some point, perhaps:
-  // static const int everywhere = 317;
+  static const int everywhere = 317;
+  static const float somewhere = 3.14159;
 
   // try to ensure test4 is actually allocated
   int dummy;