* [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
* 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
* [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
* 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
* [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
* 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
* [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
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).