[patch] Fix dangling displays in separate debug

[patch] Fix dangling displays in separate debug

[Jan Kratochvil]

Hi,

gdb.base/solib-display.exp using _separate_ debug info:
3: c_global = gdbtypes.c:1369: internal-error: check_typedef: Assertion `type' failed.
A problem internal to GDB has been detected,

This problem was fixed before by:
	[patch 1/8] Types GC [unloading observer]
	http://sourceware.org/ml/gdb-patches/2009-05/msg00544.html
	Re: [patch 3/8] Types GC [display_uses_solib_p to exp_iterate]
	http://sourceware.org/ml/gdb-patches/2009-07/msg00054.html

but as that patchset is still not in providing this temporary fixup instead.

One may only address that gdb.base/solib-display.exp was testing symbol
in-objfile while now it tests only symbol in-sepdebug-objfile and no longer
the in-objfile case.  I find the in-sepdebug-objfile as a superset of
in-objfile test but I can rework it if anyones addresses this test change.

No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.


Thanks,
Jan


gdb/
2010-04-03  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* printcmd.c (display_uses_solib_p): Check also
	SEPARATE_DEBUG_OBJFILE.

gdb/testsuite/
2010-04-03  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/solib-display.exp (split solib): New.

--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1895,7 +1895,9 @@ display_uses_solib_p (const struct display *d,
 	    return 1;
 
 	  /* SYMBOL_OBJ_SECTION (symbol) may be NULL.  */
-	  if (SYMBOL_SYMTAB (symbol)->objfile == solib->objfile)
+	  if (SYMBOL_SYMTAB (symbol)->objfile == solib->objfile
+	      || SYMBOL_SYMTAB (symbol)->objfile
+		 == solib->objfile->separate_debug_objfile)
 	    return 1;
 	}
       endpos -= oplen;
--- a/gdb/testsuite/gdb.base/solib-display.exp
+++ b/gdb/testsuite/gdb.base/solib-display.exp
@@ -53,6 +53,13 @@ if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $lib_flags] != ""
   return -1
 }
 
+set test "split solib"
+if {[gdb_gnu_strip_debug $binfile_lib] != 0} {
+    fail $test
+} else {
+    pass $test
+}
+
 gdb_exit
 gdb_start
 gdb_reinitialize_dir $srcdir/$subdir

Re: [patch] Fix dangling displays in separate debug

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> 2010-04-03  Jan Kratochvil  <jan.kratochvil@redhat.com>
Jan> 	* printcmd.c (display_uses_solib_p): Check also
Jan> 	SEPARATE_DEBUG_OBJFILE.

Jan> -	  if (SYMBOL_SYMTAB (symbol)->objfile == solib->objfile)
Jan> +	  if (SYMBOL_SYMTAB (symbol)->objfile == solib->objfile
Jan> +	      || SYMBOL_SYMTAB (symbol)->objfile
Jan> +		 == solib->objfile->separate_debug_objfile)

An objfile can now have multiple separate debuginfo objfiles, linked
using separate_debug_objfile_link.

So, I think this test should actually be:

	  if (SYMBOL_SYMTAB (symbol)->objfile == solib->objfile
	      || SYMBOL_SYMTAB (symbol)->objfile->separate_debug_objfile_backlink
		 == solib->objfile)

What do you think?

Tom

Re: [patch] Fix dangling displays in separate debug

[Jan Kratochvil]

On Wed, 07 Apr 2010 21:24:54 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> Jan> -	  if (SYMBOL_SYMTAB (symbol)->objfile == solib->objfile)
> Jan> +	  if (SYMBOL_SYMTAB (symbol)->objfile == solib->objfile
> Jan> +	      || SYMBOL_SYMTAB (symbol)->objfile
> Jan> +		 == solib->objfile->separate_debug_objfile)
> 
> An objfile can now have multiple separate debuginfo objfiles, linked
> using separate_debug_objfile_link.

Yes, I agree, forgot...


> So, I think this test should actually be:
> 
> 	  if (SYMBOL_SYMTAB (symbol)->objfile == solib->objfile
> 	      || SYMBOL_SYMTAB (symbol)->objfile->separate_debug_objfile_backlink
> 		 == solib->objfile)
> 
> What do you think?

Unaware how to improve it more.

No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.

OK to check-in?


Thanks,
Jan


gdb/
2010-04-03  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Tom Tromey  <tromey@redhat.com>

	* printcmd.c (display_uses_solib_p): Check also
	SEPARATE_DEBUG_OBJFILE_BACKLINK.  New variable symbol_objfile.

gdb/testsuite/
2010-04-03  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/solib-display.exp (split solib): New.

--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1888,6 +1888,7 @@ display_uses_solib_p (const struct display *d,
 	{
 	  const struct block *const block = elts[i + 1].block;
 	  const struct symbol *const symbol = elts[i + 2].symbol;
+	  struct objfile *symbol_objfile;
 
 	  if (block != NULL
 	      && solib_contains_address_p (solib,
@@ -1895,7 +1896,10 @@ display_uses_solib_p (const struct display *d,
 	    return 1;
 
 	  /* SYMBOL_OBJ_SECTION (symbol) may be NULL.  */
-	  if (SYMBOL_SYMTAB (symbol)->objfile == solib->objfile)
+	  symbol_objfile = SYMBOL_SYMTAB (symbol)->objfile;
+	  if (symbol_objfile == solib->objfile
+	      || symbol_objfile->separate_debug_objfile_backlink
+		 == solib->objfile)
 	    return 1;
 	}
       endpos -= oplen;
--- a/gdb/testsuite/gdb.base/solib-display.exp
+++ b/gdb/testsuite/gdb.base/solib-display.exp
@@ -53,6 +53,13 @@ if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $lib_flags] != ""
   return -1
 }
 
+set test "split solib"
+if {[gdb_gnu_strip_debug $binfile_lib] != 0} {
+    fail $test
+} else {
+    pass $test
+}
+
 gdb_exit
 gdb_start
 gdb_reinitialize_dir $srcdir/$subdir

Re: [patch] Fix dangling displays in separate debug

[Pedro Alves]

On Friday 09 April 2010 16:30:14, Jan Kratochvil wrote:
> Unaware how to improve it more.

Would using objfile_separate_debug_iterate be better?

-- 
Pedro Alves

Re: [patch] Fix dangling displays in separate debug

[Jan Kratochvil]

On Fri, 09 Apr 2010 17:34:01 +0200, Pedro Alves wrote:
> On Friday 09 April 2010 16:30:14, Jan Kratochvil wrote:
> > Unaware how to improve it more.
> 
> Would using objfile_separate_debug_iterate be better?

In this case we are examining `struct expression *'.
EXP contains `struct symbol *'.
SYMBOL points to `struct objfile *' of the separate debug info file.

If SYMBOL points to `struct objfile *' of the main binary, it is equal to
SOLIB->OBJFILE and it has been already checked before.

The main binary -> separate debug info direction provided by the iterating
functionality of objfile_separate_debug_iterate is not useful in this case.


I cannot not say I like this design but its rework I have not completed to the
check-in before:
	[patch 1/8] Types GC [unloading observer]
	http://sourceware.org/ml/gdb-patches/2009-05/msg00544.html
	Re: [patch 3/8] Types GC [display_uses_solib_p to exp_iterate]
	http://sourceware.org/ml/gdb-patches/2009-07/msg00054.html

If continuing hacks of the current code is not acceptable it can wait till / I
can push the proper reimplementation of this code from the old thread above.



Thanks,
Jan

Re: [patch] Fix dangling displays in separate debug

[Pedro Alves]

On Friday 09 April 2010 16:52:49, Jan Kratochvil wrote:
> > Would using objfile_separate_debug_iterate be better?
> 
> In this case we are examining `struct expression *'.
> EXP contains `struct symbol *'.
> SYMBOL points to `struct objfile *' of the separate debug info file.
> 
> If SYMBOL points to `struct objfile *' of the main binary, it is equal to
> SOLIB->OBJFILE and it has been already checked before.
> 
> The main binary -> separate debug info direction provided by the iterating
> functionality of objfile_separate_debug_iterate is not useful in this case.

Thanks for explaining.  I should have read the patch properly.

Just one more question:

> -         if (SYMBOL_SYMTAB (symbol)->objfile == solib->objfile)
> +         symbol_objfile = SYMBOL_SYMTAB (symbol)->objfile;
> +         if (symbol_objfile == solib->objfile
> +             || symbol_objfile->separate_debug_objfile_backlink
> +                == solib->objfile)
>             return 1;

Can't both `symbol_objfile->separate_debug_objfile_backlink' (because
symbol_objfile is the main objfile already) and `solib->objfile'
(because GDB didn't find any symbols for the shared library) be NULL,
and hence this returns false positives?

-- 
Pedro Alves

Re: [patch] Fix dangling displays in separate debug

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Jan> Unaware how to improve it more.

Pedro> Would using objfile_separate_debug_iterate be better?

Actually, I was wondering if we now claim to support multiple levels of
separate debuginfo.  Based on how it is written,
objfile_separate_debug_iterate seems to indicate that we do, though
comments in objfiles.h continue to claim that we do not.

If we do then I agree, a simple test is insufficient.

Tom

Re: [patch] Fix dangling displays in separate debug

[Jan Kratochvil]

On Fri, 09 Apr 2010 18:47:50 +0200, Pedro Alves wrote:
> On Friday 09 April 2010 16:52:49, Jan Kratochvil wrote:
> > -         if (SYMBOL_SYMTAB (symbol)->objfile == solib->objfile)
> > +         symbol_objfile = SYMBOL_SYMTAB (symbol)->objfile;
> > +         if (symbol_objfile == solib->objfile
> > +             || symbol_objfile->separate_debug_objfile_backlink
> > +                == solib->objfile)
> >             return 1;
> 
> Can't both `symbol_objfile->separate_debug_objfile_backlink' (because
> symbol_objfile is the main objfile already)

Yes, it can be NULL.

> and `solib->objfile'
> (because GDB didn't find any symbols for the shared library) be NULL,

I was convinced due to some invalid reasons solib->objfile cannot be NULL.

> and hence this returns false positives?

Yes, there was a regression, thanks for catching it.


No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.


Thanks,
Jan


gdb/
2010-04-09  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Tom Tromey  <tromey@redhat.com>
	    Pedro Alves  <pedro@codesourcery.com>

	* printcmd.c (display_uses_solib_p): Check also
	SEPARATE_DEBUG_OBJFILE_BACKLINK.  New variable symbol_objfile.
	* solist.h (struct so_list) <objfile>: New comment.
	* symtab.h (struct general_symbol_info) <obj_section>: Extend the
	comment.

gdb/testsuite/
2010-04-09  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/solib-display.exp (split solib): New.

--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1888,6 +1888,7 @@ display_uses_solib_p (const struct display *d,
 	{
 	  const struct block *const block = elts[i + 1].block;
 	  const struct symbol *const symbol = elts[i + 2].symbol;
+	  struct objfile *symbol_objfile;
 
 	  if (block != NULL
 	      && solib_contains_address_p (solib,
@@ -1895,7 +1896,11 @@ display_uses_solib_p (const struct display *d,
 	    return 1;
 
 	  /* SYMBOL_OBJ_SECTION (symbol) may be NULL.  */
-	  if (SYMBOL_SYMTAB (symbol)->objfile == solib->objfile)
+	  symbol_objfile = SYMBOL_SYMTAB (symbol)->objfile;
+	  if (solib->objfile
+	      && (symbol_objfile == solib->objfile
+		  || symbol_objfile->separate_debug_objfile_backlink
+		     == solib->objfile))
 	    return 1;
 	}
       endpos -= oplen;
--- a/gdb/solist.h
+++ b/gdb/solist.h
@@ -64,7 +64,12 @@ struct so_list
     bfd *abfd;
     char symbols_loaded;	/* flag: symbols read in yet? */
     char from_tty;		/* flag: print msgs? */
-    struct objfile *objfile;	/* objfile for loaded lib */
+
+    /* objfile with symbols for a loaded library.  Target memory is read from
+       ABFD.  OBJFILE may be NULL either before symbols have been loaded or if
+       the file cannot be found.  */
+    struct objfile *objfile;
+
     struct target_section *sections;
     struct target_section *sections_end;
 
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -148,7 +148,7 @@ struct general_symbol_info
 
   short section;
 
-  /* The section associated with this symbol. */
+  /* The section associated with this symbol.  It can be NULL.  */
 
   struct obj_section *obj_section;
 };
--- a/gdb/testsuite/gdb.base/solib-display.exp
+++ b/gdb/testsuite/gdb.base/solib-display.exp
@@ -53,6 +53,13 @@ if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $lib_flags] != ""
   return -1
 }
 
+set test "split solib"
+if {[gdb_gnu_strip_debug $binfile_lib] != 0} {
+    fail $test
+} else {
+    pass $test
+}
+
 gdb_exit
 gdb_start
 gdb_reinitialize_dir $srcdir/$subdir

Re: [patch] Fix dangling displays in separate debug

[Pedro Alves]

On Friday 09 April 2010 21:00:46, Jan Kratochvil wrote:
> On Fri, 09 Apr 2010 18:47:50 +0200, Pedro Alves wrote:

> > and `solib->objfile'
> > (because GDB didn't find any symbols for the shared library) be NULL,
> 
> I was convinced due to some invalid reasons solib->objfile cannot be NULL.

There's an easy way to see that state:

 (gdb) nosharedlibrary
 (gdb) info sharedlibrary

This will not reload symbol info, thus you'll have
a bunch of solibs with objfile set to NULL.
[do `(gdb) sharedlibrary' to reload debug info].

> gdb/
> 2010-04-09  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 	    Tom Tromey  <tromey@redhat.com>
> 	    Pedro Alves  <pedro@codesourcery.com>
> 
> 	* printcmd.c (display_uses_solib_p): Check also
> 	SEPARATE_DEBUG_OBJFILE_BACKLINK.  New variable symbol_objfile.
> 	* solist.h (struct so_list) <objfile>: New comment.
> 	* symtab.h (struct general_symbol_info) <obj_section>: Extend the
> 	comment.

This part is OK.  Thanks for the new comments.

> 
> gdb/testsuite/
> 2010-04-09  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.base/solib-display.exp (split solib): New.

Doesn't this make it so that only the separate debug info
case is exercised, and, it stops exercising the non-separate
debug info case?  I think we should instead be running the
relevant parts the test twice instead?

> --- a/gdb/testsuite/gdb.base/solib-display.exp
> +++ b/gdb/testsuite/gdb.base/solib-display.exp
> @@ -53,6 +53,13 @@ if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $lib_flags] != ""
>    return -1
>  }
>  
> +set test "split solib"
> +if {[gdb_gnu_strip_debug $binfile_lib] != 0} {
> +    fail $test

Not all systems support this, so this should 
be `unsupported' instead.  When you make the tests
run twice, this would skip the rest of the
rerunning.

> +} else {
> +    pass $test
> +}

> +
>  gdb_exit
>  gdb_start
>  gdb_reinitialize_dir $srcdir/$subdir

-- 
Pedro Alves

Re: [patch] Fix dangling displays in separate debug

[Jan Kratochvil]

On Sun, 11 Apr 2010 03:27:35 +0200, Pedro Alves wrote:
> On Friday 09 April 2010 21:00:46, Jan Kratochvil wrote:
> > I was convinced due to some invalid reasons solib->objfile cannot be NULL.
> 
> There's an easy way to see that state:
> 
>  (gdb) nosharedlibrary
>  (gdb) info sharedlibrary

Nice reproducer, thanks.


> Doesn't this make it so that only the separate debug info
> case is exercised, and, it stops exercising the non-separate
> debug info case?  I think we should instead be running the
> relevant parts the test twice instead?

I was asking about it before.  Therefore reworked it as promised.

On Sat, 03 Apr 2010 11:55:58 +0200, Jan Kratochvil wrote:
# One may only address that gdb.base/solib-display.exp was testing symbol
# in-objfile while now it tests only symbol in-sepdebug-objfile and no longer
# the in-objfile case.  I find the in-sepdebug-objfile as a superset of
# in-objfile test but I can rework it if anyones addresses this test change.


> > +if {[gdb_gnu_strip_debug $binfile_lib] != 0} {
> > +    fail $test
> 
> Not all systems support this, so this should be `unsupported' instead.

You are right, found at least one such target - i386-msdos:
+ ./binutils/objcopy --add-gnu-debuglink=1.o-debug 1.o-stripped 1.o-dest
BFD: 1.o-dest: can not represent section `.gnu_debuglink' in a.out object file format
./binutils/objcopy:1.o-dest: cannot fill debug link section `1.o-debug': Nonrepresentable section on output


With the extended requirements I rather pushed the former patch posted at:
	Re: [patch 3/8] Types GC [display_uses_solib_p to exp_iterate]
	http://sourceware.org/ml/gdb-patches/2009-07/msg00054.html

There was forgotten handling of TYPE_INSTANCE, UNOP_DYNAMIC_CAST and
UNOP_REINTERPRET_CAST.  Reduced it only for the current applicable use case;
to be extended one day for the types garbage collector on its resubmit.


No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.


Thanks,
Jan


gdb/
2010-04-19  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix crashes on dangling display expressions.
	* ada-lang.c (ada_operator_check): New function.
	(ada_exp_descriptor): Fill-in the field operator_check.
	* c-lang.c (exp_descriptor_c): Fill-in the field operator_check.
	* jv-lang.c (exp_descriptor_java): Likewise.
	* m2-lang.c (exp_descriptor_modula2): Likewise.
	* scm-lang.c (exp_descriptor_scm): Likewise.
	* parse.c (exp_descriptor_standard): Likewise.
	(operator_check_standard): New function.
	(exp_iterate, exp_uses_objfile_iter, exp_uses_objfile): New functions.
	* parser-defs.h (struct exp_descriptor): New field operator_check.
	(operator_check_standard, exp_uses_objfile): New declarations.
	* printcmd.c: Remove the inclusion of solib.h.
	(display_uses_solib_p): Remove the function.
	(clear_dangling_display_expressions): Call lookup_objfile_from_block
	and exp_uses_objfile instead of display_uses_solib_p.
	* solist.h (struct so_list) <objfile>: New comment.
	* symtab.c (lookup_objfile_from_block): Remove the static qualifier.
	* symtab.h (lookup_objfile_from_block): New declaration.
	(struct general_symbol_info) <obj_section>: Extend the comment.

gdb/testsuite/
2010-04-19  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix crashes on dangling display expressions.
	* gdb.base/solib-display.exp: Call gdb_gnu_strip_debug if LIBSEPDEBUG
	is SEP.
	(lib_flags): Remove the "debug" keyword.
	(libsepdebug): New variable for iterating new loop.
	(save_pf_prefix): New variable wrapping the loop.
	(sep_lib_flags): New variable derived from LIB_FLAGS.  Use it.
	* lib/gdb.exp (gdb_gnu_strip_debug): Document the return code.

--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -10868,6 +10868,36 @@ ada_operator_length (struct expression *exp, int pc, int *oplenp, int *argsp)
     }
 }
 
+/* Implementation of the exp_descriptor method operator_check.  */
+
+static int
+ada_operator_check (struct expression *exp, int pos,
+		    int (*objfile_func) (struct objfile *objfile, void *data),
+		    void *data)
+{
+  const union exp_element *const elts = exp->elts;
+  struct type *type = NULL;
+
+  switch (elts[pos].opcode)
+    {
+      case UNOP_IN_RANGE:
+      case UNOP_QUAL:
+	type = elts[pos + 1].type;
+	break;
+
+      default:
+	return operator_check_standard (exp, pos, objfile_func, data);
+    }
+
+  /* Invoke callbacks for TYPE and OBJFILE if they were set as non-NULL.  */
+
+  if (type && TYPE_OBJFILE (type)
+      && (*objfile_func) (TYPE_OBJFILE (type), data))
+    return 1;
+
+  return 0;
+}
+
 static char *
 ada_op_name (enum exp_opcode opcode)
 {
@@ -11256,6 +11286,7 @@ parse (void)
 static const struct exp_descriptor ada_exp_descriptor = {
   ada_print_subexp,
   ada_operator_length,
+  ada_operator_check,
   ada_op_name,
   ada_dump_subexp_body,
   ada_evaluate_subexp
--- a/gdb/c-lang.c
+++ b/gdb/c-lang.c
@@ -1139,6 +1139,7 @@ static const struct exp_descriptor exp_descriptor_c =
 {
   print_subexp_standard,
   operator_length_standard,
+  operator_check_standard,
   op_name_standard,
   dump_subexp_body_standard,
   evaluate_subexp_c
--- a/gdb/jv-lang.c
+++ b/gdb/jv-lang.c
@@ -1162,6 +1162,7 @@ const struct exp_descriptor exp_descriptor_java =
 {
   print_subexp_standard,
   operator_length_standard,
+  operator_check_standard,
   op_name_standard,
   dump_subexp_body_standard,
   evaluate_subexp_java
--- a/gdb/m2-lang.c
+++ b/gdb/m2-lang.c
@@ -356,6 +356,7 @@ const struct exp_descriptor exp_descriptor_modula2 =
 {
   print_subexp_standard,
   operator_length_standard,
+  operator_check_standard,
   op_name_standard,
   dump_subexp_body_standard,
   evaluate_subexp_modula2
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -62,6 +62,7 @@ const struct exp_descriptor exp_descriptor_standard =
   {
     print_subexp_standard,
     operator_length_standard,
+    operator_check_standard,
     op_name_standard,
     dump_subexp_body_standard,
     evaluate_subexp_standard
@@ -1373,6 +1374,150 @@ parser_fprintf (FILE *x, const char *y, ...)
   va_end (args);
 }
 
+/* Implementation of the exp_descriptor method operator_check.  */
+
+int
+operator_check_standard (struct expression *exp, int pos,
+			 int (*objfile_func) (struct objfile *objfile,
+					      void *data),
+			 void *data)
+{
+  const union exp_element *const elts = exp->elts;
+  struct type *type = NULL;
+  struct objfile *objfile = NULL;
+
+  /* Extended operators should have been already handled by exp_descriptor
+     iterate method of its specific language.  */
+  gdb_assert (elts[pos].opcode < OP_EXTENDED0);
+
+  /* Track the callers of write_exp_elt_type for this table.  */
+
+  switch (elts[pos].opcode)
+    {
+    case BINOP_VAL:
+    case OP_COMPLEX:
+    case OP_DECFLOAT:
+    case OP_DOUBLE:
+    case OP_LONG:
+    case OP_SCOPE:
+    case OP_TYPE:
+    case UNOP_CAST:
+    case UNOP_DYNAMIC_CAST:
+    case UNOP_REINTERPRET_CAST:
+    case UNOP_MAX:
+    case UNOP_MEMVAL:
+    case UNOP_MIN:
+      type = elts[pos + 1].type;
+      break;
+
+    case TYPE_INSTANCE:
+      {
+	LONGEST arg, nargs = elts[pos + 1].longconst;
+
+	for (arg = 0; arg < nargs; arg++)
+	  {
+	    struct type *type = elts[pos + 2 + arg].type;
+	    struct objfile *objfile = TYPE_OBJFILE (type);
+
+	    if (objfile && (*objfile_func) (objfile, data))
+	      return 1;
+	  }
+      }
+      break;
+
+    case UNOP_MEMVAL_TLS:
+      objfile = elts[pos + 1].objfile;
+      type = elts[pos + 2].type;
+      break;
+
+    case OP_VAR_VALUE:
+      {
+	const struct block *const block = elts[pos + 1].block;
+	const struct symbol *const symbol = elts[pos + 2].symbol;
+
+	/* Check objfile where the variable itself is placed.
+	   SYMBOL_OBJ_SECTION (symbol) may be NULL.  */
+	if ((*objfile_func) (SYMBOL_SYMTAB (symbol)->objfile, data))
+	  return 1;
+
+	/* Check objfile where is placed the code touching the variable.  */
+	objfile = lookup_objfile_from_block (block);
+
+	type = SYMBOL_TYPE (symbol);
+      }
+      break;
+    }
+
+  /* Invoke callbacks for TYPE and OBJFILE if they were set as non-NULL.  */
+
+  if (type && TYPE_OBJFILE (type)
+      && (*objfile_func) (TYPE_OBJFILE (type), data))
+    return 1;
+  if (objfile && (*objfile_func) (objfile, data))
+    return 1;
+
+  return 0;
+}
+
+/* Call OBJFILE_FUNC for any TYPE and OBJFILE found being referenced by EXP.
+   The functions are never called with NULL OBJFILE.  Functions get passed an
+   arbitrary caller supplied DATA pointer.  If any of the functions returns
+   non-zero value then (any other) non-zero value is immediately returned to
+   the caller.  Otherwise zero is returned after iterating through whole EXP.
+   */
+
+static int
+exp_iterate (struct expression *exp,
+	     int (*objfile_func) (struct objfile *objfile, void *data),
+	     void *data)
+{
+  int endpos;
+  const union exp_element *const elts = exp->elts;
+
+  for (endpos = exp->nelts; endpos > 0; )
+    {
+      int pos, args, oplen = 0;
+
+      exp->language_defn->la_exp_desc->operator_length (exp, endpos,
+							&oplen, &args);
+      gdb_assert (oplen > 0);
+
+      pos = endpos - oplen;
+      if (exp->language_defn->la_exp_desc->operator_check (exp, pos,
+							   objfile_func, data))
+	return 1;
+
+      endpos = pos;
+    }
+
+  return 0;
+}
+
+/* Helper for exp_uses_objfile.  */
+
+static int
+exp_uses_objfile_iter (struct objfile *exp_objfile, void *objfile_voidp)
+{
+  struct objfile *objfile = objfile_voidp;
+
+  if (exp_objfile->separate_debug_objfile_backlink)
+    exp_objfile = exp_objfile->separate_debug_objfile_backlink;
+
+  return exp_objfile == objfile;
+}
+
+/* Return 1 if EXP uses OBJFILE (and will become dangling when OBJFILE
+   is unloaded), otherwise return 0.  OBJFILE must not be a separate debug info
+   file.  */
+
+int
+exp_uses_objfile (struct expression *exp, struct objfile *objfile)
+{
+  gdb_assert (objfile->separate_debug_objfile_backlink == NULL);
+
+  return exp_iterate (exp, exp_uses_objfile_iter, objfile);
+}
+
 void
 _initialize_parse (void)
 {
--- a/gdb/parser-defs.h
+++ b/gdb/parser-defs.h
@@ -192,6 +192,11 @@ extern void operator_length (struct expression *, int, int *, int *);
 
 extern void operator_length_standard (struct expression *, int, int *, int *);
 
+extern int operator_check_standard (struct expression *exp, int pos,
+				    int (*objfile_func)
+				      (struct objfile *objfile, void *data),
+				    void *data);
+
 extern char *op_name_standard (enum exp_opcode);
 
 extern struct type *follow_types (struct type *);
@@ -270,6 +275,19 @@ struct exp_descriptor
        the number of subexpressions it takes.  */
     void (*operator_length) (struct expression*, int, int*, int *);
 
+    /* Call TYPE_FUNC and OBJFILE_FUNC for any TYPE and OBJFILE found being
+       referenced by the single operator of EXP at position POS.  Operator
+       parameters are located at positive (POS + number) offsets in EXP.
+       The functions should never be called with NULL TYPE or NULL OBJFILE.
+       Functions should get passed an arbitrary caller supplied DATA pointer.
+       If any of the functions returns non-zero value then (any other) non-zero
+       value should be immediately returned to the caller.  Otherwise zero
+       should be returned.  */
+    int (*operator_check) (struct expression *exp, int pos,
+			   int (*objfile_func) (struct objfile *objfile,
+						void *data),
+			   void *data);
+
     /* Name of this operator for dumping purposes.  */
     char *(*op_name) (enum exp_opcode);
 
@@ -302,4 +320,6 @@ extern void print_subexp_standard (struct expression *, int *,
 
 extern void parser_fprintf (FILE *, const char *, ...) ATTR_FORMAT (printf, 2 ,3);
 
+extern int exp_uses_objfile (struct expression *exp, struct objfile *objfile);
+
 #endif /* PARSER_DEFS_H */
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -46,7 +46,6 @@
 #include "exceptions.h"
 #include "observer.h"
 #include "solist.h"
-#include "solib.h"
 #include "parser-defs.h"
 #include "charset.h"
 #include "arch-utils.h"
@@ -1859,51 +1858,6 @@ disable_display_command (char *args, int from_tty)
       }
 }
 
-/* Return 1 if D uses SOLIB (and will become dangling when SOLIB
-   is unloaded), otherwise return 0.  */
-
-static int
-display_uses_solib_p (const struct display *d,
-		      const struct so_list *solib)
-{
-  int endpos;
-  struct expression *const exp = d->exp;
-  const union exp_element *const elts = exp->elts;
-
-  if (d->block != NULL
-      && d->pspace == solib->pspace
-      && solib_contains_address_p (solib, d->block->startaddr))
-    return 1;
-
-  for (endpos = exp->nelts; endpos > 0; )
-    {
-      int i, args, oplen = 0;
-
-      exp->language_defn->la_exp_desc->operator_length (exp, endpos,
-							&oplen, &args);
-      gdb_assert (oplen > 0);
-
-      i = endpos - oplen;
-      if (elts[i].opcode == OP_VAR_VALUE)
-	{
-	  const struct block *const block = elts[i + 1].block;
-	  const struct symbol *const symbol = elts[i + 2].symbol;
-
-	  if (block != NULL
-	      && solib_contains_address_p (solib,
-					   block->startaddr))
-	    return 1;
-
-	  /* SYMBOL_OBJ_SECTION (symbol) may be NULL.  */
-	  if (SYMBOL_SYMTAB (symbol)->objfile == solib->objfile)
-	    return 1;
-	}
-      endpos -= oplen;
-    }
-
-  return 0;
-}
-
 /* display_chain items point to blocks and expressions.  Some expressions in
    turn may point to symbols.
    Both symbols and blocks are obstack_alloc'd on objfile_stack, and are
@@ -1915,17 +1869,28 @@ display_uses_solib_p (const struct display *d,
 static void
 clear_dangling_display_expressions (struct so_list *solib)
 {
+  struct objfile *objfile = solib->objfile;
   struct display *d;
-  struct objfile *objfile = NULL;
 
-  for (d = display_chain; d; d = d->next)
+  /* With no symbol file we cannot have a block or expression from it.  */
+  if (objfile == NULL)
+    return;
+  if (objfile->separate_debug_objfile_backlink)
+    objfile = objfile->separate_debug_objfile_backlink;
+  gdb_assert (objfile->pspace == solib->pspace);
+
+  for (d = display_chain; d != NULL; d = d->next)
     {
-      if (d->exp && display_uses_solib_p (d, solib))
-	{
-	  xfree (d->exp);
-	  d->exp = NULL;
-	  d->block = NULL;
-	}
+      if (d->pspace != solib->pspace)
+	continue;
+
+      if (lookup_objfile_from_block (d->block) == objfile
+	  || (d->exp && exp_uses_objfile (d->exp, objfile)))
+      {
+	xfree (d->exp);
+	d->exp = NULL;
+	d->block = NULL;
+      }
     }
 }
 \f
--- a/gdb/scm-lang.c
+++ b/gdb/scm-lang.c
@@ -231,6 +231,7 @@ const struct exp_descriptor exp_descriptor_scm =
 {
   print_subexp_standard,
   operator_length_standard,
+  operator_check_standard,
   op_name_standard,
   dump_subexp_body_standard,
   evaluate_exp
--- a/gdb/solist.h
+++ b/gdb/solist.h
@@ -63,7 +63,12 @@ struct so_list
 
     bfd *abfd;
     char symbols_loaded;	/* flag: symbols read in yet? */
-    struct objfile *objfile;	/* objfile for loaded lib */
+
+    /* objfile with symbols for a loaded library.  Target memory is read from
+       ABFD.  OBJFILE may be NULL either before symbols have been loaded, if
+       the file cannot be found or after the command "nosharedlibrary".  */
+    struct objfile *objfile;
+
     struct target_section *sections;
     struct target_section *sections_end;
 
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -1150,7 +1150,7 @@ lookup_symbol_aux_local (const char *name, const struct block *block,
 
 /* Look up OBJFILE to BLOCK.  */
 
-static struct objfile *
+struct objfile *
 lookup_objfile_from_block (const struct block *block)
 {
   struct objfile *obj;
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -148,7 +148,7 @@ struct general_symbol_info
 
   short section;
 
-  /* The section associated with this symbol. */
+  /* The section associated with this symbol.  It can be NULL.  */
 
   struct obj_section *obj_section;
 };
@@ -1206,4 +1206,6 @@ int producer_is_realview (const char *producer);
 void fixup_section (struct general_symbol_info *ginfo,
 		    CORE_ADDR addr, struct objfile *objfile);
 
+struct objfile *lookup_objfile_from_block (const struct block *block);
+
 #endif /* !defined(SYMTAB_H) */
--- a/gdb/testsuite/gdb.base/solib-display.exp
+++ b/gdb/testsuite/gdb.base/solib-display.exp
@@ -36,7 +36,7 @@ if { [skip_shlib_tests] || [is_remote target] } {
 set libname "solib-display-lib"
 set srcfile_lib ${srcdir}/${subdir}/${libname}.c
 set binfile_lib ${objdir}/${subdir}/${libname}.so
-set lib_flags [list debug]
+set lib_flags {}
 # Binary file.
 set testfile "solib-display-main"
 set srcfile ${srcdir}/${subdir}/${testfile}.c
@@ -48,60 +48,92 @@ if [get_compiler_info ${binfile}] {
     return -1
 }
 
-if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $lib_flags] != ""
-     || [gdb_compile ${srcfile} ${binfile} executable $bin_flags] != "" } {
-  untested "Could not compile $binfile_lib or $binfile."
-  return -1
+set save_pf_prefix $pf_prefix
+# SEP must be last for the possible `unsupported' error path.
+foreach libsepdebug {NO IN SEP} {
+
+    set pf_prefix $save_pf_prefix
+    lappend pf_prefix "$libsepdebug:"
+
+    set sep_lib_flags $lib_flags
+    if {$libsepdebug != "NO"} {
+	lappend sep_lib_flags {debug}
+    }
+    if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $sep_lib_flags] != ""
+	 || [gdb_compile ${srcfile} ${binfile} executable $bin_flags] != "" } {
+      untested "Could not compile $binfile_lib or $binfile."
+      return -1
+    }
+
+    if {$libsepdebug == "SEP"} {
+	if {[gdb_gnu_strip_debug $binfile_lib] != 0} {
+	    unsupported "Could not split debug of $binfile_lib."
+	    return
+	} else {
+	    pass "split solib"
+	}
+    }
+
+    clean_restart $executable
+
+    if ![runto_main] then {
+      fail "Can't run to main"
+      return 0
+    }
+
+    gdb_test "display a_global" "1: a_global = 41"
+    gdb_test "display b_global" "2: b_global = 42"
+    gdb_test "display c_global" "3: c_global = 43"
+
+    if { [gdb_start_cmd] < 0 } {
+	fail "Can't run to main (2)"
+	continue
+    }
+
+    gdb_test "" "3: c_global = 43\\r\\n2: b_global = 42\\r\\n1: a_global = 41" "after rerun"
+
+    # Now rebuild the library without b_global
+    if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} \
+	      "$sep_lib_flags additional_flags=-DNO_B_GLOBAL"] != ""} {
+	fail "Can't rebuild $binfile_lib"
+    }
+
+    if {$libsepdebug == "SEP"} {
+	set test "split solib second time"
+	if {[gdb_gnu_strip_debug $binfile_lib] != 0} {
+	    fail $test
+	    continue
+	} else {
+	    pass $test
+	}
+    }
+
+    if { [gdb_start_cmd] < 0 } {
+	fail "Can't run to main (3)"
+	continue
+    }
+
+    gdb_test "" "3: c_global = 43\\r\\nwarning: .*b_global.*\\r\\n1: a_global = 41" "after rerun (2)"
+
+    # Now verify that displays which are not in the shared library
+    # are not cleared permaturely.
+
+    gdb_test "break [gdb_get_line_number "break here" ${testfile}.c]" \
+	    ".*Breakpoint.* at .*" 
+
+    gdb_test "continue"
+    gdb_test "display main_global" "4: main_global = 44"
+    gdb_test "display a_local" "5: a_local = 45"
+    gdb_test "display a_static" "6: a_static = 46"
+
+    if { [gdb_start_cmd] < 0 } {
+	fail "Can't run to main (4)"
+	continue
+    }
+
+    gdb_test "" "6: a_static = 46\\r\\n4: main_global = 44\\r\\n.*"
+    gdb_test "break [gdb_get_line_number "break here" ${testfile}.c]" \
+	    ".*Breakpoint.* at .*" 
+    gdb_test "continue" "6: a_static = 46\\r\\n5: a_local = 45\\r\\n4: main_global = 44\\r\\n.*"
 }
-
-clean_restart ${executable}
-
-if ![runto_main] then {
-  fail "Can't run to main"
-  return 0
-}
-
-gdb_test "display a_global" "1: a_global = 41"
-gdb_test "display b_global" "2: b_global = 42"
-gdb_test "display c_global" "3: c_global = 43"
-
-if { [gdb_start_cmd] < 0 } {
-    fail "Can't run to main (2)"
-    return 0
-}
-
-gdb_test "" "3: c_global = 43\\r\\n2: b_global = 42\\r\\n1: a_global = 41" "after rerun"
-
-# Now rebuild the library without b_global
-if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} \
-	  "$lib_flags additional_flags=-DNO_B_GLOBAL"] != ""} {
-    fail "Can't rebuild $binfile_lib"
-}
-
-if { [gdb_start_cmd] < 0 } {
-    fail "Can't run to main (3)"
-    return 0
-}
-
-gdb_test "" "3: c_global = 43\\r\\nwarning: .*b_global.*\\r\\n1: a_global = 41" "after rerun"
-
-# Now verify that displays which are not in the shared library
-# are not cleared permaturely.
-
-gdb_test "break [gdb_get_line_number "break here" ${testfile}.c]" \
-	".*Breakpoint.* at .*" 
-
-gdb_test "continue"
-gdb_test "display main_global" "4: main_global = 44"
-gdb_test "display a_local" "5: a_local = 45"
-gdb_test "display a_static" "6: a_static = 46"
-
-if { [gdb_start_cmd] < 0 } {
-    fail "Can't run to main (4)"
-    return 0
-}
-
-gdb_test "" "6: a_static = 46\\r\\n4: main_global = 44\\r\\n.*"
-gdb_test "break [gdb_get_line_number "break here" ${testfile}.c]" \
-	".*Breakpoint.* at .*" 
-gdb_test "continue" "6: a_static = 46\\r\\n5: a_local = 45\\r\\n4: main_global = 44\\r\\n.*"
+set pf_prefix $save_pf_prefix
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2896,6 +2896,9 @@ proc build_id_debug_filename_get { exec } {
 # Create stripped files for DEST, replacing it.  If ARGS is passed, it is a
 # list of optional flags.  The only currently supported flag is no-main,
 # which removes the symbol entry for main from the separate debug file.
+#
+# Function returns zero on success.  Function will return non-zero failure code
+# on some targets not supporting separate debug info (such as i386-msdos).
 
 proc gdb_gnu_strip_debug { dest args } {
 

Re: [patch] Fix dangling displays in separate debug

[Pedro Alves]

On Monday 19 April 2010 15:25:03, Jan Kratochvil wrote:
> On Sun, 11 Apr 2010 03:27:35 +0200, Pedro Alves wrote:
> > On Friday 09 April 2010 21:00:46, Jan Kratochvil wrote:
> > > I was convinced due to some invalid reasons solib->objfile cannot be NULL.
> > 
> > There's an easy way to see that state:
> > 
> >  (gdb) nosharedlibrary
> >  (gdb) info sharedlibrary
> 
> Nice reproducer, thanks.
> 
> 
> > Doesn't this make it so that only the separate debug info
> > case is exercised, and, it stops exercising the non-separate
> > debug info case?  I think we should instead be running the
> > relevant parts the test twice instead?
> 
> I was asking about it before.  Therefore reworked it as promised.
> 
> On Sat, 03 Apr 2010 11:55:58 +0200, Jan Kratochvil wrote:
> # One may only address that gdb.base/solib-display.exp was testing symbol
> # in-objfile while now it tests only symbol in-sepdebug-objfile and no longer
> # the in-objfile case.  I find the in-sepdebug-objfile as a superset of
> # in-objfile test but I can rework it if anyones addresses this test change.

Thanks.

> 
> 
> > > +if {[gdb_gnu_strip_debug $binfile_lib] != 0} {
> > > +    fail $test
> > 
> > Not all systems support this, so this should be `unsupported' instead.
> 
> You are right, found at least one such target - i386-msdos:
> + ./binutils/objcopy --add-gnu-debuglink=1.o-debug 1.o-stripped 1.o-dest
> BFD: 1.o-dest: can not represent section `.gnu_debuglink' in a.out object file format
> ./binutils/objcopy:1.o-dest: cannot fill debug link section `1.o-debug': Nonrepresentable section on output
> 
> 
> With the extended requirements I rather pushed the former patch posted at:
> 	Re: [patch 3/8] Types GC [display_uses_solib_p to exp_iterate]
> 	http://sourceware.org/ml/gdb-patches/2009-07/msg00054.html

Yikes.  I don't understand what is the extended requirement, but anyway,
I'll take a look.

> 
> There was forgotten handling of TYPE_INSTANCE, UNOP_DYNAMIC_CAST and
> UNOP_REINTERPRET_CAST.  Reduced it only for the current applicable use case;
> to be extended one day for the types garbage collector on its resubmit.
> 
> 
> No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.
> 
> 
> Thanks,
> Jan
> 
> 
> gdb/
> 2010-04-19  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix crashes on dangling display expressions.
> 	* ada-lang.c (ada_operator_check): New function.
> 	(ada_exp_descriptor): Fill-in the field operator_check.
> 	* c-lang.c (exp_descriptor_c): Fill-in the field operator_check.
> 	* jv-lang.c (exp_descriptor_java): Likewise.
> 	* m2-lang.c (exp_descriptor_modula2): Likewise.
> 	* scm-lang.c (exp_descriptor_scm): Likewise.
> 	* parse.c (exp_descriptor_standard): Likewise.
> 	(operator_check_standard): New function.
> 	(exp_iterate, exp_uses_objfile_iter, exp_uses_objfile): New functions.
> 	* parser-defs.h (struct exp_descriptor): New field operator_check.
> 	(operator_check_standard, exp_uses_objfile): New declarations.
> 	* printcmd.c: Remove the inclusion of solib.h.
> 	(display_uses_solib_p): Remove the function.
> 	(clear_dangling_display_expressions): Call lookup_objfile_from_block
> 	and exp_uses_objfile instead of display_uses_solib_p.
> 	* solist.h (struct so_list) <objfile>: New comment.
> 	* symtab.c (lookup_objfile_from_block): Remove the static qualifier.
> 	* symtab.h (lookup_objfile_from_block): New declaration.
> 	(struct general_symbol_info) <obj_section>: Extend the comment.
> 
> gdb/testsuite/
> 2010-04-19  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix crashes on dangling display expressions.
> 	* gdb.base/solib-display.exp: Call gdb_gnu_strip_debug if LIBSEPDEBUG
> 	is SEP.
> 	(lib_flags): Remove the "debug" keyword.
> 	(libsepdebug): New variable for iterating new loop.
> 	(save_pf_prefix): New variable wrapping the loop.
> 	(sep_lib_flags): New variable derived from LIB_FLAGS.  Use it.
> 	* lib/gdb.exp (gdb_gnu_strip_debug): Document the return code.
> 
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -10868,6 +10868,36 @@ ada_operator_length (struct expression *exp, int pc, int *oplenp, int *argsp)
>      }
>  }
>  
> +/* Implementation of the exp_descriptor method operator_check.  */
> +
> +static int
> +ada_operator_check (struct expression *exp, int pos,
> +		    int (*objfile_func) (struct objfile *objfile, void *data),
> +		    void *data)
> +{
> +  const union exp_element *const elts = exp->elts;
> +  struct type *type = NULL;
> +
> +  switch (elts[pos].opcode)
> +    {
> +      case UNOP_IN_RANGE:
> +      case UNOP_QUAL:
> +	type = elts[pos + 1].type;
> +	break;
> +
> +      default:
> +	return operator_check_standard (exp, pos, objfile_func, data);
> +    }
> +
> +  /* Invoke callbacks for TYPE and OBJFILE if they were set as non-NULL.  */
> +
> +  if (type && TYPE_OBJFILE (type)
> +      && (*objfile_func) (TYPE_OBJFILE (type), data))
> +    return 1;
> +
> +  return 0;
> +}
> +
>  static char *
>  ada_op_name (enum exp_opcode opcode)
>  {
> @@ -11256,6 +11286,7 @@ parse (void)
>  static const struct exp_descriptor ada_exp_descriptor = {
>    ada_print_subexp,
>    ada_operator_length,
> +  ada_operator_check,
>    ada_op_name,
>    ada_dump_subexp_body,
>    ada_evaluate_subexp
> --- a/gdb/c-lang.c
> +++ b/gdb/c-lang.c
> @@ -1139,6 +1139,7 @@ static const struct exp_descriptor exp_descriptor_c =
>  {
>    print_subexp_standard,
>    operator_length_standard,
> +  operator_check_standard,
>    op_name_standard,
>    dump_subexp_body_standard,
>    evaluate_subexp_c
> --- a/gdb/jv-lang.c
> +++ b/gdb/jv-lang.c
> @@ -1162,6 +1162,7 @@ const struct exp_descriptor exp_descriptor_java =
>  {
>    print_subexp_standard,
>    operator_length_standard,
> +  operator_check_standard,
>    op_name_standard,
>    dump_subexp_body_standard,
>    evaluate_subexp_java
> --- a/gdb/m2-lang.c
> +++ b/gdb/m2-lang.c
> @@ -356,6 +356,7 @@ const struct exp_descriptor exp_descriptor_modula2 =
>  {
>    print_subexp_standard,
>    operator_length_standard,
> +  operator_check_standard,
>    op_name_standard,
>    dump_subexp_body_standard,
>    evaluate_subexp_modula2
> --- a/gdb/parse.c
> +++ b/gdb/parse.c
> @@ -62,6 +62,7 @@ const struct exp_descriptor exp_descriptor_standard =
>    {
>      print_subexp_standard,
>      operator_length_standard,
> +    operator_check_standard,
>      op_name_standard,
>      dump_subexp_body_standard,
>      evaluate_subexp_standard
> @@ -1373,6 +1374,150 @@ parser_fprintf (FILE *x, const char *y, ...)
>    va_end (args);
>  }
>  
> +/* Implementation of the exp_descriptor method operator_check.  */
> +
> +int
> +operator_check_standard (struct expression *exp, int pos,
> +			 int (*objfile_func) (struct objfile *objfile,
> +					      void *data),
> +			 void *data)
> +{
> +  const union exp_element *const elts = exp->elts;
> +  struct type *type = NULL;
> +  struct objfile *objfile = NULL;
> +
> +  /* Extended operators should have been already handled by exp_descriptor
> +     iterate method of its specific language.  */
> +  gdb_assert (elts[pos].opcode < OP_EXTENDED0);
> +
> +  /* Track the callers of write_exp_elt_type for this table.  */
> +
> +  switch (elts[pos].opcode)
> +    {
> +    case BINOP_VAL:
> +    case OP_COMPLEX:
> +    case OP_DECFLOAT:
> +    case OP_DOUBLE:
> +    case OP_LONG:
> +    case OP_SCOPE:
> +    case OP_TYPE:
> +    case UNOP_CAST:
> +    case UNOP_DYNAMIC_CAST:
> +    case UNOP_REINTERPRET_CAST:
> +    case UNOP_MAX:
> +    case UNOP_MEMVAL:
> +    case UNOP_MIN:
> +      type = elts[pos + 1].type;
> +      break;
> +
> +    case TYPE_INSTANCE:
> +      {
> +	LONGEST arg, nargs = elts[pos + 1].longconst;
> +
> +	for (arg = 0; arg < nargs; arg++)
> +	  {
> +	    struct type *type = elts[pos + 2 + arg].type;
> +	    struct objfile *objfile = TYPE_OBJFILE (type);
> +
> +	    if (objfile && (*objfile_func) (objfile, data))
> +	      return 1;
> +	  }
> +      }
> +      break;
> +
> +    case UNOP_MEMVAL_TLS:
> +      objfile = elts[pos + 1].objfile;
> +      type = elts[pos + 2].type;
> +      break;
> +
> +    case OP_VAR_VALUE:
> +      {
> +	const struct block *const block = elts[pos + 1].block;
> +	const struct symbol *const symbol = elts[pos + 2].symbol;
> +
> +	/* Check objfile where the variable itself is placed.
> +	   SYMBOL_OBJ_SECTION (symbol) may be NULL.  */
> +	if ((*objfile_func) (SYMBOL_SYMTAB (symbol)->objfile, data))
> +	  return 1;
> +
> +	/* Check objfile where is placed the code touching the variable.  */
> +	objfile = lookup_objfile_from_block (block);
> +
> +	type = SYMBOL_TYPE (symbol);
> +      }
> +      break;
> +    }
> +
> +  /* Invoke callbacks for TYPE and OBJFILE if they were set as non-NULL.  */
> +
> +  if (type && TYPE_OBJFILE (type)
> +      && (*objfile_func) (TYPE_OBJFILE (type), data))
> +    return 1;
> +  if (objfile && (*objfile_func) (objfile, data))
> +    return 1;
> +
> +  return 0;
> +}
> +
> +/* Call OBJFILE_FUNC for any TYPE and OBJFILE found being referenced by EXP.
> +   The functions are never called with NULL OBJFILE.  Functions get passed an
> +   arbitrary caller supplied DATA pointer.  If any of the functions returns
> +   non-zero value then (any other) non-zero value is immediately returned to
> +   the caller.  Otherwise zero is returned after iterating through whole EXP.
> +   */
> +
> +static int
> +exp_iterate (struct expression *exp,
> +	     int (*objfile_func) (struct objfile *objfile, void *data),
> +	     void *data)
> +{
> +  int endpos;
> +  const union exp_element *const elts = exp->elts;
> +
> +  for (endpos = exp->nelts; endpos > 0; )
> +    {
> +      int pos, args, oplen = 0;
> +
> +      exp->language_defn->la_exp_desc->operator_length (exp, endpos,
> +							&oplen, &args);
> +      gdb_assert (oplen > 0);
> +
> +      pos = endpos - oplen;
> +      if (exp->language_defn->la_exp_desc->operator_check (exp, pos,
> +							   objfile_func, data))
> +	return 1;
> +
> +      endpos = pos;
> +    }
> +
> +  return 0;
> +}
> +
> +/* Helper for exp_uses_objfile.  */
> +
> +static int
> +exp_uses_objfile_iter (struct objfile *exp_objfile, void *objfile_voidp)
> +{
> +  struct objfile *objfile = objfile_voidp;
> +
> +  if (exp_objfile->separate_debug_objfile_backlink)
> +    exp_objfile = exp_objfile->separate_debug_objfile_backlink;
> +
> +  return exp_objfile == objfile;
> +}
> +

Most things look good.  This is get's a little blury though.  You have
this nice iterator that can determine if a given expression involved an
obfile, so, you could accurately now disable a "display" when an objfile
is unloaded from gdb --- _any_ objfile, even in principly when you
do "file" to unload the main exec's symbols, say, so that _all_ cases
of dangling displays would be cured.  Still, you only call this when an
solib is unloaded.  Can't we in principle be unloading a separate debug info
objfile, but still have the main one loaded, hence OBJFILE_VOIDP could be
a separate debug objfile?  An expression could in principle be using
symbols from either, why would we care for that here?  Doesn't this just
beg for a new `objfile_unloaded' observer (called from, say free_objfile)?
Anyway, I'm not going to require you do that for this patch, your patch
looks good already.  Just dumping my thoughts, on what looks like an
opportunity to keep the solib/objfile concepts cleanly separate, and get
rid of all the objfile backlinking here.

> +/* Return 1 if EXP uses OBJFILE (and will become dangling when OBJFILE
> +   is unloaded), otherwise return 0.  OBJFILE must not be a separate debug info
> +   file.  */
> +
> +int
> +exp_uses_objfile (struct expression *exp, struct objfile *objfile)
> +{
> +  gdb_assert (objfile->separate_debug_objfile_backlink == NULL);
> +
> +  return exp_iterate (exp, exp_uses_objfile_iter, objfile);
> +}
> +
>  void
>  _initialize_parse (void)
>  {
> --- a/gdb/parser-defs.h
> +++ b/gdb/parser-defs.h
> @@ -192,6 +192,11 @@ extern void operator_length (struct expression *, int, int *, int *);
>  
>  extern void operator_length_standard (struct expression *, int, int *, int *);
>  
> +extern int operator_check_standard (struct expression *exp, int pos,
> +				    int (*objfile_func)
> +				      (struct objfile *objfile, void *data),
> +				    void *data);
> +
>  extern char *op_name_standard (enum exp_opcode);
>  
>  extern struct type *follow_types (struct type *);
> @@ -270,6 +275,19 @@ struct exp_descriptor
>         the number of subexpressions it takes.  */
>      void (*operator_length) (struct expression*, int, int*, int *);
>  
> +    /* Call TYPE_FUNC and OBJFILE_FUNC for any TYPE and OBJFILE found being
> +       referenced by the single operator of EXP at position POS.  Operator
> +       parameters are located at positive (POS + number) offsets in EXP.
> +       The functions should never be called with NULL TYPE or NULL OBJFILE.
> +       Functions should get passed an arbitrary caller supplied DATA pointer.
> +       If any of the functions returns non-zero value then (any other) non-zero
> +       value should be immediately returned to the caller.  Otherwise zero
> +       should be returned.  */
> +    int (*operator_check) (struct expression *exp, int pos,
> +			   int (*objfile_func) (struct objfile *objfile,
> +						void *data),
> +			   void *data);
> +
>      /* Name of this operator for dumping purposes.  */
>      char *(*op_name) (enum exp_opcode);
>  
> @@ -302,4 +320,6 @@ extern void print_subexp_standard (struct expression *, int *,
>  
>  extern void parser_fprintf (FILE *, const char *, ...) ATTR_FORMAT (printf, 2 ,3);
>  
> +extern int exp_uses_objfile (struct expression *exp, struct objfile *objfile);
> +
>  #endif /* PARSER_DEFS_H */
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -46,7 +46,6 @@
>  #include "exceptions.h"
>  #include "observer.h"
>  #include "solist.h"
> -#include "solib.h"
>  #include "parser-defs.h"
>  #include "charset.h"
>  #include "arch-utils.h"
> @@ -1859,51 +1858,6 @@ disable_display_command (char *args, int from_tty)
>        }
>  }
>  
> -/* Return 1 if D uses SOLIB (and will become dangling when SOLIB
> -   is unloaded), otherwise return 0.  */
> -
> -static int
> -display_uses_solib_p (const struct display *d,
> -		      const struct so_list *solib)
> -{
> -  int endpos;
> -  struct expression *const exp = d->exp;
> -  const union exp_element *const elts = exp->elts;
> -
> -  if (d->block != NULL
> -      && d->pspace == solib->pspace
> -      && solib_contains_address_p (solib, d->block->startaddr))
> -    return 1;

Is there any `solib_contains_address_p' check left after the patch?
What about displays that don't involve symbols, like:

 `display *0xaddr_in_solib'.

They'll probably still disable themselves when memory reading
fails, but, will we now see an ugly error, or something?

> -
> -  for (endpos = exp->nelts; endpos > 0; )
> -    {
> -      int i, args, oplen = 0;
> -
> -      exp->language_defn->la_exp_desc->operator_length (exp, endpos,
> -							&oplen, &args);
> -      gdb_assert (oplen > 0);
> -
> -      i = endpos - oplen;
> -      if (elts[i].opcode == OP_VAR_VALUE)
> -	{
> -	  const struct block *const block = elts[i + 1].block;
> -	  const struct symbol *const symbol = elts[i + 2].symbol;
> -
> -	  if (block != NULL
> -	      && solib_contains_address_p (solib,
> -					   block->startaddr))
> -	    return 1;
> -
> -	  /* SYMBOL_OBJ_SECTION (symbol) may be NULL.  */
> -	  if (SYMBOL_SYMTAB (symbol)->objfile == solib->objfile)
> -	    return 1;
> -	}
> -      endpos -= oplen;
> -    }
> -
> -  return 0;
> -}
> -
>  /* display_chain items point to blocks and expressions.  Some expressions in
>     turn may point to symbols.
>     Both symbols and blocks are obstack_alloc'd on objfile_stack, and are
> @@ -1915,17 +1869,28 @@ display_uses_solib_p (const struct display *d,
>  static void
>  clear_dangling_display_expressions (struct so_list *solib)
>  {
> +  struct objfile *objfile = solib->objfile;
>    struct display *d;
> -  struct objfile *objfile = NULL;
>  
> -  for (d = display_chain; d; d = d->next)
> +  /* With no symbol file we cannot have a block or expression from it.  */
> +  if (objfile == NULL)
> +    return;
> +  if (objfile->separate_debug_objfile_backlink)
> +    objfile = objfile->separate_debug_objfile_backlink;
> +  gdb_assert (objfile->pspace == solib->pspace);
> +
> +  for (d = display_chain; d != NULL; d = d->next)
>      {
> -      if (d->exp && display_uses_solib_p (d, solib))
> -	{
> -	  xfree (d->exp);
> -	  d->exp = NULL;
> -	  d->block = NULL;
> -	}
> +      if (d->pspace != solib->pspace)
> +	continue;
> +
> +      if (lookup_objfile_from_block (d->block) == objfile
> +	  || (d->exp && exp_uses_objfile (d->exp, objfile)))
> +      {
> +	xfree (d->exp);
> +	d->exp = NULL;
> +	d->block = NULL;
> +      }
>      }
>  }
>  \f
> --- a/gdb/scm-lang.c
> +++ b/gdb/scm-lang.c
> @@ -231,6 +231,7 @@ const struct exp_descriptor exp_descriptor_scm =
>  {
>    print_subexp_standard,
>    operator_length_standard,
> +  operator_check_standard,
>    op_name_standard,
>    dump_subexp_body_standard,
>    evaluate_exp
> --- a/gdb/solist.h
> +++ b/gdb/solist.h
> @@ -63,7 +63,12 @@ struct so_list
>  
>      bfd *abfd;
>      char symbols_loaded;	/* flag: symbols read in yet? */
> -    struct objfile *objfile;	/* objfile for loaded lib */
> +
> +    /* objfile with symbols for a loaded library.  Target memory is read from
> +       ABFD.  OBJFILE may be NULL either before symbols have been loaded, if
> +       the file cannot be found or after the command "nosharedlibrary".  */
> +    struct objfile *objfile;
> +
>      struct target_section *sections;
>      struct target_section *sections_end;
>  
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -1150,7 +1150,7 @@ lookup_symbol_aux_local (const char *name, const struct block *block,
>  
>  /* Look up OBJFILE to BLOCK.  */
>  
> -static struct objfile *
> +struct objfile *
>  lookup_objfile_from_block (const struct block *block)
>  {
>    struct objfile *obj;
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -148,7 +148,7 @@ struct general_symbol_info
>  
>    short section;
>  
> -  /* The section associated with this symbol. */
> +  /* The section associated with this symbol.  It can be NULL.  */
>  
>    struct obj_section *obj_section;
>  };
> @@ -1206,4 +1206,6 @@ int producer_is_realview (const char *producer);
>  void fixup_section (struct general_symbol_info *ginfo,
>  		    CORE_ADDR addr, struct objfile *objfile);
>  
> +struct objfile *lookup_objfile_from_block (const struct block *block);
> +
>  #endif /* !defined(SYMTAB_H) */
> --- a/gdb/testsuite/gdb.base/solib-display.exp
> +++ b/gdb/testsuite/gdb.base/solib-display.exp
> @@ -36,7 +36,7 @@ if { [skip_shlib_tests] || [is_remote target] } {
>  set libname "solib-display-lib"
>  set srcfile_lib ${srcdir}/${subdir}/${libname}.c
>  set binfile_lib ${objdir}/${subdir}/${libname}.so
> -set lib_flags [list debug]
> +set lib_flags {}
>  # Binary file.
>  set testfile "solib-display-main"
>  set srcfile ${srcdir}/${subdir}/${testfile}.c
> @@ -48,60 +48,92 @@ if [get_compiler_info ${binfile}] {
>      return -1
>  }
>  
> -if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $lib_flags] != ""
> -     || [gdb_compile ${srcfile} ${binfile} executable $bin_flags] != "" } {
> -  untested "Could not compile $binfile_lib or $binfile."
> -  return -1
> +set save_pf_prefix $pf_prefix
> +# SEP must be last for the possible `unsupported' error path.
> +foreach libsepdebug {NO IN SEP} {

Could you please add $libsepdebug or something of the sorts
to each test within the loop, so that when we analyise the logs we
can distinguish FAIL/PASSes for each iteration?

> +
> +    set pf_prefix $save_pf_prefix
> +    lappend pf_prefix "$libsepdebug:"
> +
> +    set sep_lib_flags $lib_flags
> +    if {$libsepdebug != "NO"} {
> +	lappend sep_lib_flags {debug}
> +    }
> +    if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $sep_lib_flags] != ""
> +	 || [gdb_compile ${srcfile} ${binfile} executable $bin_flags] != "" } {
> +      untested "Could not compile $binfile_lib or $binfile."
> +      return -1
> +    }
> +
> +    if {$libsepdebug == "SEP"} {
> +	if {[gdb_gnu_strip_debug $binfile_lib] != 0} {
> +	    unsupported "Could not split debug of $binfile_lib."
> +	    return
> +	} else {
> +	    pass "split solib"
> +	}
> +    }
> +
> +    clean_restart $executable
> +
> +    if ![runto_main] then {
> +      fail "Can't run to main"
> +      return 0
> +    }
> +
> +    gdb_test "display a_global" "1: a_global = 41"
> +    gdb_test "display b_global" "2: b_global = 42"
> +    gdb_test "display c_global" "3: c_global = 43"
> +
> +    if { [gdb_start_cmd] < 0 } {
> +	fail "Can't run to main (2)"
> +	continue
> +    }
> +
> +    gdb_test "" "3: c_global = 43\\r\\n2: b_global = 42\\r\\n1: a_global = 41" "after rerun"
> +
> +    # Now rebuild the library without b_global
> +    if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} \
> +	      "$sep_lib_flags additional_flags=-DNO_B_GLOBAL"] != ""} {
> +	fail "Can't rebuild $binfile_lib"
> +    }
> +
> +    if {$libsepdebug == "SEP"} {
> +	set test "split solib second time"
> +	if {[gdb_gnu_strip_debug $binfile_lib] != 0} {
> +	    fail $test
> +	    continue
> +	} else {
> +	    pass $test
> +	}
> +    }
> +
> +    if { [gdb_start_cmd] < 0 } {
> +	fail "Can't run to main (3)"
> +	continue
> +    }
> +
> +    gdb_test "" "3: c_global = 43\\r\\nwarning: .*b_global.*\\r\\n1: a_global = 41" "after rerun (2)"
> +
> +    # Now verify that displays which are not in the shared library
> +    # are not cleared permaturely.
> +
> +    gdb_test "break [gdb_get_line_number "break here" ${testfile}.c]" \
> +	    ".*Breakpoint.* at .*" 
> +
> +    gdb_test "continue"
> +    gdb_test "display main_global" "4: main_global = 44"
> +    gdb_test "display a_local" "5: a_local = 45"
> +    gdb_test "display a_static" "6: a_static = 46"
> +
> +    if { [gdb_start_cmd] < 0 } {
> +	fail "Can't run to main (4)"
> +	continue
> +    }
> +
> +    gdb_test "" "6: a_static = 46\\r\\n4: main_global = 44\\r\\n.*"
> +    gdb_test "break [gdb_get_line_number "break here" ${testfile}.c]" \
> +	    ".*Breakpoint.* at .*" 
> +    gdb_test "continue" "6: a_static = 46\\r\\n5: a_local = 45\\r\\n4: main_global = 44\\r\\n.*"
>  }
> -
> -clean_restart ${executable}
> -
> -if ![runto_main] then {
> -  fail "Can't run to main"
> -  return 0
> -}
> -
> -gdb_test "display a_global" "1: a_global = 41"
> -gdb_test "display b_global" "2: b_global = 42"
> -gdb_test "display c_global" "3: c_global = 43"
> -
> -if { [gdb_start_cmd] < 0 } {
> -    fail "Can't run to main (2)"
> -    return 0
> -}
> -
> -gdb_test "" "3: c_global = 43\\r\\n2: b_global = 42\\r\\n1: a_global = 41" "after rerun"
> -
> -# Now rebuild the library without b_global
> -if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} \
> -	  "$lib_flags additional_flags=-DNO_B_GLOBAL"] != ""} {
> -    fail "Can't rebuild $binfile_lib"
> -}
> -
> -if { [gdb_start_cmd] < 0 } {
> -    fail "Can't run to main (3)"
> -    return 0
> -}
> -
> -gdb_test "" "3: c_global = 43\\r\\nwarning: .*b_global.*\\r\\n1: a_global = 41" "after rerun"
> -
> -# Now verify that displays which are not in the shared library
> -# are not cleared permaturely.
> -
> -gdb_test "break [gdb_get_line_number "break here" ${testfile}.c]" \
> -	".*Breakpoint.* at .*" 
> -
> -gdb_test "continue"
> -gdb_test "display main_global" "4: main_global = 44"
> -gdb_test "display a_local" "5: a_local = 45"
> -gdb_test "display a_static" "6: a_static = 46"
> -
> -if { [gdb_start_cmd] < 0 } {
> -    fail "Can't run to main (4)"
> -    return 0
> -}
> -
> -gdb_test "" "6: a_static = 46\\r\\n4: main_global = 44\\r\\n.*"
> -gdb_test "break [gdb_get_line_number "break here" ${testfile}.c]" \
> -	".*Breakpoint.* at .*" 
> -gdb_test "continue" "6: a_static = 46\\r\\n5: a_local = 45\\r\\n4: main_global = 44\\r\\n.*"
> +set pf_prefix $save_pf_prefix
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -2896,6 +2896,9 @@ proc build_id_debug_filename_get { exec } {
>  # Create stripped files for DEST, replacing it.  If ARGS is passed, it is a
>  # list of optional flags.  The only currently supported flag is no-main,
>  # which removes the symbol entry for main from the separate debug file.
> +#
> +# Function returns zero on success.  Function will return non-zero failure code
> +# on some targets not supporting separate debug info (such as i386-msdos).
>  
>  proc gdb_gnu_strip_debug { dest args } {
>  
> 

Otherwise, looks good to me.

-- 
Pedro Alves

Re: [patch] Fix dangling displays in separate debug

[Jan Kratochvil]

On Tue, 20 Apr 2010 13:05:30 +0200, Pedro Alves wrote:
> On Monday 19 April 2010 15:25:03, Jan Kratochvil wrote:
> > With the extended requirements I rather pushed the former patch posted at:
> > 	Re: [patch 3/8] Types GC [display_uses_solib_p to exp_iterate]
> > 	http://sourceware.org/ml/gdb-patches/2009-07/msg00054.html
> 
> Yikes.  I don't understand what is the extended requirement, but anyway,

As you have requested to support both separate and embedded debug info forms
I "had to" include there the general stripping framework (written formerly for
break-interp.exp).  This meant I also "had to" keep there the "NO" case which
uses only ELF symbols.  ELF symbols opened a can of worms of more issues in
incomplete display_uses_solib_p which I was aware of from the patch a year ago
above.

OTOH hopefully the more complete fix gets in now that way.


> > +/* Helper for exp_uses_objfile.  */
> > +
> > +static int
> > +exp_uses_objfile_iter (struct objfile *exp_objfile, void *objfile_voidp)
> > +{
> > +  struct objfile *objfile = objfile_voidp;
> > +
> > +  if (exp_objfile->separate_debug_objfile_backlink)
> > +    exp_objfile = exp_objfile->separate_debug_objfile_backlink;
> > +
> > +  return exp_objfile == objfile;
> > +}
> 
> Most things look good.  This is get's a little blury though.  You have
> this nice iterator that can determine if a given expression involved an
> obfile, so, you could accurately now disable a "display" when an objfile
> is unloaded from gdb --- _any_ objfile, even in principly when you
> do "file" to unload the main exec's symbols, say, so that _all_ cases
> of dangling displays would be cured.

Yes, the OBJFILE based iterator was originally written with this intention.


> Still, you only call this when an solib is unloaded.
...
> Doesn't this just
> beg for a new `objfile_unloaded' observer (called from, say free_objfile)?

That patch
> > 	Re: [patch 3/8] Types GC [display_uses_solib_p to exp_iterate]
> > 	http://sourceware.org/ml/gdb-patches/2009-07/msg00054.html

was based on more general objfile callback rather than the current solib
callback, using the new observer from:
	[patch 1/8] Types GC [unloading observer]
	http://sourceware.org/ml/gdb-patches/2009-05/msg00544.html

But the patch series did not make it in for some reasons which may be
only/also on my side.  So trying to get it at least a part of the series now.
It can be incrementally completed later without much additional work.

Probability of getting a whole patch series in equals p^n where p is
probability for a single patch and n is the number of patches in the series.
For low p and high n it does not work well. :-)


> Can't we in principle be unloading a separate debug info objfile, but still
> have the main one loaded, hence OBJFILE_VOIDP could be a separate debug
> objfile?

I find the internal GDB API standardizes on the normalized base objfile (that
is following the separate_debug_objfile_backlink pointer if there is one).
I have seen this as a common practice at least in:
	lookup_objfile_from_block dwarf2_per_cu_objfile elf_lookup_lib_symbol
	etc.

Therefore OBJFILE_VOIDP cannot be the separate debug objfile as the value has
been verified as normalized via separate_debug_objfile_backlink already by
exp_uses_objfile and it was previously normalized via
separate_debug_objfile_backlink by clear_dangling_display_expressions.

It is already stated in the exp_uses_objfile comment.


> An expression could in principle be using symbols from either,

Yes, therefore exp_uses_objfile_iter normalizes EXP_OBJFILE.


> Anyway, I'm not going to require you do that for this patch, your patch
> looks good already.  Just dumping my thoughts, on what looks like an
> opportunity to keep the solib/objfile concepts cleanly separate, and get
> rid of all the objfile backlinking here.

That's true but I can remove the backlinking when the objfile unloading
observer gets checked in one day.


> > -  if (d->block != NULL
> > -      && d->pspace == solib->pspace
> > -      && solib_contains_address_p (solib, d->block->startaddr))
> > -    return 1;
> 
> Is there any `solib_contains_address_p' check left after the patch?
> What about displays that don't involve symbols, like:
> 
>  `display *0xaddr_in_solib'.
> 
> They'll probably still disable themselves when memory reading
> fails, but, will we now see an ugly error, or something?

It is unrelated.  d->block specifies scope of the expression.  Absolute
address dereference requires no scope and thus GDB sets d->block = NULL.
GDB tries to never use this INNERMOST_BLOCK if it does not need to.

It still prints:
Disabling display 1 to avoid infinite recursion.
1: *0x7ffff7ddd53c = Cannot access memory at address 0x7ffff7ddd53c

d->block is the function f for `display x' in:
void f (void)
{
  int x;
}

In that case I cannot imagine when solib_contains_address_p could return for f
TRUE while lookup_objfile_from_block would not match BLOCK with OBJFILE.


After/if one day Apple's "watch -location" gets accepted there may be a problem
	watch -location global_var_in_solib
may not catch unloading of the library as it will have neither BLOCK nor EXP
binding to that OBJFILE.  But "-location" is not present in current FSF GDB.

Apple "watch -location" should do something like:
	p/x &arg
	watch *(typeof (arg)) printed_address


> > -if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $lib_flags] != ""
> > -     || [gdb_compile ${srcfile} ${binfile} executable $bin_flags] != "" } {
> > -  untested "Could not compile $binfile_lib or $binfile."
> > -  return -1
> > +set save_pf_prefix $pf_prefix
> > +# SEP must be last for the possible `unsupported' error path.
> > +foreach libsepdebug {NO IN SEP} {
> 
> Could you please add $libsepdebug or something of the sorts
> to each test within the loop, so that when we analyise the logs we
> can distinguish FAIL/PASSes for each iteration?

It already prints unique test names using $pf_prefix there:

Running ./gdb.base/solib-display.exp ...
PASS: gdb.base/solib-display.exp: NO: display a_global
PASS: gdb.base/solib-display.exp: NO: display b_global
PASS: gdb.base/solib-display.exp: NO: display c_global
PASS: gdb.base/solib-display.exp: NO: after rerun
PASS: gdb.base/solib-display.exp: NO: after rerun (2)
PASS: gdb.base/solib-display.exp: NO: break 25
PASS: gdb.base/solib-display.exp: NO: continue
PASS: gdb.base/solib-display.exp: NO: display main_global
PASS: gdb.base/solib-display.exp: NO: display a_local
PASS: gdb.base/solib-display.exp: NO: display a_static
PASS: gdb.base/solib-display.exp: NO: break 25
PASS: gdb.base/solib-display.exp: NO: continue
PASS: gdb.base/solib-display.exp: IN: display a_global
...
PASS: gdb.base/solib-display.exp: IN: continue
PASS: gdb.base/solib-display.exp: SEP: split solib
PASS: gdb.base/solib-display.exp: SEP: display a_global
...
PASS: gdb.base/solib-display.exp: SEP: continue

I hope it is OK this way and you just missed the pf_prefix updates there.



> Otherwise, looks good to me.

Please advice if the comments are OK or the patch needs some updates.


Thanks,
Jan

Re: [patch] Fix dangling displays in separate debug

[Pedro Alves]

On Thursday 22 April 2010 23:30:23, Jan Kratochvil wrote:
> > Otherwise, looks good to me.
> 
> Please advice if the comments are OK or the patch needs some updates.

Thanks for the clarifications.  The patch is OK.

-- 
Pedro Alves

Re: [patch] Fix dangling displays in separate debug

[Jan Kratochvil]

On Fri, 23 Apr 2010 00:52:36 +0200, Pedro Alves wrote:
> Thanks for the clarifications.  The patch is OK.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2010-04/msg00228.html


Thanks,
Jan