diff mbox

[v2,4/8] Search global block from basic_lookup_symbol_nonlocal

Message ID 20190827095357.GO6076@embecosm.com
State New
Headers show

Commit Message

Andrew Burgess Aug. 27, 2019, 9:53 a.m. UTC 
* Andrew Burgess <andrew.burgess@embecosm.com> [2019-08-21 11:32:25 +0100]:

> * Tom Tromey <tromey@adacore.com> [2019-08-01 11:04:08 -0600]:
> 
> > This changes basic_lookup_symbol_nonlocal to look in the global block
> > of the passed-in block.  If no block was passed in, it reverts to the
> > previous behavior.
> > 
> > This change is needed to ensure that 'FILENAME'::NAME lookups work
> > properly.  As debugging Pedro's test case showed, this was not working
> > properly in the case where multiple identical names could be found
> > (the one situation where this feature is truly needed :-).
> > 
> > This also removes some old comments from basic_lookup_symbol_nonlocal
> > that no longer apply once this patch goes in.
> 
> So I guess the tests for this are going to be in the
> gdb.base/print-file-var.exp changes that are part of patch #8.  It
> would be great if the commit message could mention this - it just
> makes life easier later on.
> 
> I wonder if we need to update other *_lookup_symbol_nonlocal functions
> in a similar way?  For example can the C tests be compiled as C++,
> which should cause GDB to use cp_lookup_symbol_nonlocal.
> 
> Looking at both basic_lookup_symbol_nonlocal and
> cp_lookup_symbol_nonlocal, I wonder if your fix could be moved into
> lookup_global_symbol?  And just have 'block_global_block (block)'
> checked before the search of all global blocks?

The patch below applies on top of this series and extends
gdb.base/print-file-var.exp to compile as both C++ and C.  The C++
will fail initially.

The patch also includes a proposed fix to move the searching of the
"current" global block into lookup_global_symbol, after this both the
C++ and C versions of the test pass, and there are no other test
regressions.

This patch is going to clash with Christian Biesinger's patch to
refactor lookup_global_symbol, but hopefully merging these two
shouldn't be that hard.

I also tweaked the test to remove some duplicate test names.

What do you think?

Thanks,
Andrew

---

Comments

Doug Evans via gdb-patches Aug. 27, 2019, 6:16 p.m. UTC | #1 
On Tue, Aug 27, 2019 at 4:54 AM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> @@ -2662,6 +2646,22 @@ lookup_global_symbol (const char *name,
>    if (objfile != NULL)
>      result = solib_global_lookup (objfile, name, domain);
>
> +  /* If a block was passed in, we want to search the corresponding
> +     global block first.  This yields "more expected" behavior, and is
> +     needed to support 'FILENAME'::VARIABLE lookups.  */
> +  const struct block *global_block = block_global_block (block);
> +  if (global_block != nullptr)
> +    {
> +      result.symbol = lookup_symbol_in_block (name,
> +                                             symbol_name_match_type::FULL,
> +                                             global_block, domain);
> +      if (result.symbol != nullptr)
> +       {
> +         result.block = global_block;
> +         return result;
> +       }
> +    }
> +
>    /* If that didn't work go a global search (of global blocks, heh).  */
>    if (result.symbol == NULL)
>      {

I like this change but shouldn't this call happen before solib_global_lookup?

(That said, I would like to remove that function...
https://patches-gcc.linaro.org/patch/21673/)

Christian
diff mbox

Patch

diff --git a/gdb/symtab.c b/gdb/symtab.c
index bd6fa35db6a..63a39e2996a 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -2426,22 +2426,6 @@  basic_lookup_symbol_nonlocal (const struct language_defn *langdef,
   if (result.symbol != NULL)
     return result;
 
-  /* If a block was passed in, we want to search the corresponding
-     global block now.  This yields "more expected" behavior, and is
-     needed to support 'FILENAME'::VARIABLE lookups.  */
-  const struct block *global_block = block_global_block (block);
-  if (global_block != nullptr)
-    {
-      result.symbol = lookup_symbol_in_block (name,
-					      symbol_name_match_type::FULL,
-					      global_block, domain);
-      if (result.symbol != nullptr)
-	{
-	  result.block = global_block;
-	  return result;
-	}
-    }
-
   /* If we didn't find a definition for a builtin type in the static block,
      search for it now.  This is actually the right thing to do and can be
      a massive performance win.  E.g., when debugging a program with lots of
@@ -2662,6 +2646,22 @@  lookup_global_symbol (const char *name,
   if (objfile != NULL)
     result = solib_global_lookup (objfile, name, domain);
 
+  /* If a block was passed in, we want to search the corresponding
+     global block first.  This yields "more expected" behavior, and is
+     needed to support 'FILENAME'::VARIABLE lookups.  */
+  const struct block *global_block = block_global_block (block);
+  if (global_block != nullptr)
+    {
+      result.symbol = lookup_symbol_in_block (name,
+					      symbol_name_match_type::FULL,
+					      global_block, domain);
+      if (result.symbol != nullptr)
+	{
+	  result.block = global_block;
+	  return result;
+	}
+    }
+
   /* If that didn't work go a global search (of global blocks, heh).  */
   if (result.symbol == NULL)
     {
diff --git a/gdb/testsuite/gdb.base/print-file-var-lib1.c b/gdb/testsuite/gdb.base/print-file-var-lib1.c
index aec04a9b02b..d172c15bc7d 100644
--- a/gdb/testsuite/gdb.base/print-file-var-lib1.c
+++ b/gdb/testsuite/gdb.base/print-file-var-lib1.c
@@ -19,6 +19,8 @@ 
 
 ATTRIBUTE_VISIBILITY int this_version_id = 104;
 
+START_EXTERN_C
+
 int
 get_version_1 (void)
 {
@@ -26,3 +28,5 @@  get_version_1 (void)
 
   return this_version_id;
 }
+
+END_EXTERN_C
diff --git a/gdb/testsuite/gdb.base/print-file-var-lib2.c b/gdb/testsuite/gdb.base/print-file-var-lib2.c
index 4dfdfa04c99..b392aff9f3d 100644
--- a/gdb/testsuite/gdb.base/print-file-var-lib2.c
+++ b/gdb/testsuite/gdb.base/print-file-var-lib2.c
@@ -19,9 +19,13 @@ 
 
 ATTRIBUTE_VISIBILITY int this_version_id = 203;
 
+START_EXTERN_C
+
 int
 get_version_2 (void)
 {
   printf ("get_version_2: &this_version_id=%p, this_version_id=%d\n", &this_version_id, this_version_id);
   return this_version_id;
 }
+
+END_EXTERN_C
diff --git a/gdb/testsuite/gdb.base/print-file-var-main.c b/gdb/testsuite/gdb.base/print-file-var-main.c
index 29d4fed22d1..1472bd44883 100644
--- a/gdb/testsuite/gdb.base/print-file-var-main.c
+++ b/gdb/testsuite/gdb.base/print-file-var-main.c
@@ -23,9 +23,13 @@ 
 
 #include "print-file-var.h"
 
+START_EXTERN_C
+
 extern int get_version_1 (void);
 extern int get_version_2 (void);
 
+END_EXTERN_C
+
 #if VERSION_ID_MAIN
 ATTRIBUTE_VISIBILITY int this_version_id = 55;
 #endif
diff --git a/gdb/testsuite/gdb.base/print-file-var.exp b/gdb/testsuite/gdb.base/print-file-var.exp
index a37cca70de6..1a065cf568b 100644
--- a/gdb/testsuite/gdb.base/print-file-var.exp
+++ b/gdb/testsuite/gdb.base/print-file-var.exp
@@ -17,7 +17,7 @@  if {[skip_shlib_tests]} {
     return -1
 }
 
-proc test {hidden dlopen version_id_main} {
+proc test {hidden dlopen version_id_main lang} {
     global srcdir subdir
 
     set main "print-file-var-main"
@@ -32,7 +32,7 @@  proc test {hidden dlopen version_id_main} {
     set libobj1 [standard_output_file ${lib1}$suffix.so]
     set libobj2 [standard_output_file ${lib2}$suffix.so]
 
-    set lib_opts { debug additional_flags=-fPIC }
+    set lib_opts { debug additional_flags=-fPIC $lang }
     lappend lib_opts "additional_flags=-DHIDDEN=$hidden"
 
     if { [gdb_compile_shlib ${srcdir}/${subdir}/${lib1}.c \
@@ -46,7 +46,7 @@  proc test {hidden dlopen version_id_main} {
 	return -1
     }
 
-    set main_opts [list debug shlib=${libobj1}]
+    set main_opts [list debug shlib=${libobj1} $lang]
 
     if {$dlopen} {
 	lappend main_opts "shlib_load" \
@@ -108,12 +108,14 @@  proc test {hidden dlopen version_id_main} {
 
     # Compare the values of $sym1 and $sym2.
     proc compare {sym1 sym2} {
-	# Done this way instead of comparing the symbols with "print $sym1
-	# == sym2" in GDB directly so that the values of the symbols end
-	# up visible in the logs, for debug purposes.
-	set vsym1 [get_integer_valueof $sym1 -1]
-	set vsym2 [get_integer_valueof $sym2 -1]
-	gdb_assert {$vsym1 == $vsym2} "$sym1 == $sym2"
+	with_test_prefix "sym1=$sym1,sym2=$sym2" {
+	    # Done this way instead of comparing the symbols with "print $sym1
+	    # == sym2" in GDB directly so that the values of the symbols end
+	    # up visible in the logs, for debug purposes.
+	    set vsym1 [get_integer_valueof $sym1 -1]
+	    set vsym2 [get_integer_valueof $sym2 -1]
+	    gdb_assert {$vsym1 == $vsym2} "$sym1 == $sym2"
+	}
     }
 
     if $version_id_main {
@@ -123,13 +125,14 @@  proc test {hidden dlopen version_id_main} {
 
     compare "'print-file-var-lib1.c'::this_version_id" "v1"
     compare "'print-file-var-lib2.c'::this_version_id" "v2"
-
 }
 
-foreach_with_prefix hidden {0 1} {
-    foreach_with_prefix dlopen {0 1} {
-	foreach_with_prefix version_id_main {0 1} {
-	    test $hidden $dlopen $version_id_main
+foreach_with_prefix lang { c c++ } {
+    foreach_with_prefix hidden {0 1} {
+	foreach_with_prefix dlopen {0 1} {
+	    foreach_with_prefix version_id_main {0 1} {
+		test $hidden $dlopen $version_id_main $lang
+	    }
 	}
     }
 }
diff --git a/gdb/testsuite/gdb.base/print-file-var.h b/gdb/testsuite/gdb.base/print-file-var.h
index fe7a3460edb..c44e4848b4a 100644
--- a/gdb/testsuite/gdb.base/print-file-var.h
+++ b/gdb/testsuite/gdb.base/print-file-var.h
@@ -23,4 +23,12 @@ 
 # define ATTRIBUTE_VISIBILITY
 #endif
 
+#ifdef __cplusplus
+# define START_EXTERN_C extern "C" {
+# define END_EXTERN_C }
+#else
+# define START_EXTERN_C
+# define END_EXTERN_C
+#endif
+
 #endif /* PRINT_FILE_VAR_H */