[PATCH 0/2] Convert symbol-lookup debug to new debug scheme

[PATCH 0/2] Convert symbol-lookup debug to new debug scheme

[Andrew Burgess]

Convert the 'set debug symbol-lookup' to the new debug scheme.  Patch
2/2 is interesting though as it changes scoped_debug_start_end.

--

Andrew Burgess (2):
  gdb: convert 'set debug symbol-lookup' to new debug printing scheme
  gdb: add SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT

 gdb/cp-namespace.c        | 113 +++++------------
 gdb/language.c            |  12 +-
 gdb/minsyms.c             |  31 ++---
 gdb/rust-lang.h           |  12 +-
 gdb/symtab.c              | 261 ++++++++++++++------------------------
 gdb/symtab.h              |  19 +++
 gdbsupport/common-debug.h |  92 +++++++++++---
 7 files changed, 245 insertions(+), 295 deletions(-)


base-commit: e03698c1227bc18835cc2e4a9146a8369635e119
-- 
2.25.4

[PATCH 1/2] gdb: convert 'set debug symbol-lookup' to new debug printing scheme

[Andrew Burgess]

Convert the implementation of 'set debug symbol-lookup' to the new
debug printing scheme.

In a few places I've updated the debug output to remove places where
the printed debug message included the function name, the new debug
scheme already adds that, but I haven't done all the possible updates.
---
 gdb/cp-namespace.c | 113 ++++++--------------
 gdb/language.c     |  12 +--
 gdb/minsyms.c      |  31 +++---
 gdb/rust-lang.h    |  12 +--
 gdb/symtab.c       | 257 ++++++++++++++++-----------------------------
 gdb/symtab.h       |  12 +++
 6 files changed, 161 insertions(+), 276 deletions(-)

diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
index 634dab6ada0..6520c8adf85 100644
--- a/gdb/cp-namespace.c
+++ b/gdb/cp-namespace.c
@@ -507,14 +507,9 @@ cp_lookup_symbol_imports_or_template (const char *scope,
   struct symbol *function = block->function ();
   struct block_symbol result;
 
-  if (symbol_lookup_debug)
-    {
-      gdb_printf (gdb_stdlog,
-		  "cp_lookup_symbol_imports_or_template"
-		  " (%s, %s, %s, %s)\n",
-		  scope, name, host_address_to_string (block),
-		  domain_name (domain));
-    }
+  symbol_lookup_debug_printf
+    ("cp_lookup_symbol_imports_or_template (%s, %s, %s, %s)",
+     scope, name, host_address_to_string (block), domain_name (domain));
 
   if (function != NULL && function->language () == language_cplus)
     {
@@ -529,13 +524,9 @@ cp_lookup_symbol_imports_or_template (const char *scope,
 
 	  if (sym != NULL)
 	    {
-	      if (symbol_lookup_debug)
-		{
-		  gdb_printf (gdb_stdlog,
-			      "cp_lookup_symbol_imports_or_template"
-			      " (...) = %s\n",
-			      host_address_to_string (sym));
-		}
+	      symbol_lookup_debug_printf
+		("cp_lookup_symbol_imports_or_template (...) = %s",
+		 host_address_to_string (sym));
 	      return (struct block_symbol) {sym, block};
 	    }
 	}
@@ -574,13 +565,9 @@ cp_lookup_symbol_imports_or_template (const char *scope,
 				      TYPE_TEMPLATE_ARGUMENTS (context));
 	      if (sym != NULL)
 		{
-		  if (symbol_lookup_debug)
-		    {
-		      gdb_printf
-			(gdb_stdlog,
-			 "cp_lookup_symbol_imports_or_template (...) = %s\n",
-			 host_address_to_string (sym));
-		    }
+		  symbol_lookup_debug_printf
+		    ("cp_lookup_symbol_imports_or_template (...) = %s",
+		     host_address_to_string (sym));
 		  return (struct block_symbol) {sym, parent};
 		}
 	    }
@@ -588,13 +575,9 @@ cp_lookup_symbol_imports_or_template (const char *scope,
     }
 
   result = cp_lookup_symbol_via_imports (scope, name, block, domain, 0, 1, 1);
-  if (symbol_lookup_debug)
-    {
-      gdb_printf (gdb_stdlog,
-		  "cp_lookup_symbol_imports_or_template (...) = %s\n",
-		  result.symbol != NULL
-		  ? host_address_to_string (result.symbol) : "NULL");
-    }
+  symbol_lookup_debug_printf
+    ("cp_lookup_symbol_imports_or_template (...) = %s",
+     result.symbol != NULL ? host_address_to_string (result.symbol) : "NULL");
   return result;
 }
 
@@ -634,13 +617,9 @@ cp_lookup_symbol_namespace (const char *scope,
 {
   struct block_symbol sym;
 
-  if (symbol_lookup_debug)
-    {
-      gdb_printf (gdb_stdlog,
-		  "cp_lookup_symbol_namespace (%s, %s, %s, %s)\n",
-		  scope, name, host_address_to_string (block),
-		  domain_name (domain));
-    }
+  symbol_lookup_debug_printf ("cp_lookup_symbol_namespace (%s, %s, %s, %s)",
+			      scope, name, host_address_to_string (block),
+			      domain_name (domain));
 
   /* First, try to find the symbol in the given namespace.  */
   sym = cp_lookup_symbol_in_namespace (scope, name, block, domain, 1);
@@ -649,13 +628,9 @@ cp_lookup_symbol_namespace (const char *scope,
   if (sym.symbol == NULL)
     sym = cp_lookup_symbol_via_all_imports (scope, name, block, domain);
 
-  if (symbol_lookup_debug)
-    {
-      gdb_printf (gdb_stdlog,
-		  "cp_lookup_symbol_namespace (...) = %s\n",
-		  sym.symbol != NULL
-		  ? host_address_to_string (sym.symbol) : "NULL");
-    }
+  symbol_lookup_debug_printf ("cp_lookup_symbol_namespace (...) = %s",
+			      sym.symbol != NULL
+			      ? host_address_to_string (sym.symbol) : "NULL");
   return sym;
 }
 
@@ -740,14 +715,9 @@ cp_lookup_symbol_nonlocal (const struct language_defn *langdef,
   struct block_symbol sym;
   const char *scope = block_scope (block);
 
-  if (symbol_lookup_debug)
-    {
-      gdb_printf (gdb_stdlog,
-		  "cp_lookup_symbol_non_local"
-		  " (%s, %s (scope %s), %s)\n",
-		  name, host_address_to_string (block), scope,
-		  domain_name (domain));
-    }
+  symbol_lookup_debug_printf
+    ("cp_lookup_symbol_non_local (%s, %s (scope %s), %s)",
+     name, host_address_to_string (block), scope, domain_name (domain));
 
   /* First, try to find the symbol in the given namespace, and all
      containing namespaces.  */
@@ -757,14 +727,10 @@ cp_lookup_symbol_nonlocal (const struct language_defn *langdef,
   if (sym.symbol == NULL)
     sym = cp_lookup_symbol_via_all_imports (scope, name, block, domain);
 
-  if (symbol_lookup_debug)
-    {
-      gdb_printf (gdb_stdlog,
-		  "cp_lookup_symbol_nonlocal (...) = %s\n",
-		  (sym.symbol != NULL
-		   ? host_address_to_string (sym.symbol)
-		   : "NULL"));
-    }
+  symbol_lookup_debug_printf ("cp_lookup_symbol_nonlocal (...) = %s",
+			      (sym.symbol != NULL
+			       ? host_address_to_string (sym.symbol)
+			       : "NULL"));
   return sym;
 }
 
@@ -921,11 +887,10 @@ cp_lookup_nested_symbol (struct type *parent_type,
     {
       const char *type_name = saved_parent_type->name ();
 
-      gdb_printf (gdb_stdlog,
-		  "cp_lookup_nested_symbol (%s, %s, %s, %s)\n",
-		  type_name != NULL ? type_name : "unnamed",
-		  nested_name, host_address_to_string (block),
-		  domain_name (domain));
+      symbol_lookup_debug_printf ("cp_lookup_nested_symbol (%s, %s, %s, %s)",
+				  type_name != NULL ? type_name : "unnamed",
+				  nested_name, host_address_to_string (block),
+				  domain_name (domain));
     }
 
   switch (parent_type->code ())
@@ -955,25 +920,17 @@ cp_lookup_nested_symbol (struct type *parent_type,
 					 concatenated_name, block, domain,
 					 1, is_in_anonymous);
 
-	if (symbol_lookup_debug)
-	  {
-	    gdb_printf (gdb_stdlog,
-			"cp_lookup_nested_symbol (...) = %s\n",
-			(sym.symbol != NULL
-			 ? host_address_to_string (sym.symbol)
-			 : "NULL"));
-	  }
+	symbol_lookup_debug_printf ("cp_lookup_nested_symbol (...) = %s",
+				    (sym.symbol != NULL
+				     ? host_address_to_string (sym.symbol)
+				     : "NULL"));
 	return sym;
       }
 
     case TYPE_CODE_FUNC:
     case TYPE_CODE_METHOD:
-      if (symbol_lookup_debug)
-	{
-	  gdb_printf (gdb_stdlog,
-		      "cp_lookup_nested_symbol (...) = NULL"
-		      " (func/method)\n");
-	}
+      symbol_lookup_debug_printf
+	("cp_lookup_nested_symbol (...) = NULL (func/method)");
       return {};
 
     default:
diff --git a/gdb/language.c b/gdb/language.c
index 3962ee8fa24..5083a86c012 100644
--- a/gdb/language.c
+++ b/gdb/language.c
@@ -1077,17 +1077,15 @@ language_lookup_primitive_type_as_symbol (const struct language_defn *la,
   struct language_gdbarch *ld = get_language_gdbarch (gdbarch);
   struct language_arch_info *lai = &ld->arch_info[la->la_language];
 
-  if (symbol_lookup_debug)
-    gdb_printf (gdb_stdlog,
-		"language_lookup_primitive_type_as_symbol"
-		" (%s, %s, %s)",
-		la->name (), host_address_to_string (gdbarch), name);
+  symbol_lookup_debug_printf
+    ("language = \"%s\", gdbarch @ %s, type = \"%s\")",
+     la->name (), host_address_to_string (gdbarch), name);
 
   struct symbol *sym
     = lai->lookup_primitive_type_as_symbol (name, la->la_language);
 
-  if (symbol_lookup_debug)
-    gdb_printf (gdb_stdlog, " = %s\n", host_address_to_string (sym));
+  symbol_lookup_debug_printf ("found symbol @ %s",
+			      host_address_to_string (sym));
 
   /* Note: The result of symbol lookup is normally a symbol *and* the block
      it was found in.  Builtin types don't live in blocks.  We *could* give
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 3b65669d176..dbde6ce5de4 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -385,13 +385,9 @@ lookup_minimal_symbol (const char *name, const char *sfile,
       if (objf == NULL || objf == objfile
 	  || objf == objfile->separate_debug_objfile_backlink)
 	{
-	  if (symbol_lookup_debug)
-	    {
-	      gdb_printf (gdb_stdlog,
-			  "lookup_minimal_symbol (%s, %s, %s)\n",
-			  name, sfile != NULL ? sfile : "NULL",
-			  objfile_debug_name (objfile));
-	    }
+	  symbol_lookup_debug_printf ("lookup_minimal_symbol (%s, %s, %s)",
+				      name, sfile != NULL ? sfile : "NULL",
+				      objfile_debug_name (objfile));
 
 	  /* Do two passes: the first over the ordinary hash table,
 	     and the second over the demangled hash table.  */
@@ -438,9 +434,9 @@ lookup_minimal_symbol (const char *name, const char *sfile,
 	{
 	  minimal_symbol *minsym = found.external_symbol.minsym;
 
-	  gdb_printf (gdb_stdlog,
-		      "lookup_minimal_symbol (...) = %s (external)\n",
-		      host_address_to_string (minsym));
+	  symbol_lookup_debug_printf
+	    ("lookup_minimal_symbol (...) = %s (external)",
+	     host_address_to_string (minsym));
 	}
       return found.external_symbol;
     }
@@ -452,9 +448,9 @@ lookup_minimal_symbol (const char *name, const char *sfile,
 	{
 	  minimal_symbol *minsym = found.file_symbol.minsym;
 
-	  gdb_printf (gdb_stdlog,
-		      "lookup_minimal_symbol (...) = %s (file-local)\n",
-		      host_address_to_string (minsym));
+	  symbol_lookup_debug_printf
+	    ("lookup_minimal_symbol (...) = %s (file-local)",
+	     host_address_to_string (minsym));
 	}
       return found.file_symbol;
     }
@@ -466,17 +462,16 @@ lookup_minimal_symbol (const char *name, const char *sfile,
 	{
 	  minimal_symbol *minsym = found.trampoline_symbol.minsym;
 
-	  gdb_printf (gdb_stdlog,
-		      "lookup_minimal_symbol (...) = %s (trampoline)\n",
-		      host_address_to_string (minsym));
+	  symbol_lookup_debug_printf
+	    ("lookup_minimal_symbol (...) = %s (trampoline)",
+	     host_address_to_string (minsym));
 	}
 
       return found.trampoline_symbol;
     }
 
   /* Not found.  */
-  if (symbol_lookup_debug)
-    gdb_printf (gdb_stdlog, "lookup_minimal_symbol (...) = NULL\n");
+  symbol_lookup_debug_printf ("lookup_minimal_symbol (...) = NULL");
   return {};
 }
 
diff --git a/gdb/rust-lang.h b/gdb/rust-lang.h
index a1d10263feb..4e5f39c4945 100644
--- a/gdb/rust-lang.h
+++ b/gdb/rust-lang.h
@@ -148,14 +148,10 @@ class rust_language : public language_defn
   {
     struct block_symbol result = {};
 
-    if (symbol_lookup_debug)
-      {
-	gdb_printf (gdb_stdlog,
-		    "rust_lookup_symbol_non_local"
-		    " (%s, %s (scope %s), %s)\n",
-		    name, host_address_to_string (block),
-		    block_scope (block), domain_name (domain));
-      }
+    symbol_lookup_debug_printf
+      ("rust_lookup_symbol_non_local (%s, %s (scope %s), %s)",
+       name, host_address_to_string (block), block_scope (block),
+       domain_name (domain));
 
     /* Look up bare names in the block's scope.  */
     std::string scopedname;
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 0d342f765f2..14d81f5468d 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -1424,13 +1424,11 @@ symbol_cache_lookup (struct symbol_cache *cache,
 
   if (eq_symbol_entry (slot, objfile_context, name, domain))
     {
-      if (symbol_lookup_debug)
-	gdb_printf (gdb_stdlog,
-		    "%s block symbol cache hit%s for %s, %s\n",
-		    block == GLOBAL_BLOCK ? "Global" : "Static",
-		    slot->state == SYMBOL_SLOT_NOT_FOUND
-		    ? " (not found)" : "",
-		    name, domain_name (domain));
+      symbol_lookup_debug_printf ("%s block symbol cache hit%s for %s, %s",
+				  block == GLOBAL_BLOCK ? "Global" : "Static",
+				  slot->state == SYMBOL_SLOT_NOT_FOUND
+				  ? " (not found)" : "", name,
+				  domain_name (domain));
       ++bsc->hits;
       if (slot->state == SYMBOL_SLOT_NOT_FOUND)
 	return SYMBOL_LOOKUP_FAILED;
@@ -1439,13 +1437,9 @@ symbol_cache_lookup (struct symbol_cache *cache,
 
   /* Symbol is not present in the cache.  */
 
-  if (symbol_lookup_debug)
-    {
-      gdb_printf (gdb_stdlog,
-		  "%s block symbol cache miss for %s, %s\n",
-		  block == GLOBAL_BLOCK ? "Global" : "Static",
-		  name, domain_name (domain));
-    }
+  symbol_lookup_debug_printf ("%s block symbol cache miss for %s, %s",
+			      block == GLOBAL_BLOCK ? "Global" : "Static",
+			      name, domain_name (domain));
   ++bsc->misses;
   return {};
 }
@@ -1996,15 +1990,9 @@ lookup_language_this (const struct language_defn *lang,
   if (lang->name_of_this () == NULL || block == NULL)
     return {};
 
-  if (symbol_lookup_debug > 1)
-    {
-      struct objfile *objfile = block_objfile (block);
-
-      gdb_printf (gdb_stdlog,
-		  "lookup_language_this (%s, %s (objfile %s))",
-		  lang->name (), host_address_to_string (block),
-		  objfile_debug_name (objfile));
-    }
+  symbol_lookup_debug_printf_v ("lookup_language_this (%s, %s (objfile %s))",
+				lang->name (), host_address_to_string (block),
+				objfile_debug_name (block_objfile (block)));
 
   while (block)
     {
@@ -2015,13 +2003,10 @@ lookup_language_this (const struct language_defn *lang,
 				 VAR_DOMAIN);
       if (sym != NULL)
 	{
-	  if (symbol_lookup_debug > 1)
-	    {
-	      gdb_printf (gdb_stdlog, " = %s (%s, block %s)\n",
-			  sym->print_name (),
-			  host_address_to_string (sym),
-			  host_address_to_string (block));
-	    }
+	  symbol_lookup_debug_printf_v
+	    ("lookup_language_this (...) = %s (%s, block %s)",
+	     sym->print_name (), host_address_to_string (sym),
+	     host_address_to_string (block));
 	  return (struct block_symbol) {sym, block};
 	}
       if (block->function ())
@@ -2029,8 +2014,7 @@ lookup_language_this (const struct language_defn *lang,
       block = block->superblock ();
     }
 
-  if (symbol_lookup_debug > 1)
-    gdb_printf (gdb_stdlog, " = NULL\n");
+  symbol_lookup_debug_printf_v ("lookup_language_this (...) = NULL");
   return {};
 }
 
@@ -2096,12 +2080,13 @@ lookup_symbol_aux (const char *name, symbol_name_match_type match_type,
       struct objfile *objfile = (block == nullptr
 				 ? nullptr : block_objfile (block));
 
-      gdb_printf (gdb_stdlog,
-		  "lookup_symbol_aux (%s, %s (objfile %s), %s, %s)\n",
-		  name, host_address_to_string (block),
-		  objfile != NULL
-		  ? objfile_debug_name (objfile) : "NULL",
-		  domain_name (domain), language_str (language));
+      symbol_lookup_debug_printf
+	("demangled symbol name = \"%s\", block @ %s (objfile %s)",
+	 name, host_address_to_string (block),
+	 objfile != NULL ? objfile_debug_name (objfile) : "NULL");
+      symbol_lookup_debug_printf
+	("domain name = \"%s\", language = \"%s\")",
+	 domain_name (domain), language_str (language));
     }
 
   /* Make sure we do something sensible with is_a_field_of_this, since
@@ -2117,11 +2102,9 @@ lookup_symbol_aux (const char *name, symbol_name_match_type match_type,
   result = lookup_local_symbol (name, match_type, block, domain, language);
   if (result.symbol != NULL)
     {
-      if (symbol_lookup_debug)
-	{
-	  gdb_printf (gdb_stdlog, "lookup_symbol_aux (...) = %s\n",
-		      host_address_to_string (result.symbol));
-	}
+      symbol_lookup_debug_printf
+	("found symbol @ %s (using lookup_local_symbol)",
+	 host_address_to_string (result.symbol));
       return result;
     }
 
@@ -2154,11 +2137,7 @@ lookup_symbol_aux (const char *name, symbol_name_match_type match_type,
 
 	  if (check_field (t, name, is_a_field_of_this))
 	    {
-	      if (symbol_lookup_debug)
-		{
-		  gdb_printf (gdb_stdlog,
-			      "lookup_symbol_aux (...) = NULL\n");
-		}
+	      symbol_lookup_debug_printf ("no symbol found");
 	      return {};
 	    }
 	}
@@ -2170,11 +2149,9 @@ lookup_symbol_aux (const char *name, symbol_name_match_type match_type,
   result = langdef->lookup_symbol_nonlocal (name, block, domain);
   if (result.symbol != NULL)
     {
-      if (symbol_lookup_debug)
-	{
-	  gdb_printf (gdb_stdlog, "lookup_symbol_aux (...) = %s\n",
-		      host_address_to_string (result.symbol));
-	}
+      symbol_lookup_debug_printf
+	("found symbol @ %s (using language lookup_symbol_nonlocal)",
+	 host_address_to_string (result.symbol));
       return result;
     }
 
@@ -2182,13 +2159,9 @@ lookup_symbol_aux (const char *name, symbol_name_match_type match_type,
      but more useful than an error.  */
 
   result = lookup_static_symbol (name, domain);
-  if (symbol_lookup_debug)
-    {
-      gdb_printf (gdb_stdlog, "lookup_symbol_aux (...) = %s\n",
-		  result.symbol != NULL
-		  ? host_address_to_string (result.symbol)
-		  : "NULL");
-    }
+  symbol_lookup_debug_printf
+    ("found symbol @ %s (using lookup_static_symbol)",
+     result.symbol != NULL ? host_address_to_string (result.symbol) : "NULL");
   return result;
 }
 
@@ -2246,31 +2219,27 @@ lookup_symbol_in_block (const char *name, symbol_name_match_type match_type,
 {
   struct symbol *sym;
 
-  if (symbol_lookup_debug > 1)
+  if (symbol_lookup_debug)
     {
-      struct objfile *objfile = (block == nullptr
-				 ? nullptr : block_objfile (block));
+      struct objfile *objfile
+	= block == nullptr ? nullptr : block_objfile (block);
 
-      gdb_printf (gdb_stdlog,
-		  "lookup_symbol_in_block (%s, %s (objfile %s), %s)",
-		  name, host_address_to_string (block),
-		  objfile_debug_name (objfile),
-		  domain_name (domain));
+      symbol_lookup_debug_printf_v
+	("lookup_symbol_in_block (%s, %s (objfile %s), %s)",
+	 name, host_address_to_string (block),
+	 objfile != nullptr ? objfile_debug_name (objfile) : "NULL",
+	 domain_name (domain));
     }
 
   sym = block_lookup_symbol (block, name, match_type, domain);
   if (sym)
     {
-      if (symbol_lookup_debug > 1)
-	{
-	  gdb_printf (gdb_stdlog, " = %s\n",
-		      host_address_to_string (sym));
-	}
+      symbol_lookup_debug_printf_v ("lookup_symbol_in_block (...) = %s",
+				    host_address_to_string (sym));
       return fixup_symbol_section (sym, NULL);
     }
 
-  if (symbol_lookup_debug > 1)
-    gdb_printf (gdb_stdlog, " = NULL\n");
+  symbol_lookup_debug_printf_v ("lookup_symbol_in_block (...) = NULL");
   return NULL;
 }
 
@@ -2308,15 +2277,11 @@ lookup_symbol_in_objfile_symtabs (struct objfile *objfile,
 {
   gdb_assert (block_index == GLOBAL_BLOCK || block_index == STATIC_BLOCK);
 
-  if (symbol_lookup_debug > 1)
-    {
-      gdb_printf (gdb_stdlog,
-		  "lookup_symbol_in_objfile_symtabs (%s, %s, %s, %s)",
-		  objfile_debug_name (objfile),
-		  block_index == GLOBAL_BLOCK
-		  ? "GLOBAL_BLOCK" : "STATIC_BLOCK",
-		  name, domain_name (domain));
-    }
+  symbol_lookup_debug_printf_v
+    ("lookup_symbol_in_objfile_symtabs (%s, %s, %s, %s)",
+     objfile_debug_name (objfile),
+     block_index == GLOBAL_BLOCK ? "GLOBAL_BLOCK" : "STATIC_BLOCK",
+     name, domain_name (domain));
 
   struct block_symbol other;
   other.symbol = NULL;
@@ -2352,18 +2317,16 @@ lookup_symbol_in_objfile_symtabs (struct objfile *objfile,
 
   if (other.symbol != NULL)
     {
-      if (symbol_lookup_debug > 1)
-	{
-	  gdb_printf (gdb_stdlog, " = %s (block %s)\n",
-		      host_address_to_string (other.symbol),
-		      host_address_to_string (other.block));
-	}
+      symbol_lookup_debug_printf_v
+	("lookup_symbol_in_objfile_symtabs (...) = %s (block %s)",
+	 host_address_to_string (other.symbol),
+	 host_address_to_string (other.block));
       other.symbol = fixup_symbol_section (other.symbol, objfile);
       return other;
     }
 
-  if (symbol_lookup_debug > 1)
-    gdb_printf (gdb_stdlog, " = NULL\n");
+  symbol_lookup_debug_printf_v
+    ("lookup_symbol_in_objfile_symtabs (...) = NULL");
   return {};
 }
 
@@ -2438,24 +2401,17 @@ lookup_symbol_via_quick_fns (struct objfile *objfile,
   const struct block *block;
   struct block_symbol result;
 
-  if (symbol_lookup_debug > 1)
-    {
-      gdb_printf (gdb_stdlog,
-		  "lookup_symbol_via_quick_fns (%s, %s, %s, %s)\n",
-		  objfile_debug_name (objfile),
-		  block_index == GLOBAL_BLOCK
-		  ? "GLOBAL_BLOCK" : "STATIC_BLOCK",
-		  name, domain_name (domain));
-    }
+  symbol_lookup_debug_printf_v
+    ("lookup_symbol_via_quick_fns (%s, %s, %s, %s)",
+     objfile_debug_name (objfile),
+     block_index == GLOBAL_BLOCK ? "GLOBAL_BLOCK" : "STATIC_BLOCK",
+     name, domain_name (domain));
 
   cust = objfile->lookup_symbol (block_index, name, domain);
   if (cust == NULL)
     {
-      if (symbol_lookup_debug > 1)
-	{
-	  gdb_printf (gdb_stdlog,
-		      "lookup_symbol_via_quick_fns (...) = NULL\n");
-	}
+      symbol_lookup_debug_printf_v
+	("lookup_symbol_via_quick_fns (...) = NULL");
       return {};
     }
 
@@ -2466,13 +2422,10 @@ lookup_symbol_via_quick_fns (struct objfile *objfile,
   if (result.symbol == NULL)
     error_in_psymtab_expansion (block_index, name, cust);
 
-  if (symbol_lookup_debug > 1)
-    {
-      gdb_printf (gdb_stdlog,
-		  "lookup_symbol_via_quick_fns (...) = %s (block %s)\n",
-		  host_address_to_string (result.symbol),
-		  host_address_to_string (block));
-    }
+  symbol_lookup_debug_printf_v
+    ("lookup_symbol_via_quick_fns (...) = %s (block %s)",
+     host_address_to_string (result.symbol),
+     host_address_to_string (block));
 
   result.symbol = fixup_symbol_section (result.symbol, objfile);
   result.block = block;
@@ -2538,24 +2491,19 @@ lookup_symbol_in_static_block (const char *name,
       struct objfile *objfile = (block == nullptr
 				 ? nullptr : block_objfile (block));
 
-      gdb_printf (gdb_stdlog,
-		  "lookup_symbol_in_static_block (%s, %s (objfile %s),"
-		  " %s)\n",
-		  name,
-		  host_address_to_string (block),
-		  objfile_debug_name (objfile),
-		  domain_name (domain));
+      symbol_lookup_debug_printf
+	("lookup_symbol_in_static_block (%s, %s (objfile %s), %s)",
+	 name, host_address_to_string (block),
+	 objfile != nullptr ? objfile_debug_name (objfile) : "NULL",
+	 domain_name (domain));
     }
 
   sym = lookup_symbol_in_block (name,
 				symbol_name_match_type::FULL,
 				static_block, domain);
-  if (symbol_lookup_debug)
-    {
-      gdb_printf (gdb_stdlog,
-		  "lookup_symbol_in_static_block (...) = %s\n",
-		  sym != NULL ? host_address_to_string (sym) : "NULL");
-    }
+  symbol_lookup_debug_printf ("lookup_symbol_in_static_block (...) = %s",
+			      sym != NULL
+			      ? host_address_to_string (sym) : "NULL");
   return (struct block_symbol) {sym, static_block};
 }
 
@@ -2572,41 +2520,30 @@ lookup_symbol_in_objfile (struct objfile *objfile, enum block_enum block_index,
 
   gdb_assert (block_index == GLOBAL_BLOCK || block_index == STATIC_BLOCK);
 
-  if (symbol_lookup_debug)
-    {
-      gdb_printf (gdb_stdlog,
-		  "lookup_symbol_in_objfile (%s, %s, %s, %s)\n",
-		  objfile_debug_name (objfile),
-		  block_index == GLOBAL_BLOCK
-		  ? "GLOBAL_BLOCK" : "STATIC_BLOCK",
-		  name, domain_name (domain));
-    }
+  symbol_lookup_debug_printf ("lookup_symbol_in_objfile (%s, %s, %s, %s)",
+			      objfile_debug_name (objfile),
+			      block_index == GLOBAL_BLOCK
+			      ? "GLOBAL_BLOCK" : "STATIC_BLOCK",
+			      name, domain_name (domain));
 
   result = lookup_symbol_in_objfile_symtabs (objfile, block_index,
 					     name, domain);
   if (result.symbol != NULL)
     {
-      if (symbol_lookup_debug)
-	{
-	  gdb_printf (gdb_stdlog,
-		      "lookup_symbol_in_objfile (...) = %s"
-		      " (in symtabs)\n",
-		      host_address_to_string (result.symbol));
-	}
+      symbol_lookup_debug_printf
+	("lookup_symbol_in_objfile (...) = %s (in symtabs)",
+	 host_address_to_string (result.symbol));
       return result;
     }
 
   result = lookup_symbol_via_quick_fns (objfile, block_index,
 					name, domain);
-  if (symbol_lookup_debug)
-    {
-      gdb_printf (gdb_stdlog,
-		  "lookup_symbol_in_objfile (...) = %s%s\n",
-		  result.symbol != NULL
-		  ? host_address_to_string (result.symbol)
-		  : "NULL",
-		  result.symbol != NULL ? " (via quick fns)" : "");
-    }
+  symbol_lookup_debug_printf ("lookup_symbol_in_objfile (...) = %s%s",
+			      result.symbol != NULL
+			      ? host_address_to_string (result.symbol)
+			      : "NULL",
+			      result.symbol != NULL ? " (via quick fns)"
+			      : "");
   return result;
 }
 
@@ -4651,12 +4588,8 @@ treg_matches_sym_type_name (const compiled_regex &treg,
   struct type *sym_type;
   std::string printed_sym_type_name;
 
-  if (symbol_lookup_debug > 1)
-    {
-      gdb_printf (gdb_stdlog,
-		  "treg_matches_sym_type_name\n     sym %s\n",
-		  sym->natural_name ());
-    }
+  symbol_lookup_debug_printf_v ("treg_matches_sym_type_name, sym %s",
+				sym->natural_name ());
 
   sym_type = sym->type ();
   if (sym_type == NULL)
@@ -4668,14 +4601,8 @@ treg_matches_sym_type_name (const compiled_regex &treg,
     printed_sym_type_name = type_to_string (sym_type);
   }
 
-
-  if (symbol_lookup_debug > 1)
-    {
-      gdb_printf (gdb_stdlog,
-		  "     sym_type_name %s\n",
-		  printed_sym_type_name.c_str ());
-    }
-
+  symbol_lookup_debug_printf_v ("sym_type_name %s",
+				printed_sym_type_name.c_str ());
 
   if (printed_sym_type_name.empty ())
     return false;
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 4f3e84bbbe9..6eca61a759a 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2625,6 +2625,18 @@ extern unsigned int symtab_create_debug;
 
 extern unsigned int symbol_lookup_debug;
 
+/* Print a "symbol-lookup" debug statement if symbol_lookup_debug is >= 1.  */
+
+#define symbol_lookup_debug_printf(fmt, ...) \
+  debug_prefixed_printf_cond (symbol_lookup_debug >= 1, "symbol-lookup", fmt, \
+			      ##__VA_ARGS__)
+
+/* Print a "symbol-lookup" debug statement if symbol_lookup_debug is >= 2.  */
+
+#define symbol_lookup_debug_printf_v(fmt, ...) \
+  debug_prefixed_printf_cond (symbol_lookup_debug >= 2, "symbol-lookup", fmt, \
+			      ##__VA_ARGS__)
+
 extern bool basenames_may_differ;
 
 bool compare_filenames_for_search (const char *filename,
-- 
2.25.4

Re: [PATCH 1/2] gdb: convert 'set debug symbol-lookup' to new debug printing scheme

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> Convert the implementation of 'set debug symbol-lookup' to the new
Andrew> debug printing scheme.

Andrew> In a few places I've updated the debug output to remove places where
Andrew> the printed debug message included the function name, the new debug
Andrew> scheme already adds that, but I haven't done all the possible updates.

This seems fine to me.  Thanks.

Tom

[PATCH 2/2] gdb: add SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT

[Andrew Burgess]

After the previous commit converted symbol lookup debug to use the new
debug scheme, this commit adds SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT.

The previous commit updated 'set debug symbol-lookup' to use the new
debug scheme, however SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT still does
not exist.  The reason for this is that 'set debug symbol-lookup'
controls an integer variable, rather than a bool, and, depending on
the value of this variable, different amounts of debug will be
produced.

Currently the *_SCOPED_DEBUG_ENTER_EXIT mechanism will only work for
control variables of type bool, this is because the underlying
scoped_debug_start_end class can only handle variables of type bool.

This commit templates scoped_debug_start_end so that the class can
accept either a 'bool &' or an invokable object, e.g. a lambda
function, or a function pointer.

The existing scoped_debug_start_end and scoped_debug_enter_exit macros
in common-debug.h are updated to support scoped_debug_enter_exit being
templated, however, nothing outside of common-debug.h needs to change.

I've then added SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT in symtab.h, and
added a couple of token uses in symtab.c.  I didn't want to add too
much in this first commit, this is really about updating
common-debug.h to support this new ability.
---
 gdb/symtab.c              |  4 ++
 gdb/symtab.h              |  7 +++
 gdbsupport/common-debug.h | 92 +++++++++++++++++++++++++++++++--------
 3 files changed, 84 insertions(+), 19 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 14d81f5468d..a7a54159b6d 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -1950,6 +1950,8 @@ lookup_symbol_in_language (const char *name, const struct block *block,
 			   const domain_enum domain, enum language lang,
 			   struct field_of_this_result *is_a_field_of_this)
 {
+  SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT;
+
   demangle_result_storage storage;
   const char *modified_name = demangle_for_lookup (name, lang, storage);
 
@@ -2072,6 +2074,8 @@ lookup_symbol_aux (const char *name, symbol_name_match_type match_type,
 		   const domain_enum domain, enum language language,
 		   struct field_of_this_result *is_a_field_of_this)
 {
+  SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT;
+
   struct block_symbol result;
   const struct language_defn *langdef;
 
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 6eca61a759a..2d1f0cec877 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2637,6 +2637,13 @@ extern unsigned int symbol_lookup_debug;
   debug_prefixed_printf_cond (symbol_lookup_debug >= 2, "symbol-lookup", fmt, \
 			      ##__VA_ARGS__)
 
+/* Print "symbol-lookup" enter/exit debug statements.  */
+
+#define SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT \
+  scoped_debug_enter_exit ([&] () -> bool { \
+    return symbol_lookup_debug > 0; \
+  }, "symbol-lookup")
+
 extern bool basenames_may_differ;
 
 bool compare_filenames_for_search (const char *filename,
diff --git a/gdbsupport/common-debug.h b/gdbsupport/common-debug.h
index 00a1e1599bc..904c1a1ea24 100644
--- a/gdbsupport/common-debug.h
+++ b/gdbsupport/common-debug.h
@@ -88,6 +88,7 @@ extern int debug_print_depth;
    it on destruction, such that nested debug statements will be printed with
    an indent and appear "inside" this one.  */
 
+template<typename PT>
 struct scoped_debug_start_end
 {
   /* DEBUG_ENABLED is a reference to a variable that indicates whether debugging
@@ -95,35 +96,35 @@ struct scoped_debug_start_end
      separately at construction and destruction, such that the start statement
      could be printed but not the end statement, or vice-versa.
 
+     DEBUG_ENABLED should either be of type 'bool &' or should be a type
+     that can be invoked.
+
      MODULE and FUNC are forwarded to debug_prefixed_printf.
 
      START_PREFIX and END_PREFIX are the statements to print on construction and
      destruction, respectively.
 
      If the FMT format string is non-nullptr, then a `: ` is appended to the
-     messages, followed by the rendering of that format string.  The format
-     string is rendered during construction and is re-used as is for the
-     message on exit.  */
+     messages, followed by the rendering of that format string with ARGS.
+     The format string is rendered during construction and is re-used as is
+     for the message on exit.  */
 
-  scoped_debug_start_end (bool &debug_enabled, const char *module,
+  scoped_debug_start_end (PT &debug_enabled, const char *module,
 			  const char *func, const char *start_prefix,
-			  const char *end_prefix, const char *fmt, ...)
-    ATTRIBUTE_NULL_PRINTF (7, 8)
+			  const char *end_prefix, const char *fmt,
+			  va_list args)
+    ATTRIBUTE_NULL_PRINTF (7, 0)
     : m_debug_enabled (debug_enabled),
       m_module (module),
       m_func (func),
       m_end_prefix (end_prefix),
       m_with_format (fmt != nullptr)
   {
-    if (m_debug_enabled)
+    if (is_debug_enabled ())
       {
 	if (fmt != nullptr)
 	  {
-	    va_list args;
-	    va_start (args, fmt);
 	    m_msg = string_vprintf (fmt, args);
-	    va_end (args);
-
 	    debug_prefixed_printf (m_module, m_func, "%s: %s",
 				   start_prefix, m_msg->c_str ());
 	  }
@@ -137,6 +138,8 @@ struct scoped_debug_start_end
 
   DISABLE_COPY_AND_ASSIGN (scoped_debug_start_end);
 
+  scoped_debug_start_end (scoped_debug_start_end &&other) = default;
+
   ~scoped_debug_start_end ()
   {
     if (m_must_decrement_print_depth)
@@ -145,7 +148,7 @@ struct scoped_debug_start_end
 	--debug_print_depth;
       }
 
-    if (m_debug_enabled)
+    if (is_debug_enabled ())
       {
 	if (m_with_format)
 	  {
@@ -167,7 +170,16 @@ struct scoped_debug_start_end
   }
 
 private:
-  bool &m_debug_enabled;
+
+  /* This function is specialized based on the type PT.  Returns true if
+     M_DEBUG_ENABLED indicates this debug setting is enabled, otherwise,
+     return false.  */
+  bool is_debug_enabled () const;
+
+  /* Reference to the debug setting, or a callback that can read the debug
+     setting.  Access the value of this by calling IS_DEBUG_ENABLED.  */
+  PT &m_debug_enabled;
+
   const char *m_module;
   const char *m_func;
   const char *m_end_prefix;
@@ -184,18 +196,60 @@ struct scoped_debug_start_end
   bool m_must_decrement_print_depth = false;
 };
 
+/* Implementation of is_debug_enabled when PT is an invokable type.  */
+
+template<typename PT>
+inline bool
+scoped_debug_start_end<PT>::is_debug_enabled () const
+{
+  return m_debug_enabled ();
+}
+
+/* Implementation of is_debug_enabled when PT is 'bool &'.  */
+
+template<>
+inline bool
+scoped_debug_start_end<bool &>::is_debug_enabled () const
+{
+  return m_debug_enabled;
+}
+
+/* Wrapper around the scoped_debug_start_end constructor to allow the
+   caller to create an object using 'auto' type, the actual type will be
+   based on the type of the PRED argument.  All arguments are forwarded to
+   the scoped_debug_start_end constructor.  */
+
+template<typename PT>
+static inline scoped_debug_start_end<PT &> ATTRIBUTE_NULL_PRINTF (6, 7)
+make_scoped_debug_start_end (PT &&pred, const char *module, const char *func,
+			     const char *start_prefix,
+			     const char *end_prefix, const char *fmt, ...)
+{
+  va_list args;
+  va_start (args, fmt);
+  auto res = scoped_debug_start_end<PT &> (pred, module, func, start_prefix,
+					   end_prefix, fmt, args);
+  va_end (args);
+
+  return res;
+}
+
 /* Helper to define a module-specific start/end debug macro.  */
 
-#define scoped_debug_start_end(debug_enabled, module, fmt, ...) \
-  scoped_debug_start_end CONCAT(scoped_debug_start_end, __LINE__) \
-    (debug_enabled, module, __func__, "start", "end", fmt, ##__VA_ARGS__)
+#define scoped_debug_start_end(debug_enabled, module, fmt, ...)		\
+  auto CONCAT(scoped_debug_start_end, __LINE__)				\
+    = make_scoped_debug_start_end (debug_enabled, module, 	\
+				   __func__, "start", "end",	\
+				   fmt, ##__VA_ARGS__)
 
 /* Helper to define a module-specific enter/exit debug macro.  This is a special
    case of `scoped_debug_start_end` where the start and end messages are "enter"
    and "exit", to denote entry and exit of a function.  */
 
-#define scoped_debug_enter_exit(debug_enabled, module) \
-  scoped_debug_start_end CONCAT(scoped_debug_start_end, __LINE__) \
-    (debug_enabled, module, __func__, "enter", "exit", nullptr)
+#define scoped_debug_enter_exit(debug_enabled, module)	\
+  auto CONCAT(scoped_debug_start_end, __LINE__)				\
+    = make_scoped_debug_start_end (debug_enabled, module, 	\
+				   __func__, "enter", "exit",	\
+				   nullptr)
 
 #endif /* COMMON_COMMON_DEBUG_H */
-- 
2.25.4

Re: [PATCH 2/2] gdb: add SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> After the previous commit converted symbol lookup debug to use the new
Andrew> debug scheme, this commit adds SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT.

Andrew> The previous commit updated 'set debug symbol-lookup' to use the new
Andrew> debug scheme, however SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT still does
Andrew> not exist.  The reason for this is that 'set debug symbol-lookup'
Andrew> controls an integer variable, rather than a bool, and, depending on
Andrew> the value of this variable, different amounts of debug will be
Andrew> produced.

Andrew> Currently the *_SCOPED_DEBUG_ENTER_EXIT mechanism will only work for
Andrew> control variables of type bool, this is because the underlying
Andrew> scoped_debug_start_end class can only handle variables of type bool.

Andrew> This commit templates scoped_debug_start_end so that the class can
Andrew> accept either a 'bool &' or an invokable object, e.g. a lambda
Andrew> function, or a function pointer.

Andrew> The existing scoped_debug_start_end and scoped_debug_enter_exit macros
Andrew> in common-debug.h are updated to support scoped_debug_enter_exit being
Andrew> templated, however, nothing outside of common-debug.h needs to change.

Andrew> I've then added SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT in symtab.h, and
Andrew> added a couple of token uses in symtab.c.  I didn't want to add too
Andrew> much in this first commit, this is really about updating
Andrew> common-debug.h to support this new ability.

This seems fine.  I suppose inlining will make it not cost too much... ?

Andrew> +#define SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT \
Andrew> +  scoped_debug_enter_exit ([&] () -> bool { \
Andrew> +    return symbol_lookup_debug > 0; \
Andrew> +  }, "symbol-lookup")

I wonder whether the [&] is needed here.  I couldn't see what it is
capturing.

Tom

Re: [PATCH 2/2] gdb: add SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT

[Simon Marchi]

On 12/9/22 12:42, Tom Tromey wrote:
>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Andrew> After the previous commit converted symbol lookup debug to use the new
> Andrew> debug scheme, this commit adds SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT.
> 
> Andrew> The previous commit updated 'set debug symbol-lookup' to use the new
> Andrew> debug scheme, however SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT still does
> Andrew> not exist.  The reason for this is that 'set debug symbol-lookup'
> Andrew> controls an integer variable, rather than a bool, and, depending on
> Andrew> the value of this variable, different amounts of debug will be
> Andrew> produced.
> 
> Andrew> Currently the *_SCOPED_DEBUG_ENTER_EXIT mechanism will only work for
> Andrew> control variables of type bool, this is because the underlying
> Andrew> scoped_debug_start_end class can only handle variables of type bool.
> 
> Andrew> This commit templates scoped_debug_start_end so that the class can
> Andrew> accept either a 'bool &' or an invokable object, e.g. a lambda
> Andrew> function, or a function pointer.
> 
> Andrew> The existing scoped_debug_start_end and scoped_debug_enter_exit macros
> Andrew> in common-debug.h are updated to support scoped_debug_enter_exit being
> Andrew> templated, however, nothing outside of common-debug.h needs to change.
> 
> Andrew> I've then added SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT in symtab.h, and
> Andrew> added a couple of token uses in symtab.c.  I didn't want to add too
> Andrew> much in this first commit, this is really about updating
> Andrew> common-debug.h to support this new ability.
> 
> This seems fine.  I suppose inlining will make it not cost too much... ?
> 
> Andrew> +#define SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT \
> Andrew> +  scoped_debug_enter_exit ([&] () -> bool { \
> Andrew> +    return symbol_lookup_debug > 0; \
> Andrew> +  }, "symbol-lookup")
> 
> I wonder whether the [&] is needed here.  I couldn't see what it is
> capturing.

Same comment here.  Instead of a lambda, you could have single function:

static bool
symbol_lookup_debug_enabled ()
{
  return symbol_lookup_debug > 0;
}

And then you could use it in symbol_lookup_debug_printf too, to factor
the logic.  You could have symbol_lookup_debug_v_enabled too for
symmetry if you wanted.

Otherwise, it all LGTM:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH 2/2] gdb: add SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 12/9/22 12:42, Tom Tromey wrote:
>>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>> 
>> Andrew> After the previous commit converted symbol lookup debug to use the new
>> Andrew> debug scheme, this commit adds SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT.
>> 
>> Andrew> The previous commit updated 'set debug symbol-lookup' to use the new
>> Andrew> debug scheme, however SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT still does
>> Andrew> not exist.  The reason for this is that 'set debug symbol-lookup'
>> Andrew> controls an integer variable, rather than a bool, and, depending on
>> Andrew> the value of this variable, different amounts of debug will be
>> Andrew> produced.
>> 
>> Andrew> Currently the *_SCOPED_DEBUG_ENTER_EXIT mechanism will only work for
>> Andrew> control variables of type bool, this is because the underlying
>> Andrew> scoped_debug_start_end class can only handle variables of type bool.
>> 
>> Andrew> This commit templates scoped_debug_start_end so that the class can
>> Andrew> accept either a 'bool &' or an invokable object, e.g. a lambda
>> Andrew> function, or a function pointer.
>> 
>> Andrew> The existing scoped_debug_start_end and scoped_debug_enter_exit macros
>> Andrew> in common-debug.h are updated to support scoped_debug_enter_exit being
>> Andrew> templated, however, nothing outside of common-debug.h needs to change.
>> 
>> Andrew> I've then added SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT in symtab.h, and
>> Andrew> added a couple of token uses in symtab.c.  I didn't want to add too
>> Andrew> much in this first commit, this is really about updating
>> Andrew> common-debug.h to support this new ability.
>> 
>> This seems fine.  I suppose inlining will make it not cost too much... ?
>> 
>> Andrew> +#define SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT \
>> Andrew> +  scoped_debug_enter_exit ([&] () -> bool { \
>> Andrew> +    return symbol_lookup_debug > 0; \
>> Andrew> +  }, "symbol-lookup")
>> 
>> I wonder whether the [&] is needed here.  I couldn't see what it is
>> capturing.
>
> Same comment here.  Instead of a lambda, you could have single function:
>
> static bool
> symbol_lookup_debug_enabled ()
> {
>   return symbol_lookup_debug > 0;
> }

Thanks for the feedback.  I did originally have a free function here,
but wanted to check the code worked fine with a lambda too ... and it
just got left like that.

I'll switch it back to a free function before committing.

Thanks,
Andrew

>
> And then you could use it in symbol_lookup_debug_printf too, to factor
> the logic.  You could have symbol_lookup_debug_v_enabled too for
> symmetry if you wanted.
>
> Otherwise, it all LGTM:
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
>
> Simon

Re: [PATCH 2/2] gdb: add SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> After the previous commit converted symbol lookup debug to use the new
> Andrew> debug scheme, this commit adds SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT.
>
> Andrew> The previous commit updated 'set debug symbol-lookup' to use the new
> Andrew> debug scheme, however SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT still does
> Andrew> not exist.  The reason for this is that 'set debug symbol-lookup'
> Andrew> controls an integer variable, rather than a bool, and, depending on
> Andrew> the value of this variable, different amounts of debug will be
> Andrew> produced.
>
> Andrew> Currently the *_SCOPED_DEBUG_ENTER_EXIT mechanism will only work for
> Andrew> control variables of type bool, this is because the underlying
> Andrew> scoped_debug_start_end class can only handle variables of type bool.
>
> Andrew> This commit templates scoped_debug_start_end so that the class can
> Andrew> accept either a 'bool &' or an invokable object, e.g. a lambda
> Andrew> function, or a function pointer.
>
> Andrew> The existing scoped_debug_start_end and scoped_debug_enter_exit macros
> Andrew> in common-debug.h are updated to support scoped_debug_enter_exit being
> Andrew> templated, however, nothing outside of common-debug.h needs to change.
>
> Andrew> I've then added SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT in symtab.h, and
> Andrew> added a couple of token uses in symtab.c.  I didn't want to add too
> Andrew> much in this first commit, this is really about updating
> Andrew> common-debug.h to support this new ability.
>
> This seems fine.  I suppose inlining will make it not cost too much... ?

I'm not particularly worried about the additional cost.  There's already
plenty of debug printing done in the symbol lookup path, and like you
say, with inlining, I would expect most of the debug checks to be folded
together.

If this does prove to be problematic, then I'm happy for some/all of
these to be pulled out later.  For me, the biggest win here, is having
the ability to add things like SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT in
GDB - even if I just add these temporarily while I'm debugging a
particular issue.

>
> Andrew> +#define SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT \
> Andrew> +  scoped_debug_enter_exit ([&] () -> bool { \
> Andrew> +    return symbol_lookup_debug > 0; \
> Andrew> +  }, "symbol-lookup")
>
> I wonder whether the [&] is needed here.  I couldn't see what it is
> capturing.

In the end I took Simon's suggestion and switched to using a global
function instead of the lambda, though scoped_debug_enter_exit will
still accept a lambda if that's ever wanted elsewhere.  The updated
patch is below.

I've now pushed both these patches.

Thanks,
Andrew

--

commit 2698da268bdd0b4a6815a15b41a42bac5f928ca7
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Tue Dec 6 12:49:55 2022 +0000

    gdb: add SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT
    
    After the previous commit converted symbol-lookup debug to use the new
    debug scheme, this commit adds SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT.
    
    The previous commit didn't add SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT
    because symbol-lookup debug is controlled by an 'unsigned int' rather
    than a 'bool' control variable, we use the numeric value to offer
    different levels of verbosity for symbol-lookup debug.
    
    The *_SCOPED_DEBUG_ENTER_EXIT mechanism currently relies on capturing
    a reference to the bool control variable, and evaluating the variable
    both on entry, and at exit, this is done in the scoped_debug_start_end
    class (see gdbsupport/common-debug.h).
    
    This commit templates scoped_debug_start_end so that the class can
    accept either a 'bool &' or an invokable object, e.g. a lambda
    function, or a function pointer.
    
    The existing scoped_debug_start_end and scoped_debug_enter_exit macros
    in common-debug.h are updated to support scoped_debug_enter_exit being
    templated, however, nothing outside of common-debug.h needs to change.
    
    I've then added SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT in symtab.h, and
    added a couple of token uses in symtab.c.  I didn't want to add too
    much in this first commit, this is really about updating
    common-debug.h to support this new functionality.
    
    Within symtab.h I created a couple of global functions that can be
    used to query the status of the symbol_lookup_debug control variable,
    these functions are then used within the two existing macros:
    
      symbol_lookup_debug_printf
      symbol_lookup_debug_printf_v
    
    and also in the new SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT macro.

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 14d81f5468d..a7a54159b6d 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -1950,6 +1950,8 @@ lookup_symbol_in_language (const char *name, const struct block *block,
 			   const domain_enum domain, enum language lang,
 			   struct field_of_this_result *is_a_field_of_this)
 {
+  SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT;
+
   demangle_result_storage storage;
   const char *modified_name = demangle_for_lookup (name, lang, storage);
 
@@ -2072,6 +2074,8 @@ lookup_symbol_aux (const char *name, symbol_name_match_type match_type,
 		   const domain_enum domain, enum language language,
 		   struct field_of_this_result *is_a_field_of_this)
 {
+  SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT;
+
   struct block_symbol result;
   const struct language_defn *langdef;
 
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 6eca61a759a..d0fa3b11c77 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2625,17 +2625,38 @@ extern unsigned int symtab_create_debug;
 
 extern unsigned int symbol_lookup_debug;
 
+/* Return true if symbol-lookup debug is turned on at all.  */
+
+static inline bool
+symbol_lookup_debug_enabled ()
+{
+  return symbol_lookup_debug > 0;
+}
+
+/* Return true if symbol-lookup debug is turned to verbose mode.  */
+
+static inline bool
+symbol_lookup_debug_enabled_v ()
+{
+  return symbol_lookup_debug > 1;
+}
+
 /* Print a "symbol-lookup" debug statement if symbol_lookup_debug is >= 1.  */
 
 #define symbol_lookup_debug_printf(fmt, ...) \
-  debug_prefixed_printf_cond (symbol_lookup_debug >= 1, "symbol-lookup", fmt, \
-			      ##__VA_ARGS__)
+  debug_prefixed_printf_cond (symbol_lookup_debug_enabled (),	\
+			      "symbol-lookup", fmt, ##__VA_ARGS__)
 
 /* Print a "symbol-lookup" debug statement if symbol_lookup_debug is >= 2.  */
 
 #define symbol_lookup_debug_printf_v(fmt, ...) \
-  debug_prefixed_printf_cond (symbol_lookup_debug >= 2, "symbol-lookup", fmt, \
-			      ##__VA_ARGS__)
+  debug_prefixed_printf_cond (symbol_lookup_debug_enabled_v (), \
+			      "symbol-lookup", fmt, ##__VA_ARGS__)
+
+/* Print "symbol-lookup" enter/exit debug statements.  */
+
+#define SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT \
+  scoped_debug_enter_exit (symbol_lookup_debug_enabled, "symbol-lookup")
 
 extern bool basenames_may_differ;
 
diff --git a/gdbsupport/common-debug.h b/gdbsupport/common-debug.h
index 00a1e1599bc..904c1a1ea24 100644
--- a/gdbsupport/common-debug.h
+++ b/gdbsupport/common-debug.h
@@ -88,6 +88,7 @@ extern int debug_print_depth;
    it on destruction, such that nested debug statements will be printed with
    an indent and appear "inside" this one.  */
 
+template<typename PT>
 struct scoped_debug_start_end
 {
   /* DEBUG_ENABLED is a reference to a variable that indicates whether debugging
@@ -95,35 +96,35 @@ struct scoped_debug_start_end
      separately at construction and destruction, such that the start statement
      could be printed but not the end statement, or vice-versa.
 
+     DEBUG_ENABLED should either be of type 'bool &' or should be a type
+     that can be invoked.
+
      MODULE and FUNC are forwarded to debug_prefixed_printf.
 
      START_PREFIX and END_PREFIX are the statements to print on construction and
      destruction, respectively.
 
      If the FMT format string is non-nullptr, then a `: ` is appended to the
-     messages, followed by the rendering of that format string.  The format
-     string is rendered during construction and is re-used as is for the
-     message on exit.  */
+     messages, followed by the rendering of that format string with ARGS.
+     The format string is rendered during construction and is re-used as is
+     for the message on exit.  */
 
-  scoped_debug_start_end (bool &debug_enabled, const char *module,
+  scoped_debug_start_end (PT &debug_enabled, const char *module,
 			  const char *func, const char *start_prefix,
-			  const char *end_prefix, const char *fmt, ...)
-    ATTRIBUTE_NULL_PRINTF (7, 8)
+			  const char *end_prefix, const char *fmt,
+			  va_list args)
+    ATTRIBUTE_NULL_PRINTF (7, 0)
     : m_debug_enabled (debug_enabled),
       m_module (module),
       m_func (func),
       m_end_prefix (end_prefix),
       m_with_format (fmt != nullptr)
   {
-    if (m_debug_enabled)
+    if (is_debug_enabled ())
       {
 	if (fmt != nullptr)
 	  {
-	    va_list args;
-	    va_start (args, fmt);
 	    m_msg = string_vprintf (fmt, args);
-	    va_end (args);
-
 	    debug_prefixed_printf (m_module, m_func, "%s: %s",
 				   start_prefix, m_msg->c_str ());
 	  }
@@ -137,6 +138,8 @@ struct scoped_debug_start_end
 
   DISABLE_COPY_AND_ASSIGN (scoped_debug_start_end);
 
+  scoped_debug_start_end (scoped_debug_start_end &&other) = default;
+
   ~scoped_debug_start_end ()
   {
     if (m_must_decrement_print_depth)
@@ -145,7 +148,7 @@ struct scoped_debug_start_end
 	--debug_print_depth;
       }
 
-    if (m_debug_enabled)
+    if (is_debug_enabled ())
       {
 	if (m_with_format)
 	  {
@@ -167,7 +170,16 @@ struct scoped_debug_start_end
   }
 
 private:
-  bool &m_debug_enabled;
+
+  /* This function is specialized based on the type PT.  Returns true if
+     M_DEBUG_ENABLED indicates this debug setting is enabled, otherwise,
+     return false.  */
+  bool is_debug_enabled () const;
+
+  /* Reference to the debug setting, or a callback that can read the debug
+     setting.  Access the value of this by calling IS_DEBUG_ENABLED.  */
+  PT &m_debug_enabled;
+
   const char *m_module;
   const char *m_func;
   const char *m_end_prefix;
@@ -184,18 +196,60 @@ struct scoped_debug_start_end
   bool m_must_decrement_print_depth = false;
 };
 
+/* Implementation of is_debug_enabled when PT is an invokable type.  */
+
+template<typename PT>
+inline bool
+scoped_debug_start_end<PT>::is_debug_enabled () const
+{
+  return m_debug_enabled ();
+}
+
+/* Implementation of is_debug_enabled when PT is 'bool &'.  */
+
+template<>
+inline bool
+scoped_debug_start_end<bool &>::is_debug_enabled () const
+{
+  return m_debug_enabled;
+}
+
+/* Wrapper around the scoped_debug_start_end constructor to allow the
+   caller to create an object using 'auto' type, the actual type will be
+   based on the type of the PRED argument.  All arguments are forwarded to
+   the scoped_debug_start_end constructor.  */
+
+template<typename PT>
+static inline scoped_debug_start_end<PT &> ATTRIBUTE_NULL_PRINTF (6, 7)
+make_scoped_debug_start_end (PT &&pred, const char *module, const char *func,
+			     const char *start_prefix,
+			     const char *end_prefix, const char *fmt, ...)
+{
+  va_list args;
+  va_start (args, fmt);
+  auto res = scoped_debug_start_end<PT &> (pred, module, func, start_prefix,
+					   end_prefix, fmt, args);
+  va_end (args);
+
+  return res;
+}
+
 /* Helper to define a module-specific start/end debug macro.  */
 
-#define scoped_debug_start_end(debug_enabled, module, fmt, ...) \
-  scoped_debug_start_end CONCAT(scoped_debug_start_end, __LINE__) \
-    (debug_enabled, module, __func__, "start", "end", fmt, ##__VA_ARGS__)
+#define scoped_debug_start_end(debug_enabled, module, fmt, ...)		\
+  auto CONCAT(scoped_debug_start_end, __LINE__)				\
+    = make_scoped_debug_start_end (debug_enabled, module, 	\
+				   __func__, "start", "end",	\
+				   fmt, ##__VA_ARGS__)
 
 /* Helper to define a module-specific enter/exit debug macro.  This is a special
    case of `scoped_debug_start_end` where the start and end messages are "enter"
    and "exit", to denote entry and exit of a function.  */
 
-#define scoped_debug_enter_exit(debug_enabled, module) \
-  scoped_debug_start_end CONCAT(scoped_debug_start_end, __LINE__) \
-    (debug_enabled, module, __func__, "enter", "exit", nullptr)
+#define scoped_debug_enter_exit(debug_enabled, module)	\
+  auto CONCAT(scoped_debug_start_end, __LINE__)				\
+    = make_scoped_debug_start_end (debug_enabled, module, 	\
+				   __func__, "enter", "exit",	\
+				   nullptr)
 
 #endif /* COMMON_COMMON_DEBUG_H */