public inbox for libabigail@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/3] always create fresh corpus objects
@ 2021-04-27 11:28 Giuliano Procida
  2021-04-27 11:28 ` [PATCH 1/3] abg-dwarf-reader: create new corpus unconditionally Giuliano Procida
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Giuliano Procida @ 2021-04-27 11:28 UTC (permalink / raw)
  To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich

This is a small series of changes that adjust how corpus objects are
created when reading into a corpus group.

Overall the changes eliminate some conditional logic and make abidiff
slightly faster when processing ABIs containing multiple corpora.

Giuliano Procida (3):
  abg-dwarf-reader: create new corpus unconditionally
  abg-reader: ensure corpus always has a symtab reader
  abg-reader: create a fresh corpus object per corpus

 src/abg-dwarf-reader.cc |  9 +-----
 src/abg-reader.cc       | 71 +++++++++++------------------------------
 2 files changed, 20 insertions(+), 60 deletions(-)

-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] abg-dwarf-reader: create new corpus unconditionally
  2021-04-27 11:28 [PATCH 0/3] always create fresh corpus objects Giuliano Procida
@ 2021-04-27 11:28 ` Giuliano Procida
  2021-04-27 11:28 ` [PATCH 2/3] abg-reader: ensure corpus always has a symtab reader Giuliano Procida
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Giuliano Procida @ 2021-04-27 11:28 UTC (permalink / raw)
  To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich

The DWARF reader appears to create a new corpus object only if one is
not already present. However, the only case where there can be
multiple corpora is when build_corpus_group_from_kernel_dist_under is
called and this function clears down the reader context, including the
current corpus, between reading ELF objects.

So it's clearer to just create a fresh corpus object unconditionally
in the DWARF reader.

	* src/abg-dwarf-reader.cc (read_debug_info_into_corpus):
	Create new corpus object unconditionally.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-dwarf-reader.cc | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
index dd9d689c..28d1c73a 100644
--- a/src/abg-dwarf-reader.cc
+++ b/src/abg-dwarf-reader.cc
@@ -14233,14 +14233,7 @@ static corpus_sptr
 read_debug_info_into_corpus(read_context& ctxt)
 {
   ctxt.clear_per_corpus_data();
-
-  if (!ctxt.current_corpus())
-    {
-      corpus_sptr corp (new corpus(ctxt.env(), ctxt.elf_path()));
-      ctxt.current_corpus(corp);
-      if (!ctxt.env())
-	ctxt.env(corp->get_environment());
-    }
+  ctxt.current_corpus(std::make_shared<corpus>(ctxt.env(), ctxt.elf_path()));
 
   // First set some mundane properties of the corpus gathered from
   // ELF.
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/3] abg-reader: ensure corpus always has a symtab reader
  2021-04-27 11:28 [PATCH 0/3] always create fresh corpus objects Giuliano Procida
  2021-04-27 11:28 ` [PATCH 1/3] abg-dwarf-reader: create new corpus unconditionally Giuliano Procida
@ 2021-04-27 11:28 ` Giuliano Procida
  2021-04-27 11:28 ` [PATCH 3/3] abg-reader: create a fresh corpus object per corpus Giuliano Procida
  2021-05-27  8:53 ` [PATCH v2 0/3] always create fresh corpus objects Giuliano Procida
  3 siblings, 0 replies; 13+ messages in thread
From: Giuliano Procida @ 2021-04-27 11:28 UTC (permalink / raw)
  To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich

In the presence of an empty abi-corpus element and with the following
change to always allocate a fresh corpus object, such objects can
sometimes be left without a symtab reader, instead of inheritng one
from the previous corpus.

The reader is called to obtain sorted lists of symbols during ABI
comparisons. The simplest way to avoid a crash is to maintain the
invariant that a reader object is always present.

With this change, if there is bad XML preventing symbols from being
read, no error is raised as before, but the logic has been tweaked so
that abi-instr parsing will nevertheless be attempted.

	* src/abg-reader.cc (read_symbol_db_from_input): Fix
	documentation for this function. Allow "successful parsing" to
	include the case where no symbols were present in the input.
	(read_corpus_from_input): Unconditionally set a symtab reader
	on the corpus object. Unconditionally parse the abi-instr of a
	corpus.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-reader.cc | 59 ++++++++++++++---------------------------------
 1 file changed, 17 insertions(+), 42 deletions(-)

diff --git a/src/abg-reader.cc b/src/abg-reader.cc
index 17b75914..313af4c9 100644
--- a/src/abg-reader.cc
+++ b/src/abg-reader.cc
@@ -1497,8 +1497,8 @@ read_translation_unit_from_input(read_context&	ctxt)
   return tu;
 }
 
-/// Parse the input XML document containing a function symbols
-/// or a variable symbol database.
+/// Parse the input XML document that may contain function symbol and
+/// variable symbol databases.
 ///
 /// A function symbols database is an XML element named
 /// "elf-function-symbols" and a variable symbols database is an XML
@@ -1507,12 +1507,11 @@ read_translation_unit_from_input(read_context&	ctxt)
 ///
 /// @param ctxt the read_context to use for the parsing.
 ///
-/// @param function_symbols is true if this function should look for a
-/// function symbols database, false if it should look for a variable
-/// symbols database.
+/// @param fn_symdb any resulting function symbol database object, if
+/// elf-function-symbols was present.
 ///
-/// @param symdb the resulting symbol database object.  This is set
-/// iff the function return true.
+/// @param var_symdb any resulting variable symbol database object, if
+/// elf-variable-symbols was present.
 ///
 /// @return true upon successful parsing, false otherwise.
 static bool
@@ -1524,8 +1523,6 @@ read_symbol_db_from_input(read_context&		 ctxt,
   if (!reader)
     return false;
 
-  bool found = false;
-
   if (!ctxt.get_corpus_node())
     for (;;)
       {
@@ -1552,17 +1549,9 @@ read_symbol_db_from_input(read_context&		 ctxt,
 	  return false;
 
 	if (has_fn_syms)
-	  {
-	    fn_symdb = build_elf_symbol_db(ctxt, node, true);
-	    if (fn_symdb)
-	      found = true;
-	  }
+	  fn_symdb = build_elf_symbol_db(ctxt, node, true);
 	else if (has_var_syms)
-	  {
-	    var_symdb = build_elf_symbol_db(ctxt, node, false);
-	    if (var_symdb)
-	      found = true;
-	  }
+	  var_symdb = build_elf_symbol_db(ctxt, node, false);
 
 	xmlTextReaderNext(reader.get());
       }
@@ -1579,22 +1568,16 @@ read_symbol_db_from_input(read_context&		 ctxt,
 	  else
 	    break;
 	  if (has_fn_syms)
-	    {
-	      fn_symdb = build_elf_symbol_db(ctxt, n, true);
-	      found = true;
-	    }
+	    fn_symdb = build_elf_symbol_db(ctxt, n, true);
 	  else if (has_var_syms)
-	    {
-	      var_symdb = build_elf_symbol_db(ctxt, n, false);
-	      found = true;
-	    }
+	    var_symdb = build_elf_symbol_db(ctxt, n, false);
 	  else
 	    break;
 	}
       ctxt.set_corpus_node(n);
     }
 
-  return found;
+  return true;
 }
 
 /// From an "elf-needed" XML_ELEMENT node, build a vector of strings
@@ -1894,24 +1877,16 @@ read_corpus_from_input(read_context& ctxt)
   string_elf_symbols_map_sptr fn_sym_db, var_sym_db;
 
   // Read the symbol databases.
-  bool is_ok = read_symbol_db_from_input(ctxt, fn_sym_db, var_sym_db);
-  if (is_ok)
-    {
-      // Note that it's possible that both fn_sym_db and var_sym_db
-      // are nil, due to potential suppression specifications.  That's
-      // fine.
-      corp.set_symtab(symtab_reader::symtab::load(fn_sym_db, var_sym_db));
-    }
+  read_symbol_db_from_input(ctxt, fn_sym_db, var_sym_db);
+  // Note that it's possible that both fn_sym_db and var_sym_db are nil,
+  // due to potential suppression specifications.  That's fine.
+  corp.set_symtab(symtab_reader::symtab::load(fn_sym_db, var_sym_db));
 
   ctxt.get_environment()->canonicalization_is_done(false);
 
   // Read the translation units.
-  do
-    {
-      translation_unit_sptr tu = read_translation_unit_from_input(ctxt);
-      is_ok = bool(tu);
-    }
-  while (is_ok);
+  while (read_translation_unit_from_input(ctxt))
+    ;
 
   if (ctxt.tracking_non_reachable_types())
     {
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/3] abg-reader: create a fresh corpus object per corpus
  2021-04-27 11:28 [PATCH 0/3] always create fresh corpus objects Giuliano Procida
  2021-04-27 11:28 ` [PATCH 1/3] abg-dwarf-reader: create new corpus unconditionally Giuliano Procida
  2021-04-27 11:28 ` [PATCH 2/3] abg-reader: ensure corpus always has a symtab reader Giuliano Procida
@ 2021-04-27 11:28 ` Giuliano Procida
  2021-06-10 16:54   ` Dodji Seketeli
  2021-05-27  8:53 ` [PATCH v2 0/3] always create fresh corpus objects Giuliano Procida
  3 siblings, 1 reply; 13+ messages in thread
From: Giuliano Procida @ 2021-04-27 11:28 UTC (permalink / raw)
  To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich

Currently the XML reader reuses the same corpus object for all
corpora in a corpus group. This has an unwanted side-effect: any
abi-instr with the same path in different corpora will collide and
parts of the ABI will be lost.

Creating a new corpus object for every abi-corpus element seems like
the right thing to do. Testing with large ABIs containing many corpora
also shows a modest (~10%) abidiff speed improvement.

	* src/abg-reader.cc (read_corpus_from_input): Always create a
	fresh corpus object for each abi-corpus XML element.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-reader.cc | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/src/abg-reader.cc b/src/abg-reader.cc
index 313af4c9..e32c9bf3 100644
--- a/src/abg-reader.cc
+++ b/src/abg-reader.cc
@@ -1776,11 +1776,7 @@ read_corpus_from_input(read_context& ctxt)
 				       BAD_CAST("abi-corpus")))
 	return nil;
 
-      if (!ctxt.get_corpus())
-	{
-	  corpus_sptr c(new corpus(ctxt.get_environment(), ""));
-	  ctxt.set_corpus(c);
-	}
+      ctxt.set_corpus(std::make_shared<corpus>(ctxt.get_environment(), ""));
 
       if (!ctxt.get_corpus_group())
 	ctxt.clear_per_corpus_data();
@@ -1834,11 +1830,7 @@ read_corpus_from_input(read_context& ctxt)
     }
   else
     {
-      if (!ctxt.get_corpus())
-	{
-	  corpus_sptr c(new corpus(ctxt.get_environment(), ""));
-	  ctxt.set_corpus(c);
-	}
+      ctxt.set_corpus(std::make_shared<corpus>(ctxt.get_environment(), ""));
 
       if (!ctxt.get_corpus_group())
 	ctxt.clear_per_corpus_data();
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 0/3] always create fresh corpus objects
  2021-04-27 11:28 [PATCH 0/3] always create fresh corpus objects Giuliano Procida
                   ` (2 preceding siblings ...)
  2021-04-27 11:28 ` [PATCH 3/3] abg-reader: create a fresh corpus object per corpus Giuliano Procida
@ 2021-05-27  8:53 ` Giuliano Procida
  2021-05-27  8:53   ` [PATCH v2 1/3] abg-dwarf-reader: create new corpus unconditionally Giuliano Procida
                     ` (2 more replies)
  3 siblings, 3 replies; 13+ messages in thread
From: Giuliano Procida @ 2021-05-27  8:53 UTC (permalink / raw)
  To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich

This is a refresh following rebase on top of current master. In
particular, the recent debug self-comparison change resulted in
some conflicts needing manual resolution.

This is a small series of changes that adjust how corpus objects are
created when reading into a corpus group.

Overall the changes eliminate some conditional logic and make abidiff
slightly faster when processing ABIs containing multiple corpora,
i.e., Linux kernels and modules.

Giuliano Procida (3):
  abg-dwarf-reader: create new corpus unconditionally
  abg-reader: ensure corpus always has a symtab reader
  abg-reader: create a fresh corpus object per corpus

 src/abg-dwarf-reader.cc |  9 +----
 src/abg-reader.cc       | 83 +++++++++++++----------------------------
 2 files changed, 26 insertions(+), 66 deletions(-)

-- 
2.31.1.818.g46aad6cb9e-goog


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 1/3] abg-dwarf-reader: create new corpus unconditionally
  2021-05-27  8:53 ` [PATCH v2 0/3] always create fresh corpus objects Giuliano Procida
@ 2021-05-27  8:53   ` Giuliano Procida
  2021-06-01  7:38     ` Giuliano Procida
                       ` (2 more replies)
  2021-05-27  8:53   ` [PATCH v2 2/3] abg-reader: ensure corpus always has a symtab reader Giuliano Procida
  2021-05-27  8:53   ` [PATCH v2 3/3] abg-reader: create a fresh corpus object per corpus Giuliano Procida
  2 siblings, 3 replies; 13+ messages in thread
From: Giuliano Procida @ 2021-05-27  8:53 UTC (permalink / raw)
  To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich

The DWARF reader appears to create a new corpus object only if one is
not already present. However, the only case where there can be
multiple corpora is when build_corpus_group_from_kernel_dist_under is
called and this function clears down the reader context, including the
current corpus, between reading ELF objects.

So it's clearer to just create a fresh corpus object unconditionally
in the DWARF reader.

	* src/abg-dwarf-reader.cc (read_debug_info_into_corpus):
	Create new corpus object unconditionally.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-dwarf-reader.cc | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
index a06ca88f..135d33c3 100644
--- a/src/abg-dwarf-reader.cc
+++ b/src/abg-dwarf-reader.cc
@@ -14243,14 +14243,7 @@ static corpus_sptr
 read_debug_info_into_corpus(read_context& ctxt)
 {
   ctxt.clear_per_corpus_data();
-
-  if (!ctxt.current_corpus())
-    {
-      corpus_sptr corp (new corpus(ctxt.env(), ctxt.elf_path()));
-      ctxt.current_corpus(corp);
-      if (!ctxt.env())
-	ctxt.env(corp->get_environment());
-    }
+  ctxt.current_corpus(std::make_shared<corpus>(ctxt.env(), ctxt.elf_path()));
 
   // First set some mundane properties of the corpus gathered from
   // ELF.
-- 
2.31.1.818.g46aad6cb9e-goog


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 2/3] abg-reader: ensure corpus always has a symtab reader
  2021-05-27  8:53 ` [PATCH v2 0/3] always create fresh corpus objects Giuliano Procida
  2021-05-27  8:53   ` [PATCH v2 1/3] abg-dwarf-reader: create new corpus unconditionally Giuliano Procida
@ 2021-05-27  8:53   ` Giuliano Procida
  2021-06-10 16:53     ` Dodji Seketeli
  2021-05-27  8:53   ` [PATCH v2 3/3] abg-reader: create a fresh corpus object per corpus Giuliano Procida
  2 siblings, 1 reply; 13+ messages in thread
From: Giuliano Procida @ 2021-05-27  8:53 UTC (permalink / raw)
  To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich

In the presence of an empty abi-corpus element and with the following
change to always allocate a fresh corpus object, such objects can
sometimes be left without a symtab reader, instead of inheritng one
from the previous corpus.

The reader is called to obtain sorted lists of symbols during ABI
comparisons. The simplest way to avoid a crash is to maintain the
invariant that a reader object is always present.

With this change, if there is bad XML preventing symbols from being
read, no error is raised as before, but the logic has been tweaked so
that abi-instr parsing will nevertheless be attempted.

	* src/abg-reader.cc (read_symbol_db_from_input): Fix
	documentation for this function. Allow "successful parsing" to
	include the case where no symbols were present in the input.
	(read_corpus_from_input): Unconditionally set a symtab reader
	on the corpus object. Unconditionally parse the abi-instr of a
	corpus.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-reader.cc | 59 ++++++++++++++---------------------------------
 1 file changed, 17 insertions(+), 42 deletions(-)

diff --git a/src/abg-reader.cc b/src/abg-reader.cc
index 0449718d..9bc43bbf 100644
--- a/src/abg-reader.cc
+++ b/src/abg-reader.cc
@@ -1608,8 +1608,8 @@ read_translation_unit_from_input(read_context&	ctxt)
   return tu;
 }
 
-/// Parse the input XML document containing a function symbols
-/// or a variable symbol database.
+/// Parse the input XML document that may contain function symbol and
+/// variable symbol databases.
 ///
 /// A function symbols database is an XML element named
 /// "elf-function-symbols" and a variable symbols database is an XML
@@ -1618,12 +1618,11 @@ read_translation_unit_from_input(read_context&	ctxt)
 ///
 /// @param ctxt the read_context to use for the parsing.
 ///
-/// @param function_symbols is true if this function should look for a
-/// function symbols database, false if it should look for a variable
-/// symbols database.
+/// @param fn_symdb any resulting function symbol database object, if
+/// elf-function-symbols was present.
 ///
-/// @param symdb the resulting symbol database object.  This is set
-/// iff the function return true.
+/// @param var_symdb any resulting variable symbol database object, if
+/// elf-variable-symbols was present.
 ///
 /// @return true upon successful parsing, false otherwise.
 static bool
@@ -1635,8 +1634,6 @@ read_symbol_db_from_input(read_context&		 ctxt,
   if (!reader)
     return false;
 
-  bool found = false;
-
   if (!ctxt.get_corpus_node())
     for (;;)
       {
@@ -1663,17 +1660,9 @@ read_symbol_db_from_input(read_context&		 ctxt,
 	  return false;
 
 	if (has_fn_syms)
-	  {
-	    fn_symdb = build_elf_symbol_db(ctxt, node, true);
-	    if (fn_symdb)
-	      found = true;
-	  }
+	  fn_symdb = build_elf_symbol_db(ctxt, node, true);
 	else if (has_var_syms)
-	  {
-	    var_symdb = build_elf_symbol_db(ctxt, node, false);
-	    if (var_symdb)
-	      found = true;
-	  }
+	  var_symdb = build_elf_symbol_db(ctxt, node, false);
 
 	xmlTextReaderNext(reader.get());
       }
@@ -1690,22 +1679,16 @@ read_symbol_db_from_input(read_context&		 ctxt,
 	  else
 	    break;
 	  if (has_fn_syms)
-	    {
-	      fn_symdb = build_elf_symbol_db(ctxt, n, true);
-	      found = true;
-	    }
+	    fn_symdb = build_elf_symbol_db(ctxt, n, true);
 	  else if (has_var_syms)
-	    {
-	      var_symdb = build_elf_symbol_db(ctxt, n, false);
-	      found = true;
-	    }
+	    var_symdb = build_elf_symbol_db(ctxt, n, false);
 	  else
 	    break;
 	}
       ctxt.set_corpus_node(n);
     }
 
-  return found;
+  return true;
 }
 
 /// From an "elf-needed" XML_ELEMENT node, build a vector of strings
@@ -2017,24 +2000,16 @@ read_corpus_from_input(read_context& ctxt)
   string_elf_symbols_map_sptr fn_sym_db, var_sym_db;
 
   // Read the symbol databases.
-  bool is_ok = read_symbol_db_from_input(ctxt, fn_sym_db, var_sym_db);
-  if (is_ok)
-    {
-      // Note that it's possible that both fn_sym_db and var_sym_db
-      // are nil, due to potential suppression specifications.  That's
-      // fine.
-      corp.set_symtab(symtab_reader::symtab::load(fn_sym_db, var_sym_db));
-    }
+  read_symbol_db_from_input(ctxt, fn_sym_db, var_sym_db);
+  // Note that it's possible that both fn_sym_db and var_sym_db are nil,
+  // due to potential suppression specifications.  That's fine.
+  corp.set_symtab(symtab_reader::symtab::load(fn_sym_db, var_sym_db));
 
   ctxt.get_environment()->canonicalization_is_done(false);
 
   // Read the translation units.
-  do
-    {
-      translation_unit_sptr tu = read_translation_unit_from_input(ctxt);
-      is_ok = bool(tu);
-    }
-  while (is_ok);
+  while (read_translation_unit_from_input(ctxt))
+    ;
 
   if (ctxt.tracking_non_reachable_types())
     {
-- 
2.31.1.818.g46aad6cb9e-goog


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 3/3] abg-reader: create a fresh corpus object per corpus
  2021-05-27  8:53 ` [PATCH v2 0/3] always create fresh corpus objects Giuliano Procida
  2021-05-27  8:53   ` [PATCH v2 1/3] abg-dwarf-reader: create new corpus unconditionally Giuliano Procida
  2021-05-27  8:53   ` [PATCH v2 2/3] abg-reader: ensure corpus always has a symtab reader Giuliano Procida
@ 2021-05-27  8:53   ` Giuliano Procida
  2 siblings, 0 replies; 13+ messages in thread
From: Giuliano Procida @ 2021-05-27  8:53 UTC (permalink / raw)
  To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich

Currently the XML reader reuses the same corpus object for all
corpora in a corpus group. This has an unwanted side-effect: any
abi-instr with the same path in different corpora will collide and
parts of the ABI will be lost.

Creating a new corpus object for every abi-corpus element seems like
the right thing to do. Testing with large ABIs containing many corpora
also shows a modest (~10%) abidiff speed improvement.

	* src/abg-reader.cc (read_corpus_from_input): Always create a
	fresh corpus object for each abi-corpus XML element.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-reader.cc | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/src/abg-reader.cc b/src/abg-reader.cc
index 9bc43bbf..febd6ca4 100644
--- a/src/abg-reader.cc
+++ b/src/abg-reader.cc
@@ -1889,16 +1889,12 @@ read_corpus_from_input(read_context& ctxt)
 				       BAD_CAST("abi-corpus")))
 	return nil;
 
-      if (!ctxt.get_corpus())
-	{
-	  corpus_sptr c(new corpus(ctxt.get_environment(), ""));
-	  ctxt.set_corpus(c);
+      ctxt.set_corpus(std::make_shared<corpus>(ctxt.get_environment(), ""));
 #ifdef WITH_DEBUG_SELF_COMPARISON
-	  if (ctxt.get_environment()->self_comparison_debug_is_on())
-	    ctxt.get_environment()->
-	      set_self_comparison_debug_input(ctxt.get_corpus());
+      if (ctxt.get_environment()->self_comparison_debug_is_on())
+	ctxt.get_environment()->
+	  set_self_comparison_debug_input(ctxt.get_corpus());
 #endif
-	}
 
       if (!ctxt.get_corpus_group())
 	ctxt.clear_per_corpus_data();
@@ -1952,16 +1948,12 @@ read_corpus_from_input(read_context& ctxt)
     }
   else
     {
-      if (!ctxt.get_corpus())
-	{
-	  corpus_sptr c(new corpus(ctxt.get_environment(), ""));
-	  ctxt.set_corpus(c);
+      ctxt.set_corpus(std::make_shared<corpus>(ctxt.get_environment(), ""));
 #ifdef WITH_DEBUG_SELF_COMPARISON
-	  if (ctxt.get_environment()->self_comparison_debug_is_on())
-	    ctxt.get_environment()->
-	      set_self_comparison_debug_input(ctxt.get_corpus());
+      if (ctxt.get_environment()->self_comparison_debug_is_on())
+	ctxt.get_environment()->
+	  set_self_comparison_debug_input(ctxt.get_corpus());
 #endif
-	}
 
       if (!ctxt.get_corpus_group())
 	ctxt.clear_per_corpus_data();
-- 
2.31.1.818.g46aad6cb9e-goog


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/3] abg-dwarf-reader: create new corpus unconditionally
  2021-05-27  8:53   ` [PATCH v2 1/3] abg-dwarf-reader: create new corpus unconditionally Giuliano Procida
@ 2021-06-01  7:38     ` Giuliano Procida
  2021-06-10 15:58     ` [PATCH v2 1/3, applied] " Dodji Seketeli
  2021-06-10 16:52     ` [PATCH v2 1/3] " Dodji Seketeli
  2 siblings, 0 replies; 13+ messages in thread
From: Giuliano Procida @ 2021-06-01  7:38 UTC (permalink / raw)
  To: Giuliano Procida via Libabigail
  Cc: Dodji Seketeli, kernel-team, Matthias Männich

I forgot to add: this addresses bug 27769.

On Thu, 27 May 2021, 09:53 Giuliano Procida, <gprocida@google.com> wrote:

> The DWARF reader appears to create a new corpus object only if one is
> not already present. However, the only case where there can be
> multiple corpora is when build_corpus_group_from_kernel_dist_under is
> called and this function clears down the reader context, including the
> current corpus, between reading ELF objects.
>
> So it's clearer to just create a fresh corpus object unconditionally
> in the DWARF reader.
>
>         * src/abg-dwarf-reader.cc (read_debug_info_into_corpus):
>         Create new corpus object unconditionally.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>
> ---
>  src/abg-dwarf-reader.cc | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
> index a06ca88f..135d33c3 100644
> --- a/src/abg-dwarf-reader.cc
> +++ b/src/abg-dwarf-reader.cc
> @@ -14243,14 +14243,7 @@ static corpus_sptr
>  read_debug_info_into_corpus(read_context& ctxt)
>  {
>    ctxt.clear_per_corpus_data();
> -
> -  if (!ctxt.current_corpus())
> -    {
> -      corpus_sptr corp (new corpus(ctxt.env(), ctxt.elf_path()));
> -      ctxt.current_corpus(corp);
> -      if (!ctxt.env())
> -       ctxt.env(corp->get_environment());
> -    }
> +  ctxt.current_corpus(std::make_shared<corpus>(ctxt.env(),
> ctxt.elf_path()));
>
>    // First set some mundane properties of the corpus gathered from
>    // ELF.
> --
> 2.31.1.818.g46aad6cb9e-goog
>
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/3, applied] abg-dwarf-reader: create new corpus unconditionally
  2021-05-27  8:53   ` [PATCH v2 1/3] abg-dwarf-reader: create new corpus unconditionally Giuliano Procida
  2021-06-01  7:38     ` Giuliano Procida
@ 2021-06-10 15:58     ` Dodji Seketeli
  2021-06-10 16:52     ` [PATCH v2 1/3] " Dodji Seketeli
  2 siblings, 0 replies; 13+ messages in thread
From: Dodji Seketeli @ 2021-06-10 15:58 UTC (permalink / raw)
  To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich

Giuliano Procida <gprocida@google.com> a écrit:

> The DWARF reader appears to create a new corpus object only if one is
> not already present. However, the only case where there can be
> multiple corpora is when build_corpus_group_from_kernel_dist_under is
> called and this function clears down the reader context, including the
> current corpus, between reading ELF objects.
>
> So it's clearer to just create a fresh corpus object unconditionally
> in the DWARF reader.
>
> 	* src/abg-dwarf-reader.cc (read_debug_info_into_corpus):
> 	Create new corpus object unconditionally.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>

Applied to master, thanks!

[...]

Cheers,

-- 
		Dodji

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/3] abg-dwarf-reader: create new corpus unconditionally
  2021-05-27  8:53   ` [PATCH v2 1/3] abg-dwarf-reader: create new corpus unconditionally Giuliano Procida
  2021-06-01  7:38     ` Giuliano Procida
  2021-06-10 15:58     ` [PATCH v2 1/3, applied] " Dodji Seketeli
@ 2021-06-10 16:52     ` Dodji Seketeli
  2 siblings, 0 replies; 13+ messages in thread
From: Dodji Seketeli @ 2021-06-10 16:52 UTC (permalink / raw)
  To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich

Giuliano Procida <gprocida@google.com> a écrit:

> The DWARF reader appears to create a new corpus object only if one is
> not already present. However, the only case where there can be
> multiple corpora is when build_corpus_group_from_kernel_dist_under is
> called and this function clears down the reader context, including the
> current corpus, between reading ELF objects.
>
> So it's clearer to just create a fresh corpus object unconditionally
> in the DWARF reader.
>
> 	* src/abg-dwarf-reader.cc (read_debug_info_into_corpus):
> 	Create new corpus object unconditionally.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>

Applied to master.

Thanks!

[...]

Cheers,

-- 
		Dodji

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/3] abg-reader: ensure corpus always has a symtab reader
  2021-05-27  8:53   ` [PATCH v2 2/3] abg-reader: ensure corpus always has a symtab reader Giuliano Procida
@ 2021-06-10 16:53     ` Dodji Seketeli
  0 siblings, 0 replies; 13+ messages in thread
From: Dodji Seketeli @ 2021-06-10 16:53 UTC (permalink / raw)
  To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich

Giuliano Procida <gprocida@google.com> a écrit:

> In the presence of an empty abi-corpus element and with the following
> change to always allocate a fresh corpus object, such objects can
> sometimes be left without a symtab reader, instead of inheritng one
> from the previous corpus.
>
> The reader is called to obtain sorted lists of symbols during ABI
> comparisons. The simplest way to avoid a crash is to maintain the
> invariant that a reader object is always present.
>
> With this change, if there is bad XML preventing symbols from being
> read, no error is raised as before, but the logic has been tweaked so
> that abi-instr parsing will nevertheless be attempted.
>
> 	* src/abg-reader.cc (read_symbol_db_from_input): Fix
> 	documentation for this function. Allow "successful parsing" to
> 	include the case where no symbols were present in the input.
> 	(read_corpus_from_input): Unconditionally set a symtab reader
> 	on the corpus object. Unconditionally parse the abi-instr of a
> 	corpus.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>

Applied to master, thanks!

[...]

Cheers,

-- 
		Dodji

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] abg-reader: create a fresh corpus object per corpus
  2021-04-27 11:28 ` [PATCH 3/3] abg-reader: create a fresh corpus object per corpus Giuliano Procida
@ 2021-06-10 16:54   ` Dodji Seketeli
  0 siblings, 0 replies; 13+ messages in thread
From: Dodji Seketeli @ 2021-06-10 16:54 UTC (permalink / raw)
  To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich

Giuliano Procida <gprocida@google.com> a écrit:

> Currently the XML reader reuses the same corpus object for all
> corpora in a corpus group. This has an unwanted side-effect: any
> abi-instr with the same path in different corpora will collide and
> parts of the ABI will be lost.
>
> Creating a new corpus object for every abi-corpus element seems like
> the right thing to do. Testing with large ABIs containing many corpora
> also shows a modest (~10%) abidiff speed improvement.
>
> 	* src/abg-reader.cc (read_corpus_from_input): Always create a
> 	fresh corpus object for each abi-corpus XML element.
>
> Signed-off-by: Giuliano Procida <gprocida@google.com>

Applied to master, thanks!

[...]

Cheers,

-- 
		Dodji

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-06-10 16:54 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-27 11:28 [PATCH 0/3] always create fresh corpus objects Giuliano Procida
2021-04-27 11:28 ` [PATCH 1/3] abg-dwarf-reader: create new corpus unconditionally Giuliano Procida
2021-04-27 11:28 ` [PATCH 2/3] abg-reader: ensure corpus always has a symtab reader Giuliano Procida
2021-04-27 11:28 ` [PATCH 3/3] abg-reader: create a fresh corpus object per corpus Giuliano Procida
2021-06-10 16:54   ` Dodji Seketeli
2021-05-27  8:53 ` [PATCH v2 0/3] always create fresh corpus objects Giuliano Procida
2021-05-27  8:53   ` [PATCH v2 1/3] abg-dwarf-reader: create new corpus unconditionally Giuliano Procida
2021-06-01  7:38     ` Giuliano Procida
2021-06-10 15:58     ` [PATCH v2 1/3, applied] " Dodji Seketeli
2021-06-10 16:52     ` [PATCH v2 1/3] " Dodji Seketeli
2021-05-27  8:53   ` [PATCH v2 2/3] abg-reader: ensure corpus always has a symtab reader Giuliano Procida
2021-06-10 16:53     ` Dodji Seketeli
2021-05-27  8:53   ` [PATCH v2 3/3] abg-reader: create a fresh corpus object per corpus Giuliano Procida

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).