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

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

[Doug Evans]

Hi.

This patch fixes http://sourceware.org/bugzilla/show_bug.cgi?id=11702

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.
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).

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,
so I added a case to new_symbol to handle it.
I'm not sure I like putting it with DW_TAG_variable,
but I assumed y'all would prefer the absence of code duplication.
If you want me to make it a separate case, let me know.
[Another way to go would be to try to use some code from the
DW_TAG_enumerator case - but since dwarf2_add_field would be the
caller for DW_TAG_variable and DW_TAG_member, I went with the current patch.]

There is one outstanding issue.
"info var static_const_member" should work, I *think*.
Is that true?
If so, then more work needs to be done (or even a different approach,
but the current patch seems reasonable).

symtab.c:search_symbols has this:

	      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
			   && SYMBOL_CLASS (sym) != LOC_UNRESOLVED
			   && SYMBOL_CLASS (sym) != LOC_BLOCK
>>>>			   && SYMBOL_CLASS (sym) != LOC_CONST)
			  || (kind == FUNCTIONS_DOMAIN && SYMBOL_CLASS (sym) == LOC_BLOCK)
			  || (kind == TYPES_DOMAIN && SYMBOL_CLASS (sym) == LOC_TYPEDEF))))


This prevents static const members from being found using the current
implementation.  Why do we care about LOC_CONST here?
[And, if we do, this also needs to check LOC_CONST_BYTES ...]

Comments?
Ok to check in?

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

	PR gdb/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.

	testsuite/
	* 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.386
diff -u -p -r1.386 NEWS
--- NEWS	25 Jun 2010 18:19:31 -0000	1.386
+++ NEWS	27 Jun 2010 17:09:16 -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.402
diff -u -p -r1.402 dwarf2read.c
--- dwarf2read.c	21 Jun 2010 19:49:19 -0000	1.402
+++ dwarf2read.c	27 Jun 2010 17:35:20 -0000
@@ -4518,6 +4518,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.  */
@@ -4625,7 +4630,7 @@ dwarf2_add_field (struct field_info *fip
 	 is a declaration, but all versions of G++ as of this writing
 	 (so through at least 3.2.1) incorrectly generate
 	 DW_TAG_variable tags.  */
-      
+
       char *physname;
 
       /* Get name of field.  */
@@ -4633,6 +4638,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);
 
@@ -8753,6 +8766,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.  */
@@ -8761,6 +8775,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.192
diff -u -p -r1.192 gdbtypes.c
--- gdbtypes.c	14 May 2010 20:17:37 -0000	1.192
+++ gdbtypes.c	27 Jun 2010 17:09:16 -0000
@@ -2511,9 +2511,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: 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	27 Jun 2010 17:20:29 -0000
@@ -125,6 +125,9 @@ 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 int initialized in the class definition, PR gdb/11702.
+gdb_test "print test4.everywhere" "\\$\[0-9\].* = 317" "static const int initialized in class definition"
+
 # 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	27 Jun 2010 17:20:29 -0000
@@ -5,8 +5,7 @@ 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;
 
   // try to ensure test4 is actually allocated
   int dummy;

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

[Doug Evans]

On Sun, Jun 27, 2010 at 11:24 AM, Doug Evans <dje@google.com> wrote:
> Hi.
>
> This patch fixes http://sourceware.org/bugzilla/show_bug.cgi?id=11702
>
> [...]
>
> There is one outstanding issue.
> "info var static_const_member" should work, I *think*.
> Is that true?
> If so, then more work needs to be done (or even a different approach,
> but the current patch seems reasonable).
>
> symtab.c:search_symbols has this:
>
>              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
>                           && SYMBOL_CLASS (sym) != LOC_UNRESOLVED
>                           && SYMBOL_CLASS (sym) != LOC_BLOCK
>>>>>                       && SYMBOL_CLASS (sym) != LOC_CONST)
>                          || (kind == FUNCTIONS_DOMAIN && SYMBOL_CLASS (sym) == LOC_BLOCK)
>                          || (kind == TYPES_DOMAIN && SYMBOL_CLASS (sym) == LOC_TYPEDEF))))
>
>
> This prevents static const members from being found using the current
> implementation.  Why do we care about LOC_CONST here?
> [And, if we do, this also needs to check LOC_CONST_BYTES ...]

Sigh.
The LOC_CONST here is used to catch enums.
Maybe another symbol table cleanup is to not put enum values in
VARIABLES_DOMAIN.

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

[Doug Evans]

On Sun, Jun 27, 2010 at 11:35 AM, Doug Evans <dje@google.com> wrote:
> Maybe another symbol table cleanup is to not put enum values in
> VARIABLES_DOMAIN.

Or rather, not rely on LOC_CONST to identify enums?
[VARIABLES_DOMAIN is just to identify a search request]

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

[Doug Evans]

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

Here's a revised patch that handles "info var static_const_member".

I noticed included.exp has code to check for the dwarf format so I
copied that here.
I don't know whether other formats support static const member variables or not.

No regressions on amd64-linux.

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

        PR gdb/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/
        * gdb.cp/m-static.exp: Add testcase.
        * gdb.cp/m-static.h (gnu_obj_4): Add initialized static const member.

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

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

	PR gdb/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/
	* 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.386
diff -u -p -r1.386 NEWS
--- NEWS	25 Jun 2010 18:19:31 -0000	1.386
+++ NEWS	27 Jun 2010 22:58:55 -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.402
diff -u -p -r1.402 dwarf2read.c
--- dwarf2read.c	21 Jun 2010 19:49:19 -0000	1.402
+++ dwarf2read.c	27 Jun 2010 22:58:55 -0000
@@ -4518,6 +4518,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.  */
@@ -4625,7 +4630,7 @@ dwarf2_add_field (struct field_info *fip
 	 is a declaration, but all versions of G++ as of this writing
 	 (so through at least 3.2.1) incorrectly generate
 	 DW_TAG_variable tags.  */
-      
+
       char *physname;
 
       /* Get name of field.  */
@@ -4633,6 +4638,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);
 
@@ -8753,6 +8766,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.  */
@@ -8761,6 +8775,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.192
diff -u -p -r1.192 gdbtypes.c
--- gdbtypes.c	14 May 2010 20:17:37 -0000	1.192
+++ gdbtypes.c	27 Jun 2010 22:58:55 -0000
@@ -2511,9 +2511,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.238
diff -u -p -r1.238 symtab.c
--- symtab.c	2 Jun 2010 22:41:55 -0000	1.238
+++ symtab.c	27 Jun 2010 22:58:55 -0000
@@ -3047,10 +3047,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	27 Jun 2010 22:58:55 -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,14 @@ 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 int 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"
+
+# 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	27 Jun 2010 22:58:55 -0000
@@ -5,8 +5,7 @@ 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;
 
   // try to ensure test4 is actually allocated
   int dummy;

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

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> On Sun, Jun 27, 2010 at 11:35 AM, Doug Evans <dje@google.com> wrote:
>> Maybe another symbol table cleanup is to not put enum values in
>> VARIABLES_DOMAIN.

Doug> Or rather, not rely on LOC_CONST to identify enums?
Doug> [VARIABLES_DOMAIN is just to identify a search request]

I see that it is documented, but I still wonder why search_symbols
excludes enum constants.

Tom

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

[Jan Kratochvil]

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?


> 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.


Thanks,
Jan

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

[Doug Evans]

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'm not sure I follow.
If all the GCC versions we care about are also adding DW_AT_external
to the DIE, gdb *could* check for it instead of die_is_declaration,
right?  Or am I missing something?

> 4.4.5 20100627 uses DW_TAG_variable but 4.5.1 20100627 uses DW_TAG_member.

Ah.

> 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'll add a few more testcases.  Thanks.
[And I'll check the whitespace fix in separately.]

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

[Jan Kratochvil]

On Mon, 28 Jun 2010 20:26:12 +0200, Doug Evans wrote:
> 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'm not sure I follow.
> If all the GCC versions we care about are also adding DW_AT_external
> to the DIE, gdb *could* check for it instead of die_is_declaration,
> right?  Or am I missing something?

Your referenced text

# DWARF-4: 4.1 Data Object Entries
# The definitions of C++ static data members of structures or classes are
# represented by variable entries flagged as external. Both file static and
# local variables in C and C++ are represented by non-external variable entries.

IMO more describes when `external' is present.  It does not describe that
member is `static' would be determined by presence of the `external' attribute.


------------------------------------------------------------------------------
There are several problems to solve:

(a) How should the DWARF look from a blue sky compiler written from scratch,
    according to the latest DWARF standard.

(b) How should GDB determine it is a `static' field.

(c) How should be (b) made backward and forward compatible with current GCC
    versions.

=> means "specifies how should be"
(b) must not => (a)
(c) must not => (a)
(a) => (b)
(b) => (c)
------------------------------------------------------------------------------


So for the (a) point: I believe GCC should not produce DW_AT_declaration when
the same DIE has DW_AT_const_value.
(Also checked GCC source that it really does not emit it intentionally and it
may be more a bug.)  That's all.

For the (b) point - GDB should check missing DW_AT_data_member_location.
(Just guessed - is it OK?  I have not tried to implement it.)

(c) I guess the (b) rule works even with current GCC output, no special
compatibility cludge is needed.


Thanks,
Jan

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

[Jan Kratochvil]

On Mon, 28 Jun 2010 21:09:59 +0200, Jan Kratochvil wrote:
> (c) I guess the (b) rule works even with current GCC output, no special
> compatibility cludge is needed.

for GDB; GCC should stop emitting DW_AT_declaration for better DWARF
correctness (with no other effect) only for DWARF-4+ as it would break
older=current GDBs otherwise.

No patch or testsuite run tried so far.


Regards,
Jan

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

[Doug Evans]

[-- 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;