[PATCH] Fix Ada crash with .debug_names

[PATCH] Fix Ada crash with .debug_names

[Tom Tromey]

PR ada/25837 points out a crash in the gdb testsuite when .debug_names
is used.  You can reproduce like:

    runtest --target_board=cc-with-debug-names \
        gdb.ada/big_packed_array.exp

The bug was introduced by commit e0802d599 ("Avoid copying in
lookup_name_info").  The problem is that the return type of
language_lookup_name changed, but in a way that didn't cause existing
callers to trigger a compilation error.  So, the matcher cache in
dw2_expand_symtabs_matching_symbol, which stored a "const string &",
would end up with invalid data.

This patch fixes the problem by updating the callers to use the new
type.

Tested on x86-64 Fedora 30.

gdb/ChangeLog
2020-04-21  Tom Tromey  <tromey@adacore.com>

	PR ada/25837:
	* dwarf2/read.c (dw2_expand_symtabs_matching_symbol): Store a
	"const char *", not a "const std::string &".
	<name_and_matcher::operator==>: Update.
	* unittests/lookup_name_info-selftests.c: Change type of
	"result".
---
 gdb/ChangeLog                              | 9 +++++++++
 gdb/dwarf2/read.c                          | 4 ++--
 gdb/unittests/lookup_name_info-selftests.c | 6 +++---
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 41db511c851..2e21e801380 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -3911,11 +3911,11 @@ dw2_expand_symtabs_matching_symbol
   struct name_and_matcher
   {
     symbol_name_matcher_ftype *matcher;
-    const std::string &name;
+    const char *name;
 
     bool operator== (const name_and_matcher &other) const
     {
-      return matcher == other.matcher && name == other.name;
+      return matcher == other.matcher && strcmp (name, other.name) == 0;
     }
   };
 
diff --git a/gdb/unittests/lookup_name_info-selftests.c b/gdb/unittests/lookup_name_info-selftests.c
index 002fc697955..6a617521cb4 100644
--- a/gdb/unittests/lookup_name_info-selftests.c
+++ b/gdb/unittests/lookup_name_info-selftests.c
@@ -37,14 +37,14 @@ check_make_paramless (const char *file, int line,
 {
   lookup_name_info lookup_name (name, symbol_name_match_type::FULL,
 				completion_mode, true /* ignore_parameters */);
-  const std::string &result = lookup_name.language_lookup_name (lang);
+  const char *result = lookup_name.language_lookup_name (lang);
 
-  if (result != expected)
+  if (strcmp (result, expected) != 0)
     {
       error (_("%s:%d: make-paramless self-test failed: (completion=%d, lang=%d) "
 	       "\"%s\" -> \"%s\", expected \"%s\""),
 	     file, line, completion_mode, lang, name,
-	     result.c_str (), expected);
+	     result, expected);
     }
 }
 
-- 
2.21.1

Re: [PATCH] Fix Ada crash with .debug_names

[Tom de Vries]

On 21-04-2020 17:23, Tom Tromey wrote:
> PR ada/25837 points out a crash in the gdb testsuite when .debug_names
> is used.  You can reproduce like:
> 
>     runtest --target_board=cc-with-debug-names \
>         gdb.ada/big_packed_array.exp
> 
> The bug was introduced by commit e0802d599 ("Avoid copying in
> lookup_name_info").  The problem is that the return type of
> language_lookup_name changed, but in a way that didn't cause existing
> callers to trigger a compilation error.  So, the matcher cache in
> dw2_expand_symtabs_matching_symbol, which stored a "const string &",
> would end up with invalid data.
> 

Hi,

it took me a while to realize that the problem is that the changed
return type of language_lookup_name caused a std::string copy
constructor to be invoked here:
...
      name_and_matcher key {
         name_matcher,
         lookup_name_without_params.language_lookup_name (lang_e)
      };
...
creating a temporary that has the lifetime of the loop body, in other
words, it'll be destroyed at the end of the loop body scope, while still
being referenced after the loop from the std::vector where we store "key".

Perhaps the log message could be extended to clarify that bit.

Otherwise, I've tested this with target board cc-with-debug-names, and
did not spot any new FAILs.

Thanks,
- Tom

---

$ cat test.c
#include <string>
#include <vector>
#include <iostream>
#include <string.h>

struct name_and_matcher
{
#if FIX
  const char *name;
#else
  const std::string &name;
#endif

  bool operator== (const name_and_matcher &other) const
  {
    bool res;
#if FIX
    res = strcmp (name, other.name) == 0;
#else
    res = name == other.name;
#endif
    return res;
  }
};

int
main (void)
{
  std::vector<name_and_matcher> matchers;

  const char *str[] = {
    "itsy", "bitsy", "spider"
  };

  int i;
  for (i = 0; i < 3; ++i)
    {
      name_and_matcher key {
         str[i]
      };

      matchers.push_back (std::move (key));
    }

  for(std::vector<name_and_matcher>::iterator it = matchers.begin ();
      it != matchers.end(); ++it)
    std::cout << it->name << "\n";

  return 0;
}

---

$ g++ test.c -O0 -DFIX=0
$ ./a.out
spider
spider
spider

---

$ g++ test.c -O0 -DFIX=1
itsy
bitsy
spider

Re: [PATCH] Fix Ada crash with .debug_names

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> it took me a while to realize that the problem is that the changed
Tom> return type of language_lookup_name caused a std::string copy
Tom> constructor to be invoked here:

Haha, me too!

Tom> Perhaps the log message could be extended to clarify that bit.

Good idea.  I'm going to check it in with that change.

Tom