[RFA] Nullified garbage-collected global variables

[RFA] Nullified garbage-collected global variables

[Jerome Guitton]

The unused data/code elimination done by LD leaves some null partial
symbols; unfortunately, LD is not smart enough to drop this unused
dwarf debug info. So it's up to GDB to discard them.

For example, consider p.c:

/******************************/

 int my_global_symbol = 42;

 static int my_static_symbol;

 int
 main ()
 {
   return 1;
 }

/******************************/

...and q.c:

/******************************/

 static int my_static_symbol;

 void
 r ()
 {
   my_static_symbol = my_global_symbol;
   my_global_symbol = my_static_symbol + my_global_symbol;
 }

/******************************/

...built with -fdata-sections/-ffunction-sections and linked with -gc-sections:

 gcc -c -g -ffunction-sections -fdata-sections p.c
 gcc -Wl,-gc-sections -Wl,-e,main p.o -o p

 gcc -c -g -ffunction-sections -fdata-sections r.c
 gcc -Wl,-gc-sections -Wl,-e,r r.o -o r

In the final executable p, my_global_symbol will be removed, as it is
not used. But the linker cannot remove the corresponding debug info;
it will just nullify my_global_symbol's address in its location
expression.

If we load both p and r in GDB (e.g. using add-symbol-file) the value
of my_global_symbol's address will depend on load order:
* p before r => &my_global_symbol == 0
* r before p => &my_global_symbol not null

The second scenario is the correct one; my_global_symbol is taken from
r. You would expect the same behavior in the case of the first
scenario...

This example may seem a bit artificial, but if you consider two
modules that are linked against the same library, then stripped using
-gc-sections, then both loaded on the same target, you will understand
that the user may have some troubles debugging the variables in his
library, depending on what has been dragged into which module and
what has been dropped, and depending on the load order.

OK to apply? If so, I'll provide a new testcase.

gdb/ChangeLog:
	* dwarf2read.c (add_partial_symbol): Do not add a global variable if
	its adress is null. Add comment to explain why.
---
 gdb/dwarf2read.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index e4ab034..da85aa2 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -2447,7 +2447,10 @@ add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
 	     Don't enter into the minimal symbol tables as there is
 	     a minimal symbol table entry from the ELF symbols already.
 	     Enter into partial symbol table if it has a location
-	     descriptor or a type.
+	     descriptor or a type. A global variable may also have been
+	     stripped out by the linker if unused, in which case its
+	     address will be nullified; do not add such variables into
+	     partial symbol table then.
 	     If the location descriptor is missing, new_symbol will create
 	     a LOC_UNRESOLVED symbol, the address of the variable will then
 	     be determined from the minimal symbol table whenever the variable
@@ -2458,7 +2461,7 @@ add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
 
 	  if (pdi->locdesc)
 	    addr = decode_locdesc (pdi->locdesc, cu);
-	  if (pdi->locdesc || pdi->has_type)
+	  if ((pdi->locdesc && addr) || (!pdi->locdesc && pdi->has_type))
 	    psym = add_psymbol_to_list (actual_name, strlen (actual_name),
 					built_actual_name,
 					VAR_DOMAIN, LOC_STATIC,
-- 
1.6.5.rc2

Re: [RFA] Nullified garbage-collected global variables

[Tom Tromey]

>>>>> "Jerome" == Jerome Guitton <guitton@adacore.com> writes:

Jerome> 	* dwarf2read.c (add_partial_symbol): Do not add a global variable if
Jerome> 	its adress is null. Add comment to explain why.

I am not totally sure about this.  The patch itself looks reasonable,
but I have a couple of questions.

Do you need to apply the same treatment to full symbols?  What happens
if the psymtab is expanded for some other reason and a full symbol of
this sort is then created?  Or is that already impossible?

What about has_section_at_zero?

Tom

Re: [RFA] Nullified garbage-collected global variables

[Jerome Guitton]

Tom Tromey (tromey@redhat.com):

> Do you need to apply the same treatment to full symbols?  What happens
> if the psymtab is expanded for some other reason and a full symbol of
> this sort is then created?  Or is that already impossible?

Well, that sounded unlikely; then I double-checked, and experimental
results rejected my assumption. e.g. my example without --readnow:

(gdb) p my_global_symbol
No symbol "my_global_symbol" in current context.

and with --readnow:

(gdb) p my_global_symbol 
Cannot access memory at address 0x0

Thank you for catching that. I'll fix my patch.


> What about has_section_at_zero?

Right, it's worth protecting us against this case as well (as we do for
elminated functions). I'll add a guard.

Thank you for the review,
Jerome

Re: [RFA] Nullified garbage-collected global variables

[Jerome Guitton]

Tom Tromey (tromey@redhat.com):

> >>>>> "Jerome" == Jerome Guitton <guitton@adacore.com> writes:
> 
> Jerome> 	* dwarf2read.c (add_partial_symbol): Do not add a global variable if
> Jerome> 	its adress is null. Add comment to explain why.
> 
> I am not totally sure about this.  The patch itself looks reasonable,
> but I have a couple of questions.
> 
> Do you need to apply the same treatment to full symbols?  What happens
> if the psymtab is expanded for some other reason and a full symbol of
> this sort is then created?  Or is that already impossible?
> 
> What about has_section_at_zero?

Here is a new patch that should fix these issues. I have tested it on
x86-linux, no regression.

OK to apply?

gdb/ChangeLog:

	* dwarf2read.c (add_partial_symbol): Do not add a global variable if
	its adress is null. Add comment to explain why.
	(new_symbol): Ditto.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 288d777..15926c6 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -3427,7 +3427,10 @@ add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
 	     Don't enter into the minimal symbol tables as there is
 	     a minimal symbol table entry from the ELF symbols already.
 	     Enter into partial symbol table if it has a location
-	     descriptor or a type.
+	     descriptor or a type.  A global variable may also have been
+	     stripped out by the linker if unused, in which case its
+	     address will be nullified; do not add such variables into
+	     partial symbol table then.
 	     If the location descriptor is missing, new_symbol will create
 	     a LOC_UNRESOLVED symbol, the address of the variable will then
 	     be determined from the minimal symbol table whenever the variable
@@ -3438,7 +3441,9 @@ add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
 
 	  if (pdi->locdesc)
 	    addr = decode_locdesc (pdi->locdesc, cu);
-	  if (pdi->locdesc || pdi->has_type)
+	  if ((pdi->locdesc
+	       && (addr || dwarf2_per_objfile->has_section_at_zero))
+	      || (!pdi->locdesc && pdi->has_type))
 	    psym = add_psymbol_to_list (actual_name, strlen (actual_name),
 					built_actual_name,
 					VAR_DOMAIN, LOC_STATIC,
@@ -9860,7 +9865,16 @@ 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))
+	      if (SYMBOL_CLASS (sym) == LOC_STATIC
+		  && SYMBOL_VALUE_ADDRESS (sym) == 0
+		  && !dwarf2_per_objfile->has_section_at_zero)
+		{
+		  /* When a static variable is eliminated by the linker,
+		     the corresponding debug information is not stripped
+		     out, but the variable address is set to null;
+		     do not add such variables into symbol table.  */
+		}
+	      else if (attr2 && (DW_UNSND (attr2) != 0))
 		{
 		  struct pending **list_to_add;
 

Re: [RFA] Nullified garbage-collected global variables

[Jerome Guitton]

Jerome Guitton (guitton@adacore.com):

> Well, that sounded unlikely; then I double-checked, and experimental
> results rejected my assumption. e.g. my example without --readnow:
> 
> (gdb) p my_global_symbol
> No symbol "my_global_symbol" in current context.
> 
> and with --readnow:
> 
> (gdb) p my_global_symbol 
> Cannot access memory at address 0x0
> 
> Thank you for catching that. I'll fix my patch.

...and I will add this case in a testcase if/when the patch is accepted.

Re: [RFA] Nullified garbage-collected global variables

[Tom Tromey]

>>>>> "Jerome" == Jerome Guitton <guitton@adacore.com> writes:

Jerome> 	* dwarf2read.c (add_partial_symbol): Do not add a global variable if
Jerome> 	its adress is null. Add comment to explain why.
Jerome> 	(new_symbol): Ditto.

This is ok.  Thanks.

Tom

Re: [RFA] Nullified garbage-collected global variables

[Jerome Guitton]

Tom Tromey (tromey@redhat.com):

> >>>>> "Jerome" == Jerome Guitton <guitton@adacore.com> writes:
> 
> Jerome> 	* dwarf2read.c (add_partial_symbol): Do not add a global variable if
> Jerome> 	its adress is null. Add comment to explain why.
> Jerome> 	(new_symbol): Ditto.
> 
> This is ok.  Thanks.

Thank you. I was actually polishing my tests when I realized that I was
missing a case (partial symbols for static variables). I've modified
my patch to handle it; it actually makes it cleaner, I think.

Unless you object, I will commit this new version.


gdb/ChangeLog:

	* dwarf2read.c (add_partial_symbol): Do not add a global variable if
	its adress is null. Add comment to explain why.
	(new_symbol): Ditto.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 288d777..80222c3 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -3421,7 +3421,19 @@ add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
 	}
       break;
     case DW_TAG_variable:
-      if (pdi->is_external)
+      if (pdi->locdesc)
+	addr = decode_locdesc (pdi->locdesc, cu);
+
+      if (pdi->locdesc
+	  && addr == 0
+	  && !dwarf2_per_objfile->has_section_at_zero)
+	{
+	  /* A global or static variable may also have been stripped
+	     out by the linker if unused, in which case its address
+	     will be nullified; do not add such variables into partial
+	     symbol table then.  */
+	}
+      else if (pdi->is_external)
 	{
 	  /* Global Variable.
 	     Don't enter into the minimal symbol tables as there is
@@ -3436,8 +3448,6 @@ add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
 	     used by GDB, but it comes in handy for debugging partial symbol
 	     table building.  */
 
-	  if (pdi->locdesc)
-	    addr = decode_locdesc (pdi->locdesc, cu);
 	  if (pdi->locdesc || pdi->has_type)
 	    psym = add_psymbol_to_list (actual_name, strlen (actual_name),
 					built_actual_name,
@@ -3455,7 +3465,6 @@ add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
 		xfree (actual_name);
 	      return;
 	    }
-	  addr = decode_locdesc (pdi->locdesc, cu);
 	  /*prim_record_minimal_symbol (actual_name, addr + baseaddr,
 	     mst_file_data, objfile); */
 	  psym = add_psymbol_to_list (actual_name, strlen (actual_name),
@@ -9860,7 +9869,16 @@ 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))
+	      if (SYMBOL_CLASS (sym) == LOC_STATIC
+		  && SYMBOL_VALUE_ADDRESS (sym) == 0
+		  && !dwarf2_per_objfile->has_section_at_zero)
+		{
+		  /* When a static variable is eliminated by the linker,
+		     the corresponding debug information is not stripped
+		     out, but the variable address is set to null;
+		     do not add such variables into symbol table.  */
+		}
+	      else if (attr2 && (DW_UNSND (attr2) != 0))
 		{
 		  struct pending **list_to_add;
 

Re: [RFA] Nullified garbage-collected global variables

[Tom Tromey]

>>>>> "Jerome" == Jerome Guitton <guitton@adacore.com> writes:

Jerome> Unless you object, I will commit this new version.
Jerome> gdb/ChangeLog:

Jerome> 	* dwarf2read.c (add_partial_symbol): Do not add a global variable if
Jerome> 	its adress is null. Add comment to explain why.
Jerome> 	(new_symbol): Ditto.

Looks ok to me.

FWIW -- I prefer it if a fix and its corresponding test case are
submitted and committed as a unit.  I think it is more logical, and also
the test case sometimes helps the review of the patch.

Tom

Re: [RFA] Nullified garbage-collected global variables

[Jerome Guitton]

Tom Tromey (tromey@redhat.com):

> Jerome> 	* dwarf2read.c (add_partial_symbol): Do not add a global variable if
> Jerome> 	its adress is null. Add comment to explain why.
> Jerome> 	(new_symbol): Ditto.
> 
> Looks ok to me.

This has now been committed.


> FWIW -- I prefer it if a fix and its corresponding test case are
> submitted and committed as a unit.  I think it is more logical, and also
> the test case sometimes helps the review of the patch.

That makes perfect sense. I will do that in the future.

Thank you
Jerome