[PATCH] Delete test target descriptions when exiting

[PATCH] Delete test target descriptions when exiting

[Simon Marchi]

Looking at the address sanitizer output, this was a quite low hanging
fruit.  We create target_desc objects for testing that we never free.
Saving them in unique_ptrs takes care of it.

I created a small struct to hold these because I thought it would help
readability.

gdb/ChangeLog:

	* target-descriptions.c (struct xml_test_tdesc): New.
	(xml_tdesc): Change type to std::vector<xml_test_tdesc>.
	(record_xml_tdesc): Update.
	(maintenance_check_xml_descriptions): Update.
	* target-descriptions.h (record_xml_tdesc): Update comment.
---
 gdb/target-descriptions.c | 26 +++++++++++++++++++-------
 gdb/target-descriptions.h |  2 +-
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index a96416c..087de14 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -1734,7 +1734,19 @@ maint_print_c_tdesc_cmd (const char *args, int from_tty)
 
 namespace selftests {
 
-static std::vector<std::pair<const char*, const target_desc *>> xml_tdesc;
+/* A reference target description, used for testing (see record_xml_tdesc).  */
+
+struct xml_test_tdesc
+{
+  xml_test_tdesc (const char *name, std::unique_ptr<const target_desc> &&tdesc)
+    : name (name), tdesc (std::move (tdesc))
+  {}
+
+  const char *name;
+  std::unique_ptr<const target_desc> tdesc;
+};
+
+static std::vector<xml_test_tdesc> xml_tdesc;
 
 #if GDB_SELF_TEST
 
@@ -1743,7 +1755,7 @@ static std::vector<std::pair<const char*, const target_desc *>> xml_tdesc;
 void
 record_xml_tdesc (const char *xml_file, const struct target_desc *tdesc)
 {
-  xml_tdesc.emplace_back (xml_file, tdesc);
+  xml_tdesc.emplace_back (xml_file, std::unique_ptr<const target_desc> (tdesc));
 }
 #endif
 
@@ -1798,17 +1810,17 @@ maintenance_check_xml_descriptions (const char *dir, int from_tty)
 
   for (auto const &e : selftests::xml_tdesc)
     {
-      std::string tdesc_xml = (feature_dir + SLASH_STRING + e.first);
+      std::string tdesc_xml = (feature_dir + SLASH_STRING + e.name);
       const target_desc *tdesc
 	= file_read_description_xml (tdesc_xml.data ());
 
-      if (tdesc == NULL || *tdesc != *e.second)
+      if (tdesc == NULL || *tdesc != *e.tdesc)
 	{
-	  printf_filtered ( _("Descriptions for %s do not match.\n"), e.first);
+	  printf_filtered ( _("Descriptions for %s do not match.\n"), e.name);
 	  failed++;
 	}
-      else if (!maintenance_check_tdesc_xml_convert (tdesc, e.first)
-	       || !maintenance_check_tdesc_xml_convert (e.second, e.first))
+      else if (!maintenance_check_tdesc_xml_convert (tdesc, e.name)
+	       || !maintenance_check_tdesc_xml_convert (e.tdesc.get (), e.name))
 	failed++;
     }
   printf_filtered (_("Tested %lu XML files, %d failed\n"),
diff --git a/gdb/target-descriptions.h b/gdb/target-descriptions.h
index 87403ac..96290b7 100644
--- a/gdb/target-descriptions.h
+++ b/gdb/target-descriptions.h
@@ -215,7 +215,7 @@ namespace selftests {
 
 /* Record that XML_FILE should generate a target description that equals
    TDESC, to be verified by the "maintenance check xml-descriptions"
-   command.  */
+   command.  This function takes ownership of TDESC.  */
 
 void record_xml_tdesc (const char *xml_file,
 		       const struct target_desc *tdesc);
-- 
2.7.4

Re: [PATCH] Delete test target descriptions when exiting

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:

Simon> Looking at the address sanitizer output, this was a quite low hanging
Simon> fruit.  We create target_desc objects for testing that we never free.
Simon> Saving them in unique_ptrs takes care of it.

FWIW this looks fine to me.

As I mentioned a few minutes ago, I have a bunch of -fsanitize=address
and -fsanitize=undefined patches.  I sent a few a little while ago, but
I have more here.  I thought I'd mention the state of this so we can
avoid duplicating work.

I have *not* looked at the AddressSanitizer leak reports at all, and I
wasn't planning to.

And, I don't yet have patches for any of the existing bugs:

https://sourceware.org/bugzilla/buglist.cgi?bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&list_id=43341&longdesc=AddressSanitizer&longdesc_type=allwordssubstr&product=gdb&query_format=advanced

Of these, only 22860 occurs during an ordinary "make check"; for that
one I have an idea of how to fix it, but I haven't tried it.

Tom

Re: [PP?] Re: [PATCH] Delete test target descriptions when exiting

[Simon Marchi]

On 2018-07-30 17:26, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:
> 
> Simon> Looking at the address sanitizer output, this was a quite low 
> hanging
> Simon> fruit.  We create target_desc objects for testing that we never 
> free.
> Simon> Saving them in unique_ptrs takes care of it.
> 
> FWIW this looks fine to me.

Thanks, I'll push it soon.

> As I mentioned a few minutes ago, I have a bunch of -fsanitize=address
> and -fsanitize=undefined patches.  I sent a few a little while ago, but
> I have more here.  I thought I'd mention the state of this so we can
> avoid duplicating work.
> 
> I have *not* looked at the AddressSanitizer leak reports at all, and I
> wasn't planning to.

Indeed, it's because you mentioned it that I took a look.  I'll try to 
integrate that in my "standard" build.

> And, I don't yet have patches for any of the existing bugs:
> 
> https://sourceware.org/bugzilla/buglist.cgi?bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&list_id=43341&longdesc=AddressSanitizer&longdesc_type=allwordssubstr&product=gdb&query_format=advanced
> 
> Of these, only 22860 occurs during an ordinary "make check"; for that
> one I have an idea of how to fix it, but I haven't tried it.

Understood.

Thanks,

Simon