[RFC] choose symbol from given block's objfile first.

[RFC] choose symbol from given block's objfile first.

[Joel Brobecker]

Hello,

This patch is a prototype for an issue very briefly discussed on IRC.
Quick description of the problem:

    We have a program which is linked against two shared libraries.
    Both libraries define a global symbol with the same name. In my
    example, I used an int called this_library_version.  When trying
    to print the value of this global, GDB just randomly selects
    the first one it finds, and I couldn't find a way (that worked)
    which allowed me to print the value of the other global.

The idea behind this patch is to reduce a little bit the randomness.
If one is debugging code inside one of the shared libraries, then
the variable that the user probably wants is the one that is defined
inside that shared library.

It's a bit of a poor man's answer to this issue. On the one hand,
you do not always get the same symbol every single time. But on
the other hand, the selection process is implicit and not always
work-able for the user.  Eventually, what we thought we needed was
extend the expression parser to allow the user to qualify his
variable name with the name of the objfile, such as for instance:

        (gdb) print libsomething.so::this_library_version

This is something that can be done in parallel to this effort.
But I also believe that this is a useful patch, because I think
that the answer yielded by GDB using this algorithm has more chances
of returning the symbol that the user actually meant. A typical
scenario is that he's debugging code in some DSO, and then sees that
his code is referencing some variable which just happens to be
a global variable, and that the same variable is defined elsewhere.
Trying to figure out what's going on, he naively does:

        (gdb) print this_library_version

With this patch, he'll get the variable whose value corresponds to
his current context. Without this patch, and with enough bad luck,
the user gets an unrelated value, and gets confused because the
program does not behave as expected based on the global's value
printed by GDB.

This is only a prototype at the moment. It fixes the problem while
not causing any regression with the testsuite.  I am wondering about
the loop over all objfiles callling lookup_symbol_aux_quick. I think
I would need to apply the same treatment, in case
lookup_symbol_aux_symtabs doesn't return any match.  But I'm not sure
why I'd need that...

I also have a feeling that these are not the only locations where
we'll need to make this sort of change if we want to make this type
of behavior consistent across all our searches. Perhaps it make
make sense to introduce a new macro that iterates over all objfiles
starting with a specific objfile first. I can't propose a name for
that macro though...

        ALL_OBJFILES_STARTING_WITH

? Long-ish, but the best I've got so far. I thought about
ALL_OBJFILES_FROM, but I think the name is misleading. Another
option is to just extend ALL_OBJFILES to accept an objfile as
a second argument. But I think we'd be impacting too much code
just for a handle of places that might need it...


gdb/ChangeLog:

        * findvar.c (default_read_var_value): For LOC_UNRESOLVED symbols,
        try locating the symbol in the symbol's own objfile first, before
        extending the search to all objfiles.
        * symtab.c (lookup_symbol_aux_objfile): New function, extracted
        out of lookup_symbol_aux_symtabs.
        (lookup_symbol_aux_symtabs): Add new parameter "context_objfile".
        Replace extracted-out code by call to lookup_symbol_aux_objfile.
        if CONTEXT_OBJFILE is not NULL, then search this objfile first,
        before searching the other objfiles.

Thoughts?

Thank you!
-- 
Joel

---
 gdb/findvar.c |   10 ++++++-
 gdb/symtab.c  |   90 ++++++++++++++++++++++++++++++++++++++------------------
 2 files changed, 70 insertions(+), 30 deletions(-)

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 9009e6f..ed7903c 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -562,7 +562,15 @@ default_read_var_value (struct symbol *var, struct frame_info *frame)
 	struct minimal_symbol *msym;
 	struct obj_section *obj_section;
 
-	msym = lookup_minimal_symbol (SYMBOL_LINKAGE_NAME (var), NULL, NULL);
+	/* First, try locating the associated minimal symbol within
+	   the same objfile.  This prevents us from selecting another
+	   symbol with the same name but located in a different objfile.  */
+	msym = lookup_minimal_symbol (SYMBOL_LINKAGE_NAME (var), NULL,
+				      SYMBOL_SYMTAB (var)->objfile);
+	/* If the lookup failed, try expanding the search to all
+	   objfiles.  */
+	if (msym == NULL)
+	  msym = lookup_minimal_symbol (SYMBOL_LINKAGE_NAME (var), NULL, NULL);
 	if (msym == NULL)
 	  error (_("No global symbol \"%s\"."), SYMBOL_LINKAGE_NAME (var));
 	if (overlay_debugging)
diff --git a/gdb/symtab.c b/gdb/symtab.c
index d68e542..8542beb 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -95,7 +95,8 @@ struct symbol *lookup_symbol_aux_local (const char *name,
 static
 struct symbol *lookup_symbol_aux_symtabs (int block_index,
 					  const char *name,
-					  const domain_enum domain);
+					  const domain_enum domain,
+					  struct objfile *context_objfile);
 
 static
 struct symbol *lookup_symbol_aux_quick (struct objfile *objfile,
@@ -1361,7 +1362,7 @@ lookup_static_symbol_aux (const char *name, const domain_enum domain)
   struct objfile *objfile;
   struct symbol *sym;
 
-  sym = lookup_symbol_aux_symtabs (STATIC_BLOCK, name, domain);
+  sym = lookup_symbol_aux_symtabs (STATIC_BLOCK, name, domain, NULL);
   if (sym != NULL)
     return sym;
 
@@ -1500,40 +1501,70 @@ lookup_global_symbol_from_objfile (const struct objfile *main_objfile,
   return NULL;
 }
 
-/* Check to see if the symbol is defined in one of the symtabs.
-   BLOCK_INDEX should be either GLOBAL_BLOCK or STATIC_BLOCK,
+/* Check to see if the symbol is defined in one of the OBJFILE's
+   symtabs.  BLOCK_INDEX should be either GLOBAL_BLOCK or STATIC_BLOCK,
    depending on whether or not we want to search global symbols or
    static symbols.  */
 
 static struct symbol *
-lookup_symbol_aux_symtabs (int block_index, const char *name,
-			   const domain_enum domain)
+lookup_symbol_aux_objfile (struct objfile *objfile, int block_index,
+			   const char *name, const domain_enum domain)
 {
-  struct symbol *sym;
-  struct objfile *objfile;
+  struct symbol *sym = NULL;
   struct blockvector *bv;
   const struct block *block;
   struct symtab *s;
 
+  if (objfile->sf)
+    objfile->sf->qf->pre_expand_symtabs_matching (objfile, block_index,
+						  name, domain);
+
+  ALL_OBJFILE_SYMTABS (objfile, s)
+    if (s->primary)
+      {
+	bv = BLOCKVECTOR (s);
+	block = BLOCKVECTOR_BLOCK (bv, block_index);
+	sym = lookup_block_symbol (block, name, domain);
+	if (sym)
+	  {
+	    block_found = block;
+	    return fixup_symbol_section (sym, objfile);
+	  }
+      }
+
+  return NULL;
+}
+
+/* Same as lookup_symbol_aux_objfile, except that it searches all
+   objfiles, stating with CONTEXT_OBJFILE first.  Return the first
+   match found.
+
+   If CONTEXT_OBJFILE is NULL, then start the search with any objfile.  */
+
+static struct symbol *
+lookup_symbol_aux_symtabs (int block_index, const char *name,
+			   const domain_enum domain,
+			   struct objfile *context_objfile)
+{
+  struct symbol *sym;
+  struct objfile *objfile;
+
+  if (context_objfile != NULL)
+    {
+      sym = lookup_symbol_aux_objfile (context_objfile, block_index,
+				       name, domain);
+      if (sym)
+	return sym;
+    }
+
   ALL_OBJFILES (objfile)
   {
-    if (objfile->sf)
-      objfile->sf->qf->pre_expand_symtabs_matching (objfile,
-						    block_index,
-						    name, domain);
-
-    ALL_OBJFILE_SYMTABS (objfile, s)
-      if (s->primary)
-	{
-	  bv = BLOCKVECTOR (s);
-	  block = BLOCKVECTOR_BLOCK (bv, block_index);
-	  sym = lookup_block_symbol (block, name, domain);
-	  if (sym)
-	    {
-	      block_found = block;
-	      return fixup_symbol_section (sym, objfile);
-	    }
-	}
+    if (objfile != context_objfile)
+      {
+	sym = lookup_symbol_aux_objfile (objfile, block_index, name, domain);
+	if (sym)
+	  return sym;
+      }
   }
 
   return NULL;
@@ -1659,16 +1690,17 @@ lookup_symbol_global (const char *name,
 		      const domain_enum domain)
 {
   struct symbol *sym = NULL;
+  struct objfile *block_objfile = NULL;
   struct objfile *objfile = NULL;
 
   /* Call library-specific lookup procedure.  */
-  objfile = lookup_objfile_from_block (block);
-  if (objfile != NULL)
-    sym = solib_global_lookup (objfile, name, domain);
+  block_objfile = lookup_objfile_from_block (block);
+  if (block_objfile != NULL)
+    sym = solib_global_lookup (block_objfile, name, domain);
   if (sym != NULL)
     return sym;
 
-  sym = lookup_symbol_aux_symtabs (GLOBAL_BLOCK, name, domain);
+  sym = lookup_symbol_aux_symtabs (GLOBAL_BLOCK, name, domain, block_objfile);
   if (sym != NULL)
     return sym;
 
-- 
1.7.1

Re: [RFC] choose symbol from given block's objfile first.

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> But I also believe that this is a useful patch, because I think
Joel> that the answer yielded by GDB using this algorithm has more chances
Joel> of returning the symbol that the user actually meant.

I agree.  This change would make gdb's behavior less surprising in some
corner cases, and I think it is hard to argue that people know or rely
on gdb's current behavior.

Joel> This is only a prototype at the moment. It fixes the problem while
Joel> not causing any regression with the testsuite.  I am wondering about
Joel> the loop over all objfiles callling lookup_symbol_aux_quick. I think
Joel> I would need to apply the same treatment, in case
Joel> lookup_symbol_aux_symtabs doesn't return any match.  But I'm not sure
Joel> why I'd need that...

Yes, I think you'd need this change there as well.

gdb should end up in the lookup_symbol_aux_quick loop if the symbol in
question is in an unexpanded psymtab.  In this case you want to be sure
to check the block objfile first.

Joel>         ALL_OBJFILES_STARTING_WITH

Works for me.

Tom

[RFA] choose symbol from given block's objfile first.

[Joel Brobecker]

[about needing to update the calls to lookup_symbol_aux_quick]
> Yes, I think you'd need this change there as well.
> 
> gdb should end up in the lookup_symbol_aux_quick loop if the symbol in
> question is in an unexpanded psymtab.  In this case you want to be sure
> to check the block objfile first.

I think I see why I got tricked: Is it because the debug info is DWARF?
With DWARF, the lookup_static_symbol_aux first pre-expands all partial
symbols containing a match, so we never go into the quick method...

Attached are two patches, the first being the code change, and the
second a testcase for it.

gdb/ChangeLog:

        * findvar.c (default_read_var_value): For LOC_UNRESOLVED symbols,
        try locating the symbol in the symbol's own objfile first, before
        extending the search to all objfiles.
        * symtab.c (lookup_symbol_aux_objfile): New function, extracted
        out of lookup_symbol_aux_symtabs.
        (lookup_symbol_aux_symtabs): Add new parameter "exclude_objfile".
        Replace extracted-out code by call to lookup_symbol_aux_objfile.
        Do not search EXCLUDE_OBJFILE.
        (lookup_static_symbol_aux): Update call to lookup_symbol_aux_symtabs.
        (lookup_symbol_global): Search for matches in the block's objfile
        first, before searching all other objfiles.

gdb/testsuite/ChangeLog:

        * gdb.base/ctxobj-f.c, gdb.base/ctxobj-m.c, gdb.base/ctxobj-v.c,
        gdb.base/ctxobj.exp: New files.

Tested on x86_64-linux.  No regression.

OK to commit?
-- 
Joel

Re: [RFA] choose symbol from given block's objfile first.

[Joel Brobecker]

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

> Attached are two patches, the first being the code change, and the
> second a testcase for it.
> 
> gdb/ChangeLog:
> 
>         * findvar.c (default_read_var_value): For LOC_UNRESOLVED symbols,
>         try locating the symbol in the symbol's own objfile first, before
>         extending the search to all objfiles.
>         * symtab.c (lookup_symbol_aux_objfile): New function, extracted
>         out of lookup_symbol_aux_symtabs.
>         (lookup_symbol_aux_symtabs): Add new parameter "exclude_objfile".
>         Replace extracted-out code by call to lookup_symbol_aux_objfile.
>         Do not search EXCLUDE_OBJFILE.
>         (lookup_static_symbol_aux): Update call to lookup_symbol_aux_symtabs.
>         (lookup_symbol_global): Search for matches in the block's objfile
>         first, before searching all other objfiles.
> 
> gdb/testsuite/ChangeLog:
> 
>         * gdb.base/ctxobj-f.c, gdb.base/ctxobj-m.c, gdb.base/ctxobj-v.c,
>         gdb.base/ctxobj.exp: New files.
> 
> Tested on x86_64-linux.  No regression.

ENOPATCH.

-- 
Joel

[-- Attachment #2: 0001-Search-global-symbols-from-the-expression-s-block-ob.patch --]
[-- Type: text/x-diff, Size: 6962 bytes --]

From 60b3578a23eef7b06e0a2e6a38191a2bcb09651e Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Mon, 7 May 2012 14:43:28 -0700
Subject: [PATCH 1/2] Search global symbols from the expression's block objfile first.

gdb/ChangeLog:

        * findvar.c (default_read_var_value): For LOC_UNRESOLVED symbols,
        try locating the symbol in the symbol's own objfile first, before
        extending the search to all objfiles.
        * symtab.c (lookup_symbol_aux_objfile): New function, extracted
        out of lookup_symbol_aux_symtabs.
        (lookup_symbol_aux_symtabs): Add new parameter "exclude_objfile".
        Replace extracted-out code by call to lookup_symbol_aux_objfile.
        Do not search EXCLUDE_OBJFILE.
        (lookup_static_symbol_aux): Update call to lookup_symbol_aux_symtabs.
        (lookup_symbol_global): Search for matches in the block's objfile
        first, before searching all other objfiles.
---
 gdb/findvar.c |   10 +++++-
 gdb/symtab.c  |  108 ++++++++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 85 insertions(+), 33 deletions(-)

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 9009e6f..ed7903c 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -562,7 +562,15 @@ default_read_var_value (struct symbol *var, struct frame_info *frame)
 	struct minimal_symbol *msym;
 	struct obj_section *obj_section;
 
-	msym = lookup_minimal_symbol (SYMBOL_LINKAGE_NAME (var), NULL, NULL);
+	/* First, try locating the associated minimal symbol within
+	   the same objfile.  This prevents us from selecting another
+	   symbol with the same name but located in a different objfile.  */
+	msym = lookup_minimal_symbol (SYMBOL_LINKAGE_NAME (var), NULL,
+				      SYMBOL_SYMTAB (var)->objfile);
+	/* If the lookup failed, try expanding the search to all
+	   objfiles.  */
+	if (msym == NULL)
+	  msym = lookup_minimal_symbol (SYMBOL_LINKAGE_NAME (var), NULL, NULL);
 	if (msym == NULL)
 	  error (_("No global symbol \"%s\"."), SYMBOL_LINKAGE_NAME (var));
 	if (overlay_debugging)
diff --git a/gdb/symtab.c b/gdb/symtab.c
index d68e542..3fce349 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -95,7 +95,8 @@ struct symbol *lookup_symbol_aux_local (const char *name,
 static
 struct symbol *lookup_symbol_aux_symtabs (int block_index,
 					  const char *name,
-					  const domain_enum domain);
+					  const domain_enum domain,
+					  struct objfile *exclude_objfile);
 
 static
 struct symbol *lookup_symbol_aux_quick (struct objfile *objfile,
@@ -1361,7 +1362,7 @@ lookup_static_symbol_aux (const char *name, const domain_enum domain)
   struct objfile *objfile;
   struct symbol *sym;
 
-  sym = lookup_symbol_aux_symtabs (STATIC_BLOCK, name, domain);
+  sym = lookup_symbol_aux_symtabs (STATIC_BLOCK, name, domain, NULL);
   if (sym != NULL)
     return sym;
 
@@ -1500,40 +1501,61 @@ lookup_global_symbol_from_objfile (const struct objfile *main_objfile,
   return NULL;
 }
 
-/* Check to see if the symbol is defined in one of the symtabs.
-   BLOCK_INDEX should be either GLOBAL_BLOCK or STATIC_BLOCK,
+/* Check to see if the symbol is defined in one of the OBJFILE's
+   symtabs.  BLOCK_INDEX should be either GLOBAL_BLOCK or STATIC_BLOCK,
    depending on whether or not we want to search global symbols or
    static symbols.  */
 
 static struct symbol *
-lookup_symbol_aux_symtabs (int block_index, const char *name,
-			   const domain_enum domain)
+lookup_symbol_aux_objfile (struct objfile *objfile, int block_index,
+			   const char *name, const domain_enum domain)
 {
-  struct symbol *sym;
-  struct objfile *objfile;
+  struct symbol *sym = NULL;
   struct blockvector *bv;
   const struct block *block;
   struct symtab *s;
 
+  if (objfile->sf)
+    objfile->sf->qf->pre_expand_symtabs_matching (objfile, block_index,
+						  name, domain);
+
+  ALL_OBJFILE_SYMTABS (objfile, s)
+    if (s->primary)
+      {
+	bv = BLOCKVECTOR (s);
+	block = BLOCKVECTOR_BLOCK (bv, block_index);
+	sym = lookup_block_symbol (block, name, domain);
+	if (sym)
+	  {
+	    block_found = block;
+	    return fixup_symbol_section (sym, objfile);
+	  }
+      }
+
+  return NULL;
+}
+
+/* Same as lookup_symbol_aux_objfile, except that it searches all
+   objfiles except for EXCLUDE_OBJFILE.  Return the first match found.
+
+   If EXCLUDE_OBJFILE is NULL, then all objfiles are searched.  */
+
+static struct symbol *
+lookup_symbol_aux_symtabs (int block_index, const char *name,
+			   const domain_enum domain,
+			   struct objfile *exclude_objfile)
+{
+  struct symbol *sym;
+  struct objfile *objfile;
+
   ALL_OBJFILES (objfile)
   {
-    if (objfile->sf)
-      objfile->sf->qf->pre_expand_symtabs_matching (objfile,
-						    block_index,
-						    name, domain);
-
-    ALL_OBJFILE_SYMTABS (objfile, s)
-      if (s->primary)
-	{
-	  bv = BLOCKVECTOR (s);
-	  block = BLOCKVECTOR_BLOCK (bv, block_index);
-	  sym = lookup_block_symbol (block, name, domain);
-	  if (sym)
-	    {
-	      block_found = block;
-	      return fixup_symbol_section (sym, objfile);
-	    }
-	}
+    if (objfile != exclude_objfile)
+      {
+	sym = lookup_symbol_aux_objfile (objfile, block_index, name, domain);
+	if (sym)
+	  return sym;
+      }
   }
 
   return NULL;
@@ -1659,24 +1681,46 @@ lookup_symbol_global (const char *name,
 		      const domain_enum domain)
 {
   struct symbol *sym = NULL;
+  struct objfile *block_objfile = NULL;
   struct objfile *objfile = NULL;
 
   /* Call library-specific lookup procedure.  */
-  objfile = lookup_objfile_from_block (block);
-  if (objfile != NULL)
-    sym = solib_global_lookup (objfile, name, domain);
+  block_objfile = lookup_objfile_from_block (block);
+  if (block_objfile != NULL)
+    sym = solib_global_lookup (block_objfile, name, domain);
   if (sym != NULL)
     return sym;
 
-  sym = lookup_symbol_aux_symtabs (GLOBAL_BLOCK, name, domain);
+  /* If BLOCK_OBJFILE is not NULL, then search this objfile first.
+     In case the global symbol is defined in multiple objfiles,
+     we have a better chance of finding the most relevant symbol.  */
+
+  if (block_objfile != NULL)
+    {
+      sym = lookup_symbol_aux_objfile (block_objfile, GLOBAL_BLOCK,
+				       name, domain);
+      if (sym == NULL)
+	sym = lookup_symbol_aux_quick (block_objfile, GLOBAL_BLOCK,
+				       name, domain);
+      if (sym != NULL)
+	return sym;
+    }
+
+  /* Symbol not found in the BLOCK_OBJFILE, so try all the other
+     objfiles, starting with symtabs first, and then partial symtabs.  */
+
+  sym = lookup_symbol_aux_symtabs (GLOBAL_BLOCK, name, domain, block_objfile);
   if (sym != NULL)
     return sym;
 
   ALL_OBJFILES (objfile)
   {
-    sym = lookup_symbol_aux_quick (objfile, GLOBAL_BLOCK, name, domain);
-    if (sym)
-      return sym;
+    if (objfile != block_objfile)
+      {
+	sym = lookup_symbol_aux_quick (objfile, GLOBAL_BLOCK, name, domain);
+	if (sym)
+	  return sym;
+      }
   }
 
   return NULL;
-- 
1.7.1


[-- Attachment #3: 0002-New-testcase-gdb.base-ctxobj.exp.patch --]
[-- Type: text/x-diff, Size: 7938 bytes --]

From c3b79f2dfd48d1052d012ce1c390fdb271bad2dc Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Wed, 9 May 2012 10:35:31 -0700
Subject: [PATCH 2/2] New testcase: gdb.base/ctxobj.exp

gdb/testsuite/ChangeLog:

        * gdb.base/ctxobj-f.c, gdb.base/ctxobj-m.c, gdb.base/ctxobj-v.c,
        gdb.base/ctxobj.exp: New files.
---
 gdb/testsuite/gdb.base/ctxobj-f.c |   27 +++++++++++
 gdb/testsuite/gdb.base/ctxobj-m.c |   31 ++++++++++++
 gdb/testsuite/gdb.base/ctxobj-v.c |   21 ++++++++
 gdb/testsuite/gdb.base/ctxobj.exp |   94 +++++++++++++++++++++++++++++++++++++
 4 files changed, 173 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/ctxobj-f.c
 create mode 100644 gdb/testsuite/gdb.base/ctxobj-m.c
 create mode 100644 gdb/testsuite/gdb.base/ctxobj-v.c
 create mode 100644 gdb/testsuite/gdb.base/ctxobj.exp

diff --git a/gdb/testsuite/gdb.base/ctxobj-f.c b/gdb/testsuite/gdb.base/ctxobj-f.c
new file mode 100644
index 0000000..43cc328
--- /dev/null
+++ b/gdb/testsuite/gdb.base/ctxobj-f.c
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+   Copyright 2012 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+extern int this_version_num;
+
+#ifndef GET_VERSION
+#error GET_VERSION macro is undefined
+#endif
+
+int
+GET_VERSION (void)
+{
+  return this_version_num;
+}
diff --git a/gdb/testsuite/gdb.base/ctxobj-m.c b/gdb/testsuite/gdb.base/ctxobj-m.c
new file mode 100644
index 0000000..203f838
--- /dev/null
+++ b/gdb/testsuite/gdb.base/ctxobj-m.c
@@ -0,0 +1,31 @@
+/* This testcase is part of GDB, the GNU debugger.
+   Copyright 2012 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+extern int get_version_1 (void);
+extern int get_version_2 (void);
+
+int
+main (void)
+{
+  if (get_version_1 () != 104)
+    return 1;
+
+  if (get_version_2 () != 203)
+    return 2;
+
+  return 0;
+}
+
diff --git a/gdb/testsuite/gdb.base/ctxobj-v.c b/gdb/testsuite/gdb.base/ctxobj-v.c
new file mode 100644
index 0000000..6d3a8d0
--- /dev/null
+++ b/gdb/testsuite/gdb.base/ctxobj-v.c
@@ -0,0 +1,21 @@
+/* This testcase is part of GDB, the GNU debugger.
+   Copyright 2012 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef VERSION
+#error VERSION macro is not defined.
+#endif
+
+int this_version_num = VERSION;
diff --git a/gdb/testsuite/gdb.base/ctxobj.exp b/gdb/testsuite/gdb.base/ctxobj.exp
new file mode 100644
index 0000000..57ed4c1
--- /dev/null
+++ b/gdb/testsuite/gdb.base/ctxobj.exp
@@ -0,0 +1,94 @@
+# Copyright 2012 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+set executable ctxobj-m
+
+# The sources used to build two shared libraries (SO).  We use the exact
+# same sources to build both SOs, but differentiate them through the use
+# of macros defined when calling the compiler.
+#
+# We need two source files per SO, because we need to test the situation
+# where we are trying to print the value of a global variable defined
+# in that SO while the variable's associated symtab has not been created
+# yet.
+set libsrc [list "${srcdir}/${subdir}/ctxobj-v.c" \
+                 "${srcdir}/${subdir}/ctxobj-f.c"]
+
+set libobj1 "${objdir}/${subdir}/libctxobj1.so"
+set libobj2 "${objdir}/${subdir}/libctxobj2.so"
+
+set libobj1_opts { debug additional_flags=-fPIC
+                   additional_flags=-DVERSION=104
+                   additional_flags=-DGET_VERSION=get_version_1 }
+set libobj2_opts { debug additional_flags=-fPIC
+                   additional_flags=-DVERSION=203
+                   additional_flags=-DGET_VERSION=get_version_2 }
+
+if { [gdb_compile_shlib $libsrc $libobj1 $libobj1_opts ] != "" } {
+    return -1
+}
+if { [gdb_compile_shlib $libsrc $libobj2 $libobj2_opts ] != "" } {
+    return -1
+}
+if { [gdb_compile "${srcdir}/${subdir}/${executable}.c" \
+                  "${objdir}/${subdir}/${executable}" \
+                  executable \
+                  [list debug shlib=${libobj1} shlib=${libobj2}]]
+     != ""} {
+    return -1
+}
+
+clean_restart $executable
+
+if ![runto_main] {
+    untested "could not run to main"
+    return -1
+}
+
+gdb_breakpoint "get_version_1"
+gdb_test "continue" \
+         "Breakpoint $decimal, get_version_1 \\(\\).*" \
+         "continue to get_version_1"
+
+# Try printing "this_version_num".  There are two global variables
+# with that name, but we should pick the one in the shared library
+# we are currently debugging.  We will know we picked the correct one
+# if the value printed is 104.  The first print test verifies that
+# we're doing things right when the partial symtab hasn't been
+# expanded.  And the second print test will do the same, but after
+# the partial symtab has been expanded.
+
+gdb_test "print this_version_num" \
+         " = 104" \
+        "print libctxobj1's this_version_num from partial symtab"
+
+gdb_test "print this_version_num" \
+         " = 104" \
+        "print libctxobj1's this_version_num from symtab"
+
+# Do the same, but from get_version_2.
+
+gdb_breakpoint "get_version_2"
+gdb_test "continue" \
+         "Breakpoint $decimal, get_version_2 \\(\\).*" \
+         "continue to get_version_2"
+
+gdb_test "print this_version_num" \
+         " = 203" \
+        "print libctxobj2's this_version_num from partial symtab"
+
+gdb_test "print this_version_num" \
+         " = 203" \
+        "print libctxobj2's this_version_num from symtab"
-- 
1.7.1

Re: [RFA] choose symbol from given block's objfile first.

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> I think I see why I got tricked: Is it because the debug info is DWARF?
Joel> With DWARF, the lookup_static_symbol_aux first pre-expands all partial
Joel> symbols containing a match, so we never go into the quick method...

Not DWARF, but it depends on whether you are using psymtabs or the
.gdb_index.

There are two methods in quick_symbol_functions that can be supplied by
a symbol reader: pre_expand_symtabs_matching and lookup_symbol.
Psymtabs supplies the latter, the index code supplies the former.

Tom

Re: [RFA] choose symbol from given block's objfile first.

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> Attached are two patches, the first being the code change, and the
Joel> second a testcase for it.

It looks reasonable to me, but I wonder whether it is complete.
That is -- there are a lot of uses of ALL_OBJFILES; I wonder whether any
of them need updating.

Tom

Re: [RFA] choose symbol from given block's objfile first.

[Joel Brobecker]

> It looks reasonable to me, but I wonder whether it is complete.
> That is -- there are a lot of uses of ALL_OBJFILES; I wonder whether any
> of them need updating.

I did a second audit, and found that a lot of those uses were for
purposes other than symbol lookup (finding the symtab associated
a PC, for instance), so an update shouldn't be needed there.

There are a couple of places where the loop is used to find symbols,
but collects all matching symbols instead of the first one.

There are also some locations where the loop is inside a routine
that does a lookup without "context".  For instance,
basic_lookup_transparent_type.  In that case, the only real caller
I could find was check_typedef, which I don't think we want to
change.

Then, there are the symtab iterators, which we might want to
update in order to allow a different iteration order.  But I think
it'll make better sense in that case to do what I just did, and
check the priority objfile first, and then call the iterator.
Following that thread, one can see that the callers of "lookup_symtab"
might want to ask that a particular objfile be checked first.
But in the end, I decided to stop looking, because the call tree
quickly explodes...

-- 
Joel

Re: [RFA] choose symbol from given block's objfile first.

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> I did a second audit, and found that a lot of those uses were for
Joel> purposes other than symbol lookup (finding the symtab associated
Joel> a PC, for instance), so an update shouldn't be needed there.
[...]

Sounds good.  Thanks for looking.

Joel> There are also some locations where the loop is inside a routine
Joel> that does a lookup without "context".  For instance,
Joel> basic_lookup_transparent_type.  In that case, the only real caller
Joel> I could find was check_typedef, which I don't think we want to
Joel> change.

I'm not so sure.  It seems like you could make a multi-objfile test case
where an incomplete type is incorrectly resolved to a type in another
objfile.

Tom

Re: [RFA] choose symbol from given block's objfile first.

[Joel Brobecker]

> Joel> There are also some locations where the loop is inside a routine
> Joel> that does a lookup without "context".  For instance,
> Joel> basic_lookup_transparent_type.  In that case, the only real caller
> Joel> I could find was check_typedef, which I don't think we want to
> Joel> change.
> 
> I'm not so sure.  It seems like you could make a multi-objfile test case
> where an incomplete type is incorrectly resolved to a type in another
> objfile.

Yeah, I can see now that it should be relatively doable (using opaque
types).  I'll keep that in mind for a rainy day. I still have to work
on enhancing the expresion parser for both C and Ada, as well as that PR
that I need to create... If I have a chance today, I'll try to create
a testcase that breaks check_typedef.

(someone is not looking forward to updating all calls to check_typedef :-))

So, just to be sure: Are we OK with this iteration for now?

-- 
Joel

Re: [RFA] choose symbol from given block's objfile first.

[Joel Brobecker]

> > Attached are two patches, the first being the code change, and the
> > second a testcase for it.

And here is a second testcase that gets fixed by this patch as well.
I suspect the reason why my patch fixes it is because the variables
in the shared libraries must be LOC_UNRESOLVED, and thus resolved
through default_read_var_value/lookup_minimal_symbol.

gdb/testsuite/ChangeLog:

    * gdb.base/print-file-var-lib1.c, gdb.base/print-file-var-lib2.c,
    gdb.base/print-file-var-main.c, gdb.base/print-file-var.exp:
    New files.

gdb/ChangeLog:

        * config/djgpp/fnchange.lst: Add entries for print-file-var-lib1.c,
        print-file-var-lib2.c, print-file-var-main.c and
        print-file-var.exp (located in gdb/testsuite/gdb.base).

Tested on x86_64-linux.
-- 
Joel

Re: [RFA] choose symbol from given block's objfile first.

[Joel Brobecker]

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

[grumble grumble]

And here is a second testcase that gets fixed by this patch as well.
I suspect the reason why my patch fixes it is because the variables
in the shared libraries must be LOC_UNRESOLVED, and thus resolved
through default_read_var_value/lookup_minimal_symbol.

gdb/testsuite/ChangeLog:

    * gdb.base/print-file-var-lib1.c, gdb.base/print-file-var-lib2.c,
    gdb.base/print-file-var-main.c, gdb.base/print-file-var.exp:
    New files.

gdb/ChangeLog:

        * config/djgpp/fnchange.lst: Add entries for print-file-var-lib1.c,
        print-file-var-lib2.c, print-file-var-main.c and
        print-file-var.exp (located in gdb/testsuite/gdb.base).

Tested on x86_64-linux.

-- 
Joel

[-- Attachment #2: 0003-Add-print-file-var-testcase-with-two-libs-defining-t.patch --]
[-- Type: text/x-diff, Size: 7740 bytes --]

From ea60097df50e222f0ed7629df3fa5b4155d15cf0 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Wed, 9 May 2012 14:41:13 -0700
Subject: [PATCH] Add print 'file'::var testcase with two libs defining the same global variable

gdb/testsuite/ChangeLog:

	* gdb.base/print-file-var-lib1.c, gdb.base/print-file-var-lib2.c,
	gdb.base/print-file-var-main.c, gdb.base/print-file-var.exp:
	New files.

gdb/ChangeLog:

        * config/djgpp/fnchange.lst: Add entries for print-file-var-lib1.c,
        print-file-var-lib2.c, print-file-var-main.c and
        print-file-var.exp (located in gdb/testsuite/gdb.base).
---
 gdb/config/djgpp/fnchange.lst                |    4 ++
 gdb/testsuite/gdb.base/print-file-var-lib1.c |   23 +++++++++++
 gdb/testsuite/gdb.base/print-file-var-lib2.c |   23 +++++++++++
 gdb/testsuite/gdb.base/print-file-var-main.c |   29 +++++++++++++
 gdb/testsuite/gdb.base/print-file-var.exp    |   55 ++++++++++++++++++++++++++
 5 files changed, 134 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/print-file-var-lib1.c
 create mode 100644 gdb/testsuite/gdb.base/print-file-var-lib2.c
 create mode 100644 gdb/testsuite/gdb.base/print-file-var-main.c
 create mode 100644 gdb/testsuite/gdb.base/print-file-var.exp

diff --git a/gdb/config/djgpp/fnchange.lst b/gdb/config/djgpp/fnchange.lst
index 59e456e..7d5ea82 100644
--- a/gdb/config/djgpp/fnchange.lst
+++ b/gdb/config/djgpp/fnchange.lst
@@ -417,6 +417,10 @@
 @V@/gdb/testsuite/gdb.base/hook-stop-frame.c @V@/gdb/testsuite/gdb.base/hstop-frame.c
 @V@/gdb/testsuite/gdb.base/hook-stop-continue.exp @V@/gdb/testsuite/gdb.base/hstop-continue.exp
 @V@/gdb/testsuite/gdb.base/hook-stop-frame.exp @V@/gdb/testsuite/gdb.base/hstop-frame.exp
+@V@/gdb/testsuite/gdb.base/print-file-var-lib1.c @V@/gdb/testsuite/gdb.base/pfv-lib1.c
+@V@/gdb/testsuite/gdb.base/print-file-var-lib2.c @V@/gdb/testsuite/gdb.base/pfv-lib2.c
+@V@/gdb/testsuite/gdb.base/print-file-var-main.c @V@/gdb/testsuite/gdb.base/pfv-main.c
+@V@/gdb/testsuite/gdb.base/print-file-var.exp @V@/gdb/testsuite/gdb.base/pfv.exp
 @V@/gdb/testsuite/gdb.base/return-nodebug1.c @V@/gdb/testsuite/gdb.base/return-1nodebug.c
 @V@/gdb/testsuite/gdb.base/siginfo-addr.c @V@/gdb/testsuite/gdb.base/si-addr.c
 @V@/gdb/testsuite/gdb.base/siginfo-obj.c @V@/gdb/testsuite/gdb.base/si-obj.c
diff --git a/gdb/testsuite/gdb.base/print-file-var-lib1.c b/gdb/testsuite/gdb.base/print-file-var-lib1.c
new file mode 100644
index 0000000..dc9d03d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/print-file-var-lib1.c
@@ -0,0 +1,23 @@
+/* This testcase is part of GDB, the GNU debugger.
+   Copyright 2012 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int this_version_id = 104;
+
+int
+get_version_1 (void)
+{
+  return this_version_id;
+}
diff --git a/gdb/testsuite/gdb.base/print-file-var-lib2.c b/gdb/testsuite/gdb.base/print-file-var-lib2.c
new file mode 100644
index 0000000..1803cb2
--- /dev/null
+++ b/gdb/testsuite/gdb.base/print-file-var-lib2.c
@@ -0,0 +1,23 @@
+/* This testcase is part of GDB, the GNU debugger.
+   Copyright 2012 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int this_version_id = 203;
+
+int
+get_version_2 (void)
+{
+  return this_version_id;
+}
diff --git a/gdb/testsuite/gdb.base/print-file-var-main.c b/gdb/testsuite/gdb.base/print-file-var-main.c
new file mode 100644
index 0000000..b8baf0f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/print-file-var-main.c
@@ -0,0 +1,29 @@
+/* This testcase is part of GDB, the GNU debugger.
+   Copyright 2012 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+extern int get_version_1 (void);
+extern int get_version_2 (void);
+
+int
+main (void)
+{
+  if (get_version_1 () != 104)
+    return 1;
+  if (get_version_2 () != 104)
+    return 2;
+  return 0;
+}
+
diff --git a/gdb/testsuite/gdb.base/print-file-var.exp b/gdb/testsuite/gdb.base/print-file-var.exp
new file mode 100644
index 0000000..67c3ac4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/print-file-var.exp
@@ -0,0 +1,55 @@
+# Copyright 2012 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+set executable print-file-var-main
+
+set lib1 "print-file-var-lib1"
+set lib2 "print-file-var-lib2"
+
+set libobj1 "${objdir}/${subdir}/${lib1}.so"
+set libobj2 "${objdir}/${subdir}/${lib2}.so"
+
+set lib_opts { debug additional_flags=-fPIC }
+
+if { [gdb_compile_shlib ${srcdir}/${subdir}/${lib1}.c \
+                        ${libobj1} \
+                        ${lib_opts} ] != "" } {
+    return -1
+}
+if { [gdb_compile_shlib ${srcdir}/${subdir}/${lib2}.c \
+                        ${libobj2} \
+                        ${lib_opts} ] != "" } {
+    return -1
+}
+if { [gdb_compile "${srcdir}/${subdir}/${executable}.c" \
+                  "${objdir}/${subdir}/${executable}" \
+                  executable \
+                  [list debug shlib=${libobj1} shlib=${libobj2}]]
+     != ""} {
+    return -1
+}
+
+clean_restart $executable
+
+if ![runto_main] {
+    untested "could not run to main"
+    return -1
+}
+
+gdb_test "print 'print-file-var-lib1.c'::this_version_id" \
+         " = 104"
+
+gdb_test "print 'print-file-var-lib2.c'::this_version_id" \
+         " = 203"
-- 
1.7.1

Re: [RFA] choose symbol from given block's objfile first.

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> So, just to be sure: Are we OK with this iteration for now?

Yes, I am.

Tom

Re: [RFC] choose symbol from given block's objfile first.

[Pedro Alves]

On 05/07/2012 11:43 PM, Joel Brobecker wrote:

> Hello,
> 
> This patch is a prototype for an issue very briefly discussed on IRC.
> Quick description of the problem:
> 
>     We have a program which is linked against two shared libraries.
>     Both libraries define a global symbol with the same name. In my
>     example, I used an int called this_library_version.  When trying
>     to print the value of this global, GDB just randomly selects
>     the first one it finds, and I couldn't find a way (that worked)
>     which allowed me to print the value of the other global.
> 
> The idea behind this patch is to reduce a little bit the randomness.
> If one is debugging code inside one of the shared libraries, then
> the variable that the user probably wants is the one that is defined
> inside that shared library.


+1.

> 
> It's a bit of a poor man's answer to this issue. On the one hand,
> you do not always get the same symbol every single time. But on
> the other hand, the selection process is implicit and not always
> work-able for the user.  Eventually, what we thought we needed was
> extend the expression parser to allow the user to qualify his
> variable name with the name of the objfile, such as for instance:
> 
>         (gdb) print libsomething.so::this_library_version
> 

> This is something that can be done in parallel to this effort.

Yeah... It's one of those features that I think if we added together
all the time people have spent saying IWBN to have it, it'd sum up
to enough to implement it.  :-)

<pedantic mode>
Still, this wouldn't solve the case of loading the same
library twice...  Which symbol would you print?
</pedantic mode>

-- 
Pedro Alves

Re: [RFC] choose symbol from given block's objfile first.

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> <pedantic mode>
Pedro> Still, this wouldn't solve the case of loading the same
Pedro> library twice...  Which symbol would you print?
Pedro> </pedantic mode>

Yeah, we'd need additional syntax for that.

One idea that came up on irc was to have 'info var' print the address of
variables.  That way you could always at least find the address of the
one you want.

Tom

Re: [RFC] choose symbol from given block's objfile first.

[Matt Rice]

On Thu, May 10, 2012 at 7:32 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
> Pedro> <pedantic mode>
> Pedro> Still, this wouldn't solve the case of loading the same
> Pedro> library twice...  Which symbol would you print?
> Pedro> </pedantic mode>
>
> Yeah, we'd need additional syntax for that.
>
> One idea that came up on irc was to have 'info var' print the address of
> variables.  That way you could always at least find the address of the
> one you want.

another idea (inspired by the handle returned by dlopen, argument to
dlsym and friends), is something like

(gdb) print libsomething.so@1::this_library_version
(gdb) print libsomething.so@2::this_library_version

and some associated command to get a list of libraries and their handle.

Re: [RFC] choose symbol from given block's objfile first.

[Pedro Alves]

On 05/10/2012 03:50 PM, Matt Rice wrote:

> On Thu, May 10, 2012 at 7:32 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>>
>> Pedro> <pedantic mode>
>> Pedro> Still, this wouldn't solve the case of loading the same
>> Pedro> library twice...  Which symbol would you print?
>> Pedro> </pedantic mode>
>>
>> Yeah, we'd need additional syntax for that.
>>
>> One idea that came up on irc was to have 'info var' print the address of
>> variables.  That way you could always at least find the address of the
>> one you want.
> 
> another idea (inspired by the handle returned by dlopen, argument to
> dlsym and friends), is something like
> 
> (gdb) print libsomething.so@1::this_library_version
> (gdb) print libsomething.so@2::this_library_version
> 
> and some associated command to get a list of libraries and their handle.


Sounds like a good idea.  "info sharedlibrary" itself could show
the unambiguous handles.

-- 
Pedro Alves

checked in: [RFA] choose symbol from given block's objfile first.

[Joel Brobecker]

Thanks, Tom.

I have checked all 3 patches in (one code fix, and two testcases).

-- 
Joel

Re: [RFA] choose symbol from given block's objfile first.

[Joel Brobecker]

> > I'm not so sure.  It seems like you could make a multi-objfile test case
> > where an incomplete type is incorrectly resolved to a type in another
> > objfile.

I created a PR for that:
http://sourceware.org/bugzilla/show_bug.cgi?id=14093

I noted in the PR description that applying the same treatment as
what we did here would not be sufficient if the same type name is
used twice in the same objfile for two distinct types. In that case,
I don't know if there is really a solution other than forcing the
user to cast the result to a type using an expression that includes
sufficient information to pick the correct one.

-- 
Joel

Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]

[Jan Kratochvil]

On Wed, 09 May 2012 21:05:29 +0200, Joel Brobecker wrote:
> gdb/ChangeLog:
> 
>         * findvar.c (default_read_var_value): For LOC_UNRESOLVED symbols,
>         try locating the symbol in the symbol's own objfile first, before
>         extending the search to all objfiles.
>         * symtab.c (lookup_symbol_aux_objfile): New function, extracted
>         out of lookup_symbol_aux_symtabs.
>         (lookup_symbol_aux_symtabs): Add new parameter "exclude_objfile".
>         Replace extracted-out code by call to lookup_symbol_aux_objfile.
>         Do not search EXCLUDE_OBJFILE.
>         (lookup_static_symbol_aux): Update call to lookup_symbol_aux_symtabs.
>         (lookup_symbol_global): Search for matches in the block's objfile
>         first, before searching all other objfiles.
> 
> gdb/testsuite/ChangeLog:
> 
>         * gdb.base/ctxobj-f.c, gdb.base/ctxobj-m.c, gdb.base/ctxobj-v.c,
>         gdb.base/ctxobj.exp: New files.
> 
> Tested on x86_64-linux.  No regression.

I have different opinion.

 Running gdb/testsuite/gdb.fortran/library-module.exp ...
 PASS: gdb.fortran/library-module.exp: continue to breakpoint: i-is-2-in-lib
-PASS: gdb.fortran/library-module.exp: print var_i in lib
+FAIL: gdb.fortran/library-module.exp: print var_i in lib
 PASS: gdb.fortran/library-module.exp: continue to breakpoint: i-is-2-in-main
-PASS: gdb.fortran/library-module.exp: print var_i in main
+FAIL: gdb.fortran/library-module.exp: print var_i in main
 PASS: gdb.fortran/library-module.exp: print var_j
 PASS: gdb.fortran/library-module.exp: print var_k

439e2afe4a00ff5f7c07ac033208207e095b1708 is the first bad commit
commit 439e2afe4a00ff5f7c07ac033208207e095b1708
Author: Joel Brobecker <brobecker@gnat.com>
Date:   Thu May 10 16:24:38 2012 +0000

    Search global symbols from the expression's block objfile first.


Regards,
Jan

Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]

[Joel Brobecker]

> I have different opinion.
> 
>  Running gdb/testsuite/gdb.fortran/library-module.exp ...
>  PASS: gdb.fortran/library-module.exp: continue to breakpoint: i-is-2-in-lib
> -PASS: gdb.fortran/library-module.exp: print var_i in lib
> +FAIL: gdb.fortran/library-module.exp: print var_i in lib
>  PASS: gdb.fortran/library-module.exp: continue to breakpoint: i-is-2-in-main
> -PASS: gdb.fortran/library-module.exp: print var_i in main
> +FAIL: gdb.fortran/library-module.exp: print var_i in main
>  PASS: gdb.fortran/library-module.exp: print var_j
>  PASS: gdb.fortran/library-module.exp: print var_k

Can you send me the binary? I'll try taking a look.

Thanks,
-- 
Joel

Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]

[Joel Brobecker]

>  Running gdb/testsuite/gdb.fortran/library-module.exp ...
> -PASS: gdb.fortran/library-module.exp: print var_i in lib
> +FAIL: gdb.fortran/library-module.exp: print var_i in lib
> -PASS: gdb.fortran/library-module.exp: print var_i in main
> +FAIL: gdb.fortran/library-module.exp: print var_i in main

Jan and I discussed this briefly on IRC, and here is what's happening.

The `lib' unit, which is compiled into a shared library, defines
a global named var_i.  The main subprogram makes a reference to
that variable.  What happens in our case is something that Jan called
copy-relocation. Quoting Jan:

        From the naive programmer's point of view there is
        single variable.  But sure technically there are two
        copies of that variable, due to copy-relocation.  The
        variable located in the .so file is dead and GDB must
        not use it.  But GDB has some bugs in it.  Just

Looking at the debugging information, we indeed see 2 definitions
for that global variable:

  . The first one is of course the global variable defined in
    the `lib' module:

        <1><d6>: Abbrev Number: 3 (DW_TAG_variable)
           <d7>   DW_AT_name        : var_i
           <df>   DW_AT_MIPS_linkage_name: __lib__var_i
           <ec>   DW_AT_type        : <0xfb>
           <f0>   DW_AT_external    : 1
           <f1>   DW_AT_location    : 9 byte block: 3 f8 8 20 0 0 0 0 0        (DW_OP_addr: 2008f8)

  . And then the second instance, which is defined as an external
    variable, but within the scope of the main subprogram (that's
    the `<2>' on the first line:

        <2><c9>: Abbrev Number: 3 (DW_TAG_variable)
           <ca>   DW_AT_name        : var_i
           <d2>   DW_AT_MIPS_linkage_name: __lib__var_i
           <df>   DW_AT_type        : <0x106>
           <e3>   DW_AT_external    : 1
           <e4>   DW_AT_declaration : 1

There is no DW_AT_location for the second one, so I assume we go
through the minimal symbol table to get its actual address.

The Fortran implementation is taking advantage of the fact that
the executable is always going to be the first objfile being searched
by ALL_OBJFILES. My patch breaks that assumption. Jan also seems to
think that we may have the same sort of issue with C++ but we do not
have a reproducer.

I had some time to think this over during the weekend, and only see
two solutions:

  (1) Back my patch out, which I think would be a real shame;
  (2) Conditionalize my patch on the current language. If the current
      language is Fortran, then loop over the objfiles as before.
      I am a little unhappy about adding a reference to the current
      language, but I don't see a way to infer any meaningful
      language from the data we have in lookup_symbol_global (we have
      the objfile's global block, and a domain_enum).

There is an intermediate solution, which is to always search the
main objfile first, and then the current DSO, and then the rest.
I think that'd be getting a little too complicated to explain to
the average user...

Thoughts?
-- 
Joel

Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]

[Jan Kratochvil]

Hi Joel,

On Mon, 14 May 2012 16:39:27 +0200, Joel Brobecker wrote:
>   (2) Conditionalize my patch on the current language. If the current
>       language is Fortran, then loop over the objfiles as before.

Please no Fortran exceptions.  GDB is buggy even for plain C, see:
	Copy-Relocate debug error
	http://sourceware.org/ml/gdb/2012-01/msg00120.html

I can look at it myself but only later this week.


> There is an intermediate solution, which is to always search the
> main objfile first, and then the current DSO, and then the rest.

Some such order may be what ld.so does for the symbol search which should be
the real right solution.

Except for that there is probably wrong debug info in some cases from GCC and
(a) I believe GDB should try to be compatible with current widespread buggy
GCC debug info, (b) We may try to propose fixing the GCC produced debug info
but I have no idea how yet, some such possibilities were described in
	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40040


Thanks,
Jan

Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]

[Joel Brobecker]

> > There is an intermediate solution, which is to always search the
> > main objfile first, and then the current DSO, and then the rest.
> 
> Some such order may be what ld.so does for the symbol search which
> should be the real right solution.

Is it possible to link an executable that defines a global variable
against a shared library that also defines a global variable with
the same name? It's something I tried on GNU/Linux, but the linker
rejected the link.

If that's something that's not possible, then I am OK with that
intermediate solution, because it means that real duplicates are
going to be only within shared libraries. If it's defined inside
the main objfile, then it cannot be defined as a global in any other
shared library, and we should be OK.

> Except for that there is probably wrong debug info in some cases from
> GCC and (a) I believe GDB should try to be compatible with current
> widespread buggy GCC debug info, (b) We may try to propose fixing the
> GCC produced debug info but I have no idea how yet, some such
> possibilities were described in
> 	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40040

I agree with both.

-- 
Joel

Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]

[Jan Kratochvil]

On Mon, 14 May 2012 17:05:52 +0200, Joel Brobecker wrote:
> Is it possible to link an executable that defines a global variable
> against a shared library that also defines a global variable with
> the same name? It's something I tried on GNU/Linux, but the linker
> rejected the link.

Works for me, it is also perfectly defined this way:

echo -e '#include <stdio.h>\nint x=1;void f(void){printf("lib:%d\\n",x);}'|gcc -x c -fPIC -Wall -shared -o var.so -;echo -e '#include <stdio.h>\nint x=2;extern void f(void);int main(void){printf("main:%d\\n",x);f();return 0;}'|gcc ./var.so -x c -Wall -o var -;./var
main:2
lib:2


Thanks,
Jan

Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]

[Joel Brobecker]

> Works for me, it is also perfectly defined this way:
> 
> echo -e '#include <stdio.h>\nint x=1;void f(void){printf("lib:%d\\n",x);}'|gcc -x c -fPIC -Wall -shared -o var.so -;echo -e '#include <stdio.h>\nint x=2;extern void f(void);int main(void){printf("main:%d\\n",x);f();return 0;}'|gcc ./var.so -x c -Wall -o var -;./var
> main:2
> lib:2

[Mumbling something about the guy who invented the notion that one-liners
are cool and witty... Your one-liners are easy  to understand, but
always extremely labour intenstive to read]

That being said, thanks for setting me straight. I don't understand
why the linker rejected it for me, and I don't have the code anymore.

So this is another example of copy-relocation? When you say this is
perfectly defined, this looks horrifying to me. It feels like you can
break a shared library's code that way...

-- 
Joel

Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]

[Jan Kratochvil]

On Mon, 14 May 2012 18:56:37 +0200, Joel Brobecker wrote:
> So this is another example of copy-relocation?

No, this was just overriding library's symbol by executable's symbol.

$ main executable's has now just 'extern int x;'
echo -e '#include <stdio.h>\nint x=1;void f(void){printf("lib:%d\\n",x);}'|gcc -x c -fPIC -Wall -shared -o var.so -;echo -e '#include <stdio.h>\nextern int x;extern void f(void);int main(void){printf("main:%d\\n",x);f();return 0;}'|gcc ./var.so -x c -Wall -o var -;./var
main:1
lib:1
$ readelf -Wr var
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
0000000000600a40  0000000700000005 R_X86_64_COPY          0000000000600a40 x + 0

This is copy-relocation because the variable must be in the main executable
but at the same time it must be initialized from the library's value.


> When you say this is perfectly defined, this looks horrifying to me. It
> feels like you can break a shared library's code that way...

If library does not want to get its data overriden it should use
-fvisibility=hidden and properly mark any really exported variables by
"__attribute__ ((visibility("default")))".  See man gcc for -fvisibility.

Exporting any variables from shared libraries should be rather avoided anyway
as it is generally expensive, because compiler has to ensure &variable has the
same address from any module.

That -fvisibility=hidden is not default is just unfortunately backward
compatibility.  The default -fvisibility=default is needlessly expensive.
glibc does these tricks with visibilities.


Regards,
Jan

Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]

[Pedro Alves]

On 05/14/2012 03:51 PM, Jan Kratochvil wrote:

> Some such order may be what ld.so does for the symbol search which should be
> the real right solution.


I agree; I was going to suggest it.  And naturally, this is target dependent.
On Windows/PE, there's no concept of symbol preemption, so looking in the current
image / objfile would always be good.  We already do something of the sort
with solib_global_lookup ->  elf_lookup_lib_symbol.

-- 
Pedro Alves

Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]

[Joel Brobecker]

> I agree; I was going to suggest it.  And naturally, this is target
> dependent.  On Windows/PE, there's no concept of symbol preemption, so
> looking in the current image / objfile would always be good.  We
> already do something of the sort with solib_global_lookup ->
> elf_lookup_lib_symbol.

<brainstorming>

So enhance elf_lookup_lib_symbol to search the main objfile?
It would mean that, if the main objfile doesn't have the symbol,
and thus returns no match, the normal lookup loop would potentially
search the main objfile a second time...

Perhaps a gdbarch method?

</brainstorming>

-- 
Joel

Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]

[Jan Kratochvil]

On Mon, 14 May 2012 19:59:13 +0200, Joel Brobecker wrote:
> So enhance elf_lookup_lib_symbol to search the main objfile?

When we are already fixing it we should already fix it right.

Therefore to have the right lookup order even between shared libraries.

There should be also testfile for it testing it all.

The lookup gets more complex as a library symbol may be not only static vs.
global but it may be also hidden.  And moreover it can be also weak (and now
also secondary-weak with H.J.'s proposal).

This is a small project on its work to get GDB right, I plan it already for
many years (like many other GDB projects).  Sure we may also choose only some
subset of all these fixes.


Regards,
Jan

Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]

[Joel Brobecker]

> On Mon, 14 May 2012 19:59:13 +0200, Joel Brobecker wrote:
> > So enhance elf_lookup_lib_symbol to search the main objfile?
> 
> When we are already fixing it we should already fix it right.
> Therefore to have the right lookup order even between shared libraries.

... but at the same time taking into account the context from which
the user made the request...

I want to make sure that we don't get carried away too much and forget
to include plans to fix the regression in our immediate future. For
instance, Pedro suggested that we should search the main objfile first,
but that this should be target dependent.  Thinking about it, he's right,
of course. But coding it right is probably going to take some thinking.
On the other hand, implementing this approach for all targets seems
easy enough, and would buy us some time.


-- 
Joel

RFC for: "Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]"

[Joel Brobecker]

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

Hello everyone,

Here is something that implements a compromise between the previous
behavior, and the new behavior in terms of objfile search order.

The gist of the patch is that it introduces an iterator that can
take a "context" objfile into account for the iteration order.
The rest are small adjustements to use the iterator. Long term,
I think we are opening the door for more arch-dependency by
adjusting the body of the iterator. I'll leave that part to someone
else who's motivated (I think it's a can of worms).

This patch assumes that my other patch has been reverted.  This way,
it allows us to start from the initial code, and see how I am
changing it this time.

It also includes a C testcase, which hopefully reproduces the same
issue as gdb.fortran/library-module.exp, but in C, to widen the
number of people testing this feature.

For now, it's only a prototype, so I'm seeking comments. I will
document the code when we're happy with the approach.

Tested on x86_64-linux, no regression. Fixes the new testcase.

Thanks,
-- 
Joel

[-- Attachment #2: 0001-WIP.patch --]
[-- Type: text/x-diff, Size: 12550 bytes --]

From da06bff34e77513ee10b664e009cd0c871ad5f82 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Wed, 16 May 2012 11:33:26 -0700
Subject: [PATCH] WIP.

---
 gdb/findvar.c                         |   29 +++++++++-
 gdb/objfiles.c                        |   46 ++++++++++++++
 gdb/objfiles.h                        |    8 +++
 gdb/symtab.c                          |  104 ++++++++++++++++++++++----------
 gdb/testsuite/gdb.base/cp-rel-lib.c   |   29 +++++++++
 gdb/testsuite/gdb.base/cp-rel-var.c   |   36 +++++++++++
 gdb/testsuite/gdb.base/cp-rel-var.exp |   48 +++++++++++++++
 7 files changed, 266 insertions(+), 34 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/cp-rel-lib.c
 create mode 100644 gdb/testsuite/gdb.base/cp-rel-var.c
 create mode 100644 gdb/testsuite/gdb.base/cp-rel-var.exp

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 9009e6f..2f6fb29 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -406,6 +406,25 @@ symbol_read_needs_frame (struct symbol *sym)
   return 1;
 }
 
+struct minsym_lookup_data
+{
+  const char *name;
+  struct minimal_symbol *result;
+};
+
+static int
+minsym_lookup_iterator_cb (struct objfile *objfile, void *cb_data)
+{
+  struct minsym_lookup_data *data = (struct minsym_lookup_data *)cb_data;
+
+  gdb_assert (data->result == NULL);
+
+  data->result = lookup_minimal_symbol (data->name, NULL, objfile);
+
+  /* The iterator should stop iff a match was found.  */
+  return (data->result != NULL);
+}
+
 /* A default implementation for the "la_read_var_value" hook in
    the language vector which should work in most situations.  */
 
@@ -559,10 +578,18 @@ default_read_var_value (struct symbol *var, struct frame_info *frame)
 
     case LOC_UNRESOLVED:
       {
+	struct minsym_lookup_data lookup_data;
 	struct minimal_symbol *msym;
 	struct obj_section *obj_section;
 
-	msym = lookup_minimal_symbol (SYMBOL_LINKAGE_NAME (var), NULL, NULL);
+	memset (&lookup_data, 0, sizeof (lookup_data));
+	lookup_data.name = SYMBOL_LINKAGE_NAME (var);
+
+	iterate_over_objfiles_in_search_order (minsym_lookup_iterator_cb,
+					       &lookup_data,
+					       SYMBOL_SYMTAB (var)->objfile);
+	msym = lookup_data.result;
+
 	if (msym == NULL)
 	  error (_("No global symbol \"%s\"."), SYMBOL_LINKAGE_NAME (var));
 	if (overlay_debugging)
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 8d9f8a5..8dffac5 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -1525,6 +1525,52 @@ gdb_bfd_unref (struct bfd *abfd)
   xfree (name);
 }
 
+/* FIXME: Document.  */
+
+void
+iterate_over_objfiles_in_search_order
+  (iterate_over_objfiles_in_search_order_cb cb,
+   void *cb_data,
+   struct objfile *context_objfile)
+{
+  int stop = 0;
+  struct objfile *objfile;
+
+  if (context_objfile && context_objfile->flags & OBJF_MAINLINE)
+    {
+      stop = cb (context_objfile, cb_data);
+      if (stop)
+	return;
+    }
+
+  ALL_OBJFILES (objfile)
+    {
+      if (objfile->flags & OBJF_MAINLINE && objfile != context_objfile)
+	{
+	  stop = cb (objfile, cb_data);
+	  if (stop)
+	    return;
+	}
+    }
+
+  if (context_objfile && !(context_objfile->flags & OBJF_MAINLINE))
+    {
+      stop = cb (context_objfile, cb_data);
+      if (stop)
+	return;
+    }
+
+  ALL_OBJFILES (objfile)
+    {
+      if (!(objfile->flags & OBJF_MAINLINE) && objfile != context_objfile)
+	{
+	  stop = cb (objfile, cb_data);
+	  if (stop)
+	    return;
+	}
+    }
+}
+
 /* Provide a prototype to silence -Wmissing-prototypes.  */
 extern initialize_file_ftype _initialize_objfiles;
 
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index d5c807f..ca204db 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -525,6 +525,14 @@ extern void *objfile_data (struct objfile *objfile,
 extern struct bfd *gdb_bfd_ref (struct bfd *abfd);
 extern void gdb_bfd_unref (struct bfd *abfd);
 extern int gdb_bfd_close_or_warn (struct bfd *abfd);
+
+typedef int (*iterate_over_objfiles_in_search_order_cb)
+  (struct objfile *objfile, void *cb_data);
+
+extern void iterate_over_objfiles_in_search_order
+  (iterate_over_objfiles_in_search_order_cb cb,
+   void *cb_data,
+   struct objfile *context_objfile);
 \f
 
 /* Traverse all object files in the current program space.
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 0ed42fd..2486c9a 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -1500,40 +1500,55 @@ lookup_global_symbol_from_objfile (const struct objfile *main_objfile,
   return NULL;
 }
 
-/* Check to see if the symbol is defined in one of the symtabs.
-   BLOCK_INDEX should be either GLOBAL_BLOCK or STATIC_BLOCK,
+/* Check to see if the symbol is defined in one of the OBJFILE's
+   symtabs.  BLOCK_INDEX should be either GLOBAL_BLOCK or STATIC_BLOCK,
    depending on whether or not we want to search global symbols or
    static symbols.  */
 
 static struct symbol *
+lookup_symbol_aux_objfile (struct objfile *objfile, int block_index,
+			   const char *name, const domain_enum domain)
+{
+  struct symbol *sym = NULL;
+  struct blockvector *bv;
+  const struct block *block;
+  struct symtab *s;
+
+  if (objfile->sf)
+    objfile->sf->qf->pre_expand_symtabs_matching (objfile, block_index,
+						  name, domain);
+
+  ALL_OBJFILE_SYMTABS (objfile, s)
+    if (s->primary)
+      {
+	bv = BLOCKVECTOR (s);
+	block = BLOCKVECTOR_BLOCK (bv, block_index);
+	sym = lookup_block_symbol (block, name, domain);
+	if (sym)
+	  {
+	    block_found = block;
+	    return fixup_symbol_section (sym, objfile);
+	  }
+      }
+
+  return NULL;
+}
+
+/* Same as lookup_symbol_aux_objfile, except that it searches all
+   objfiles.  Return the first match found.  */
+
+static struct symbol *
 lookup_symbol_aux_symtabs (int block_index, const char *name,
 			   const domain_enum domain)
 {
   struct symbol *sym;
   struct objfile *objfile;
-  struct blockvector *bv;
-  const struct block *block;
-  struct symtab *s;
 
   ALL_OBJFILES (objfile)
   {
-    if (objfile->sf)
-      objfile->sf->qf->pre_expand_symtabs_matching (objfile,
-						    block_index,
-						    name, domain);
-
-    ALL_OBJFILE_SYMTABS (objfile, s)
-      if (s->primary)
-	{
-	  bv = BLOCKVECTOR (s);
-	  block = BLOCKVECTOR_BLOCK (bv, block_index);
-	  sym = lookup_block_symbol (block, name, domain);
-	  if (sym)
-	    {
-	      block_found = block;
-	      return fixup_symbol_section (sym, objfile);
-	    }
-	}
+    sym = lookup_symbol_aux_objfile (objfile, block_index, name, domain);
+    if (sym)
+      return sym;
   }
 
   return NULL;
@@ -1650,6 +1665,33 @@ lookup_symbol_static (const char *name,
     return NULL;
 }
 
+struct global_sym_lookup_data
+{
+  const char *name;
+  domain_enum domain;
+  struct symbol *result;
+};
+
+static int
+lookup_symbol_global_iterator_cb (struct objfile *objfile,
+				  void *cb_data)
+{
+  struct global_sym_lookup_data *data = 
+    (struct global_sym_lookup_data *) cb_data;
+
+  gdb_assert (data->result == NULL);
+
+  data->result = lookup_symbol_aux_objfile (objfile, GLOBAL_BLOCK,
+					    data->name, data->domain);
+  if (data->result == NULL)
+    data->result = lookup_symbol_aux_quick (objfile, GLOBAL_BLOCK,
+					    data->name, data->domain);
+
+  /* If we found a match, tell the iterator to stop.  Otherwise,
+     keep going.  */
+  return (data->result != NULL);
+}
+
 /* Lookup a symbol in all files' global blocks (searching psymtabs if
    necessary).  */
 
@@ -1660,6 +1702,7 @@ lookup_symbol_global (const char *name,
 {
   struct symbol *sym = NULL;
   struct objfile *objfile = NULL;
+  struct global_sym_lookup_data lookup_data;
 
   /* Call library-specific lookup procedure.  */
   objfile = lookup_objfile_from_block (block);
@@ -1668,18 +1711,13 @@ lookup_symbol_global (const char *name,
   if (sym != NULL)
     return sym;
 
-  sym = lookup_symbol_aux_symtabs (GLOBAL_BLOCK, name, domain);
-  if (sym != NULL)
-    return sym;
+  memset (&lookup_data, 0, sizeof (lookup_data));
+  lookup_data.name = name;
+  lookup_data.domain = domain;
+  iterate_over_objfiles_in_search_order (lookup_symbol_global_iterator_cb,
+					 &lookup_data, objfile);
 
-  ALL_OBJFILES (objfile)
-  {
-    sym = lookup_symbol_aux_quick (objfile, GLOBAL_BLOCK, name, domain);
-    if (sym)
-      return sym;
-  }
-
-  return NULL;
+  return lookup_data.result;
 }
 
 int
diff --git a/gdb/testsuite/gdb.base/cp-rel-lib.c b/gdb/testsuite/gdb.base/cp-rel-lib.c
new file mode 100644
index 0000000..d2c6984
--- /dev/null
+++ b/gdb/testsuite/gdb.base/cp-rel-lib.c
@@ -0,0 +1,29 @@
+/* This testcase is part of GDB, the GNU debugger.
+   Copyright 2012 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+
+int cp_rel_var = 1;
+
+int
+get_cp_rel_var (void)
+{
+  if (cp_rel_var != 1)
+    abort ();
+
+  cp_rel_var = 2;
+  return cp_rel_var;  /* LIB_STOP */
+}
diff --git a/gdb/testsuite/gdb.base/cp-rel-var.c b/gdb/testsuite/gdb.base/cp-rel-var.c
new file mode 100644
index 0000000..fd7f751
--- /dev/null
+++ b/gdb/testsuite/gdb.base/cp-rel-var.c
@@ -0,0 +1,36 @@
+/* This testcase is part of GDB, the GNU debugger.
+   Copyright 2012 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+
+int cp_rel_var;
+
+extern int get_cp_rel_var (void);
+
+int
+main (void)
+{
+  int tmp;
+
+  if (cp_rel_var != 1)
+    abort ();
+
+  tmp = get_cp_rel_var ();
+  if (cp_rel_var != 2)  /* VAR_STOP */
+    abort ();
+
+  return (tmp != cp_rel_var);
+}
diff --git a/gdb/testsuite/gdb.base/cp-rel-var.exp b/gdb/testsuite/gdb.base/cp-rel-var.exp
new file mode 100644
index 0000000..aa99379
--- /dev/null
+++ b/gdb/testsuite/gdb.base/cp-rel-var.exp
@@ -0,0 +1,48 @@
+# Copyright 2012 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+set executable cp-rel-var
+
+set libsrc "${srcdir}/${subdir}/cp-rel-lib.c"
+set libobj "${objdir}/${subdir}/libcp-rel-lib.so"
+set libobj_opts { debug additional_flags=-fPIC }
+
+if { [gdb_compile_shlib $libsrc $libobj $libobj_opts ] != "" } {
+    return -1
+}
+
+if { [gdb_compile "${srcdir}/${subdir}/${executable}.c" \
+                  "${objdir}/${subdir}/${executable}" \
+                  executable \
+                  [list debug shlib=${libobj}]]
+     != ""} {
+    return -1
+}
+
+clean_restart $executable
+
+if ![runto_main] {
+    untested "could not run to main"
+    return -1
+}
+
+gdb_breakpoint $libsrc:[gdb_get_line_number "STOP" $libsrc]
+gdb_continue_to_breakpoint "LIB_STOP" ".*LIB_STOP.*"
+gdb_test "print cp_rel_var" " = 2" "print cp_rel_var from lib"
+
+gdb_breakpoint ${executable}.c:[gdb_get_line_number "STOP" ${executable}.c]
+gdb_continue_to_breakpoint "VAR_STOP" ".*VAR_STOP.*"
+gdb_test "print cp_rel_var" " = 2" "print cp_rel_var from main"
+
-- 
1.7.1

gdb.base/print-file-var.exp false PASS [Re: [RFA] choose symbol from given block's objfile first.]

[Jan Kratochvil]

On Wed, 09 May 2012 23:48:56 +0200, Joel Brobecker wrote:
> +++ b/gdb/testsuite/gdb.base/print-file-var-lib1.c
[..]
> +int this_version_id = 104;
> +
> +int
> +get_version_1 (void)
> +{
> +  return this_version_id;
> +}
[...]
> +++ b/gdb/testsuite/gdb.base/print-file-var-lib2.c
[...]
> +int this_version_id = 203;
> +
> +int
> +get_version_2 (void)
> +{
> +  return this_version_id;
> +}
[...]
> +++ b/gdb/testsuite/gdb.base/print-file-var-main.c
[...]
> +int
> +main (void)
> +{
> +  if (get_version_1 () != 104)
> +    return 1;
> +  if (get_version_2 () != 104)
> +    return 2;
> +  return 0;
> +}
[...]
> +++ b/gdb/testsuite/gdb.base/print-file-var.exp
[...]
> +gdb_test "print 'print-file-var-lib1.c'::this_version_id" \
> +         " = 104"
> +
> +gdb_test "print 'print-file-var-lib2.c'::this_version_id" \
> +         " = 203"

This testcase proves GDB behaves incorrectly - if the code sees 104 in both
cases then GDB should also print 104 in both cases.  I am curious why did you
check it in when you have proven yourself the regression.


Regards,
Jan

Re: gdb.base/print-file-var.exp false PASS [Re: [RFA] choose symbol from given block's objfile first.]

[Joel Brobecker]

On Fri, May 18, 2012 at 06:10:08PM +0200, Jan Kratochvil wrote:
> On Wed, 09 May 2012 23:48:56 +0200, Joel Brobecker wrote:
> > +++ b/gdb/testsuite/gdb.base/print-file-var-lib1.c
> [..]
> > +int this_version_id = 104;
> > +
> > +int
> > +get_version_1 (void)
> > +{
> > +  return this_version_id;
> > +}
> [...]
> > +++ b/gdb/testsuite/gdb.base/print-file-var-lib2.c
> [...]
> > +int this_version_id = 203;
> > +
> > +int
> > +get_version_2 (void)
> > +{
> > +  return this_version_id;
> > +}
> [...]
> > +++ b/gdb/testsuite/gdb.base/print-file-var-main.c
> [...]
> > +int
> > +main (void)
> > +{
> > +  if (get_version_1 () != 104)
> > +    return 1;
> > +  if (get_version_2 () != 104)
> > +    return 2;
> > +  return 0;
> > +}
> [...]
> > +++ b/gdb/testsuite/gdb.base/print-file-var.exp
> [...]
> > +gdb_test "print 'print-file-var-lib1.c'::this_version_id" \
> > +         " = 104"
> > +
> > +gdb_test "print 'print-file-var-lib2.c'::this_version_id" \
> > +         " = 203"
> 
> This testcase proves GDB behaves incorrectly - if the code sees 104 in both
> cases then GDB should also print 104 in both cases.  I am curious why did you
> check it in when you have proven yourself the regression.

I really don't know how to read your last sentence, and I am going to
pretend you did not write it. Please correct me if I misunderstood
what you are trying to imply.

My mistake, I was expecting get_version_2 in this example to return
203, not 104. But I see now that, on my Linux machine, it does return
104.  On the other hand, on Windows, with the same code, the function
returns 203.  So the code is not portable.

I wonder how things are working on GNU/Linux, because the two shared
libraries are linked independently, and then the main executable
does not reference the global variable at all.

I don't know what to do. I can remove the testcase entirely, or
we can test the target and adjust the expected output based on
that.

This is still not going to help with the GDB side of things. But
I don't think that this is a regression. I don't think we have any
way of knowing which instance of the variable to pick.

-- 
Joel

Re: gdb.base/print-file-var.exp false PASS [Re: [RFA] choose symbol from given block's objfile first.]

[Jan Kratochvil]

On Fri, 18 May 2012 19:17:08 +0200, Joel Brobecker wrote:
> I really don't know how to read your last sentence, and I am going to
> pretend you did not write it. Please correct me if I misunderstood
> what you are trying to imply.

I think it is clear that if GDB commands prints value X of variable Y then the
inferior program (either running standalone or under GDB) should also evaluate
variable Y as value X.

$ gdb.base/print-file-var-main ;echo $?
0

Therefore inferior sees 'print-file-var-lib2.c'::this_version_id as 104
> > > +  if (get_version_2 () != 104)
> > > +    return 2;
as otherwise the executable would return 2 (and not 0).

But GDB PASSes if 'print-file-var-lib2.c'::this_version_id is read as 203.
I do not understand this discrepancy between print-file-var.exp and
print-file-var-main.c.


> On the other hand, on Windows, with the same code, the function
> returns 203.  So the code is not portable.

I did not remember the MS-Windows platform behavior (Pedro has stated it).
In such case this program returns code 2 (and not 0) on MS-Windows?



> I wonder how things are working on GNU/Linux, because the two shared
> libraries are linked independently, and then the main executable
> does not reference the global variable at all.

As the libraries are build with -fPIC they use .got references:
0000000000000670 <get_version_1>:
[...]
 674:   48 8b 05 95 02 20 00    mov    0x200295(%rip),%rax        # 200910 <_DYNAMIC+0x1e0>
 67b:   8b 00                   mov    (%rax),%eax

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  [22] .data             PROGBITS        0000000000200950 000950 000004 00  WA  0   0  4
  [20] .got              PROGBITS        0000000000200900 000900 000030 08  WA  0   0  8

Relocation section '.rela.dyn' at offset 0x450 contains 9 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
0000000000200910  0000000d00000006 R_X86_64_GLOB_DAT      0000000000200950 this_version_id + 0
 - in .got

Symbol table '.dynsym' contains 14 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
    13: 0000000000200950     4 OBJECT  GLOBAL DEFAULT   22 this_version_id
 - in .data

And therefore for the first library ld.so resolves the .got reference into
itself but for the second library the .got reference points to the first library's variable.


> I don't know what to do. I can remove the testcase entirely, or
> we can test the target and adjust the expected output based on
> that.

I think it depends how complete/good fix ends up checked in GDB.


> This is still not going to help with the GDB side of things. But
> I don't think that this is a regression. I don't think we have any
> way of knowing which instance of the variable to pick.

This check-in regressed the C example from:
	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40040

cat >clib.c <<EOH
int var = 1;
int func (void) { return var; }
EOH
cat >cmain.c <<EOH
extern int var; extern int func (void); int main (void) {
  var = 2;
  return var == func () ? 0 : 1; }
EOH
C="gcc -Wall -g"; $C -o clib.so -shared -fPIC clib.c; $C -o cmain cmain.c ./clib.so
gdb ./cmain
(gdb) b func
(gdb) run
Breakpoint 1, func () at clib.c:6
6   return var;
(gdb) p var
before - matches inferior:
$1 = 2
current - does not match inferior:
$1 = 1


Thanks,
Jan

Re: RFC for: "Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]"

[Jan Kratochvil]

On Wed, 16 May 2012 21:57:18 +0200, Joel Brobecker wrote:
> --- a/gdb/objfiles.c
> +++ b/gdb/objfiles.c
[...]
> +void
> +iterate_over_objfiles_in_search_order
> +  (iterate_over_objfiles_in_search_order_cb cb,
> +   void *cb_data,
> +   struct objfile *context_objfile)
> +{
> +  int stop = 0;
> +  struct objfile *objfile;
> +
> +  if (context_objfile && context_objfile->flags & OBJF_MAINLINE)
> +    {
> +      stop = cb (context_objfile, cb_data);
> +      if (stop)
> +	return;
> +    }
> +
> +  ALL_OBJFILES (objfile)
> +    {
> +      if (objfile->flags & OBJF_MAINLINE && objfile != context_objfile)
> +	{
> +	  stop = cb (objfile, cb_data);
> +	  if (stop)
> +	    return;
> +	}
> +    }

I see a bit problem this function is objfile based and not solib based, as the
solib order from svr4_current_sos() is completely lost here.

That is if we have two shared libraries with global variable X ld.so defines
which one gets used depending on their dlopen order.

so_list_head already has wrong order due to update_solib_list.  Maybe
update_solib_list should be fixed to keep both so_list_head order and also
properly ensure matching objfiles order.

If someone does some add-symbol-file by hand the order gets lost anyway.

Maybe we could ensure just so_list_head order, place this "correct order
search" to elf_lookup_lib_symbol and fall back to the old unfixed objfiles
search in arbitrary order to keep add-symbol-file working.

Sure there should be some exceptions for hidden symbols but this should be
implementable on top of your CONTEXT_OBJFILE being there probably for this
purpose.  Plus weak symbols.


> +
> +  if (context_objfile && !(context_objfile->flags & OBJF_MAINLINE))
> +    {
> +      stop = cb (context_objfile, cb_data);
> +      if (stop)
> +	return;
> +    }
> +
> +  ALL_OBJFILES (objfile)
> +    {
> +      if (!(objfile->flags & OBJF_MAINLINE) && objfile != context_objfile)
> +	{
> +	  stop = cb (objfile, cb_data);
> +	  if (stop)
> +	    return;
> +	}
> +    }
> +}

I do not see why to use the OBJF_MAINLINE exceptions here.  I see the correct
order is just the most simple ALL_OBJFILES loop.

Maybe you could re-state the case you were trying to fix as your testcase
	gdb.base/print-file-var.exp false PASS [Re: [RFA] choose symbol from given block's objfile first.]
	http://sourceware.org/ml/gdb-patches/2012-05/msg00692.html
is wrong.

address_info ("info addr") function is wrong but it does not apply for
"print".


Thanks,
Jan

Re: RFC for: "Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]"

[Joel Brobecker]

> I see a bit problem this function is objfile based and not solib
> based, as the solib order from svr4_current_sos() is completely lost
> here.
> 
> That is if we have two shared libraries with global variable X ld.so
> defines which one gets used depending on their dlopen order.
> 
> so_list_head already has wrong order due to update_solib_list.  Maybe
> update_solib_list should be fixed to keep both so_list_head order and
> also properly ensure matching objfiles order.
> 
> If someone does some add-symbol-file by hand the order gets lost
> anyway.
> 
> Maybe we could ensure just so_list_head order, place this "correct
> order search" to elf_lookup_lib_symbol and fall back to the old
> unfixed objfiles search in arbitrary order to keep add-symbol-file
> working.
> 
> Sure there should be some exceptions for hidden symbols but this
> should be implementable on top of your CONTEXT_OBJFILE being there
> probably for this purpose.  Plus weak symbols.

I just want to make sure that we do not get carried away trying to
implement the perfect solution, because I don't have the time in
the next few weeks to implement it, and I don't even know how to
do that at the moment.

What I am trying to do is to fix the regression that you observed
in the Fortran testsuite, which somewhat also affects all languages
as well.

> > +  if (context_objfile && !(context_objfile->flags & OBJF_MAINLINE))
> > +    {
> > +      stop = cb (context_objfile, cb_data);
> > +      if (stop)
> > +	return;
> > +    }
> > +
> > +  ALL_OBJFILES (objfile)
> > +    {
> > +      if (!(objfile->flags & OBJF_MAINLINE) && objfile != context_objfile)
> > +	{
> > +	  stop = cb (objfile, cb_data);
> > +	  if (stop)
> > +	    return;
> > +	}
> > +    }
> > +}
> 
> I do not see why to use the OBJF_MAINLINE exceptions here.  I see the
> correct order is just the most simple ALL_OBJFILES loop.

You have to start with the MAINLINE objfiles first, as you demonstrated
with your Fortran program.

What the code above does, in practice, is trying to go through
the ALL_OBJFILES list in the same order as before, except that
we try the "context" objfile file first.  But, to make things
even more interesting, having a "context" objfile does not mean
that it should be checked ahead of any MAINLINE objfile, because
otherwise we break your Fortran test.  Another way of expressing
the intent of my patch is to say: Try the "context" objfile first,
unless it's not a MAINLINE objfile, in which case we try the
MAINLINE objfiles, and then our "context" objfile.

> Maybe you could re-state the case you were trying to fix as your testcase
> 	gdb.base/print-file-var.exp false PASS [Re: [RFA] choose symbol from given block's objfile first.]
> 	http://sourceware.org/ml/gdb-patches/2012-05/msg00692.html
> is wrong.

Actually, I prefer target-dependent, since I showed that it is not
wrong for x86-windows.

I definitely feel like I've opened a can of worms that I will not have
time and energy to fix (actual linking behavior is target dependent,
and getting the correct search order is also target dependent). This
was also meant as a cheap improvement while I work on extending the
expression parser(s) [I plan on enhancing C and Ada only, not the
other languages]

So, we have two options, at this point:

  (1) Revert my original patch;

  (2) Enhance the heuristics in my patch. This second patch is an
      attempt at this.

I think it would be a loss to revert, but if we cannot converge on
the heuristics for the search, then we'll just go to the previous
search order (which may also be arbitrary in some ways).

In the end, I think the current heuristics are not perfect but more
helpful in some situations, without hopefully be worse for some
other situations. If we can't agree on that, then I think the best
option at this point is to revert my patch, as it obviously had
some unintended and unwanted side effects.

Which way do we want to go?

-- 
Joel

PS: Just a reminder that we are also potentially one week away from
    branching.

Re: RFC for: "Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]"

[Jan Kratochvil]

On Mon, 28 May 2012 16:27:27 +0200, Joel Brobecker wrote:
> I just want to make sure that we do not get carried away trying to
> implement the perfect solution, because I don't have the time in
> the next few weeks to implement it, and I don't even know how to
> do that at the moment.

I somehow also share this approach currently.


> What the code above does, in practice, is trying to go through
> the ALL_OBJFILES list in the same order as before, except that
> we try the "context" objfile file first.  But, to make things
> even more interesting, having a "context" objfile does not mean
> that it should be checked ahead of any MAINLINE objfile, because
> otherwise we break your Fortran test.

Just the MAINLINE exception is not enough, it still regresses GDB:

==> 50.c <==
//extern int v;
extern int f (void);
extern int g (void);
int main (void) {
  return 10 * f () + g ();
}
==> 50l.c <==
int v=1;
int f (void) {
  return v;
}
==> 50ll.c <==
int v=2;
int g (void) {
  return v;
}
==============
gcc -o 50l.so 50l.c -Wall -g -shared -fPIC;gcc -o 50ll.so 50ll.c -Wall -g -shared -fPIC;gcc -o 50a 50.c -Wall -g ./50l.so ./50ll.so;gcc -o 50b 50.c -Wall -g ./50ll.so ./50l.so;./50a;echo $?;./50b;echo $?
for e in 50a 50b;do ./$e;echo $?;./gdb -q ./$e -ex 'b f' -ex 'b g' -ex r -ex 'p v' -ex c -ex 'p v' -ex c -ex q;done

before your patches (and therefore matching GDB the inferior behavior):
11
$1 = 1
$2 = 1
22
$1 = 2
$2 = 2

with your final patches (undo the first, apply this WIP second patch)
11
$1 = 1
$2 = 2
22
$1 = 1
$2 = 2

GDB is just behaving right in normal cases.

GDB is not correct, that it does not correctly maintain objfiles order in the
case of some dlopens/dlcloses (at least I guess so, I did not try to
reproduce).


> Another way of expressing
> the intent of my patch is to say: Try the "context" objfile first,
> unless it's not a MAINLINE objfile, in which case we try the
> MAINLINE objfiles, and then our "context" objfile.

But '"context" objfile first' is incompatible with SVR4.  The behavior
apparently has to be target specific.


> So, we have two options, at this point:
> 
>   (1) Revert my original patch;
> 
>   (2) Enhance the heuristics in my patch. This second patch is an
>       attempt at this.
> 
> I think it would be a loss to revert, but if we cannot converge on
> the heuristics for the search, then we'll just go to the previous
> search order (which may also be arbitrary in some ways).

Not sure what do you call 'heuristics' but as long as it becomes target
dependent the patch should be simple.


Thanks,
Jan

Re: RFC for: "Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]"

[Joel Brobecker]

> Just the MAINLINE exception is not enough, it still regresses GDB:

OK, I see what you mean, now.

> before your patches (and therefore matching GDB the inferior behavior):
> 11
> $1 = 1
> $2 = 1
> 22
> $1 = 2
> $2 = 2

On x86-windows, the two programs alway return the same value: 12,
which means that the global variables remain distinct, and localized
to each DLL.  It's not a surprise, it's the same test I've made before.
Just to be thorough, I defined a third instance of global variable v,
this time in 50.c (the main body), and change the return value to
also add the value of this global:

    $ cat 50.c
    int v = 100;

    extern int f (void);
    extern int g (void);

    int main (void) {
      return 10 * f () + g () + v;
    }

This time, the return value is always 112. Again, no surprise.

> But '"context" objfile first' is incompatible with SVR4.  The behavior
> apparently has to be target specific.

OK, I think we should be able to modify my patch to add a new
gdbarch attribute, unset by default, which we will set on all
Windows arches. Then, the iterator can query that gdbarch attribute
to decide whether to ignore the "context" objfile or not.

The following implementation for iterate_over_objfiles_in_search_order
should work; WDYT?

    void
    iterate_over_objfiles_in_search_order
      (iterate_over_objfiles_in_search_order_cb cb,
       void *cb_data,
       struct objfile *context_objfile)
    {
      int stop = 0;
      struct objfile *objfile;

      if (context_objfile
          && gdbarch_dso_global_vars_distinct_p
               (get_objfile_arch (context_objfile)))
        {
          /* Global variables defined with the same name in multiple
             object files get merged into one single entity.  Favouring
             the context objfile in this case would be wrong, as we might
             end up picking the wrong location.  */
          context_objfile = NULL;
        }

      if (context_objfile)
        {
          stop = cb (context_objfile, cb_data);
          if (stop)
            return;
        }

      ALL_OBJFILES (objfile)
        {
          if (objfile != context_objfile)
            {
              stop = cb (objfile, cb_data);
              if (stop)
                return;
            }
        }
    }

(untested)

The thing I am worried about is the design of the gdbarch part,
because I don't think I master all the elements across platforms.
But I suppose that this is only of relative importance, as we can
refine it as we go.

Thanks,
-- 
Joel

Re: RFC for: "Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]"

[Joel Brobecker]

>       if (context_objfile
>           && gdbarch_dso_global_vars_distinct_p
>                (get_objfile_arch (context_objfile)))

There should probably be a !gdbarch_..._distinct_p.

-- 
Joel

Re: RFC for: "Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]"

[Jan Kratochvil]

On Tue, 29 May 2012 17:44:36 +0200, Joel Brobecker wrote:
> OK, I think we should be able to modify my patch to add a new
> gdbarch attribute, unset by default, which we will set on all
> Windows arches. Then, the iterator can query that gdbarch attribute
> to decide whether to ignore the "context" objfile or not.

I agree in general.

Just a gdbarch design detail - gdbarch attribute should be a real function so
that the default (SVR4-compatible) implementation can just ignore the
context_objfile parameter.  The MS-Windows implementation will unconditionally
follow context_objfile.  This will not clutter the default code with
MS-Windows specifics.


Thanks,
Jan

Re: RFC for: "Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]"

[Joel Brobecker]

> Just a gdbarch design detail - gdbarch attribute should be a real function so
> that the default (SVR4-compatible) implementation can just ignore the
> context_objfile parameter.  The MS-Windows implementation will unconditionally
> follow context_objfile.  This will not clutter the default code with
> MS-Windows specifics.

I don't understand what you are trying to say regarding the gdbarch
attribute. Do you mean that, instead of a yes/no integer, it should
be a pointer to a function returning the yes/no integer?

-- 
Joel

Re: RFC for: "Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]"

[Jan Kratochvil]

On Tue, 29 May 2012 18:02:01 +0200, Joel Brobecker wrote:
> > Just a gdbarch design detail - gdbarch attribute should be a real function so
> > that the default (SVR4-compatible) implementation can just ignore the
> > context_objfile parameter.  The MS-Windows implementation will unconditionally
> > follow context_objfile.  This will not clutter the default code with
> > MS-Windows specifics.
> 
> I don't understand what you are trying to say regarding the gdbarch
> attribute. Do you mean that, instead of a yes/no integer, it should
> be a pointer to a function returning the yes/no integer?

It should be a pointer to function implementing the functionality - which
differs across platforms:

M:void:iterate_over_objfiles_in_search_order: iterate_over_objfiles_in_search_order_cb cb, void *cb_data, struct objfile *context_objfile: cb, cb_data, context_objfile
 - I did not check it should be very exactly this way like m vs. M etc.

default implementation:
    static void
    default_iterate_over_objfiles_in_search_order
      (iterate_over_objfiles_in_search_order_cb cb,
       void *cb_data,
       struct objfile *context_objfile)
    {
      int stop = 0;
      struct objfile *objfile;

      ALL_OBJFILES (objfile)
        {
              stop = cb (objfile, cb_data);
              if (stop)
                return;
       	}
    }
    set_gdbarch_iterate_over_objfiles_in_search_order (gdbarch, default_iterate_over_objfiles_in_search_order);


ms-windows implementation

    static void
    windows_iterate_over_objfiles_in_search_order
      (iterate_over_objfiles_in_search_order_cb cb,
       void *cb_data,
       struct objfile *context_objfile)
    {
      int stop = 0;
      struct objfile *objfile;

      if (context_objfile)
        {
          stop = cb (context_objfile, cb_data);
          if (stop)
            return;
        }

      ALL_OBJFILES (objfile)
        {
          if (objfile != context_objfile)
            {
              stop = cb (objfile, cb_data);
              if (stop)
                return;
            }
       	}
    }
    set_gdbarch_iterate_over_objfiles_in_search_order (gdbarch, windows_iterate_over_objfiles_in_search_order);


Thanks,
Jan

Re: RFC for: "Re: Regression for gdb.fortran/library-module.exp [Re: [RFA] choose symbol from given block's objfile first.]"

[Pedro Alves]

On 05/29/2012 05:11 PM, Jan Kratochvil wrote:

> It should be a pointer to function implementing the functionality - which
> differs across platforms:
> 
> M:void:iterate_over_objfiles_in_search_order: iterate_over_objfiles_in_search_order_cb cb, void *cb_data, struct objfile *context_objfile: cb, cb_data, context_objfile
>  - I did not check it should be very exactly this way like m vs. M etc.
> 
> default implementation:
>     static void
>     default_iterate_over_objfiles_in_search_order
>       (iterate_over_objfiles_in_search_order_cb cb,
>        void *cb_data,
>        struct objfile *context_objfile)
>     {
...


>     set_gdbarch_iterate_over_objfiles_in_search_order (gdbarch, default_iterate_over_objfiles_in_search_order);
> 
> 
> ms-windows implementation
> 
>     static void
>     windows_iterate_over_objfiles_in_search_order
>       (iterate_over_objfiles_in_search_order_cb cb,
>        void *cb_data,
>        struct objfile *context_objfile)
>     {

...

>     set_gdbarch_iterate_over_objfiles_in_search_order (gdbarch, windows_iterate_over_objfiles_in_search_order);
> 

Agreed.

(I'd suggest renaming context_objfile -> current_objfile, as that sounds to me closer
to what it really is used for, rather than a generic context, but that's a nit.)

-- 
Pedro Alves