From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-il1-x144.google.com (mail-il1-x144.google.com [IPv6:2607:f8b0:4864:20::144]) by sourceware.org (Postfix) with ESMTPS id 02C4F386F82D for ; Fri, 1 May 2020 14:23:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 02C4F386F82D Received: by mail-il1-x144.google.com with SMTP id m5so4556284ilj.10 for ; Fri, 01 May 2020 07:23:22 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=jmTOz8z4k+cEefrhr3Dgqj3ENqkAGAj4efyx7SltmV4=; b=UlevLlo6HALVzK+GpI6Q1v2X34AvZPST0manlr8wMeBwEjERCaY/itmzc8zIuXPRuv 64NVinJq6nY8S/KCJlQFfZ13OE9h737BIegHRJ4hbByWAnUr5cFecNY0Lh9MXtEpx/Xk THhqoDToWNzAbGJZnSEOPEItK6Q9Kzc5UF2NjoYz8cLAlwpG3rb077pu8CM6Q+lmg5dw LzemO0VtfguKUQvNhPjmVwg4CBuSpdasAo6CU89zpHP+d0US7qPx2uOszKYuvfGkPpdo fjf7+I+ClmZv3rcFwkaKLhAxHioLPJtMsoeCaBY7JG9HTYB6tU4wRwjCmcky8lfnTVC+ TeSg== X-Gm-Message-State: AGi0PuY5VigbmSP3tnWGEXnWwl2OgMABE/4eeHugI1DqNgP7etLoOzhJ 7LFP8jKVr84JTWAMI3RUXD2Qbm0k2lHBvb5lAUlqCQ== X-Google-Smtp-Source: APiQypItuigd7Ztia2TG+1zsQ2hHow3f0lLkftRLLdAnJSRKPTlXzzka7mavXUFbsc+x72bwHVaSPtNOs1/4K1XZrUQ= X-Received: by 2002:a92:c7a9:: with SMTP id f9mr3999583ilk.0.1588343002030; Fri, 01 May 2020 07:23:22 -0700 (PDT) MIME-Version: 1.0 References: <20200430215256.19135-1-maennich@google.com> In-Reply-To: <20200430215256.19135-1-maennich@google.com> From: Giuliano Procida Date: Fri, 1 May 2020 15:23:05 +0100 Message-ID: Subject: Re: [RESEND PATCH] reader context: do not reuse current corpus and corpus_group To: Matthias Maennich Cc: libabigail@sourceware.org, Dodji Seketeli , kernel-team@android.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-38.6 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libabigail mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 May 2020 14:23:25 -0000 Hi. Does this change have any impact on incomplete (forward-declared) type differences? Does it significantly impact performance on large inputs? Otherwise, this change makes sense and Reviewed-by: Giuliano Procida More generally, do you have a good understanding of what in the context is generic to the tool run and what is specific to a file being read and can they be separated cleanly? Giuliano. On Thu, 30 Apr 2020 at 22:53, Matthias Maennich wrote: > > libabigail's readers (abg-reader and abg-dwarf-reader) currently spare > some allocations by reusing the reader context's existing current corpus > and current corpus group. When building a corpus_group's vector of > corpora, reusing the shared_ptr referring to a corpus means we are > modifying the corpus data of a previously read corpus. As a user of the > read*corpus functions, that isn't entirely transparent and when storing > corpare like in the vector above, we might introduce subtle bugs. > > Fix this by explicitly creating new corpus / corpus group instances when > reading from elf/dwarf or xml. > > * src/abg-dwarf-reader.cc (read_debug_info_into_corpus): always > instantiate a new corpus instance. > * src/abg-reader.cc (read_corpus_from_input): Likewise. > (read_corpus_group_from_input): always instantiate a new corpus > group instance. > > Signed-off-by: Matthias Maennich > --- > src/abg-dwarf-reader.cc | 11 ++++----- > src/abg-reader.cc | 49 +++++++++++++++++------------------------ > 2 files changed, 24 insertions(+), 36 deletions(-) > > diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc > index 850281ad1ce5..d99d25bfb3b9 100644 > --- a/src/abg-dwarf-reader.cc > +++ b/src/abg-dwarf-reader.cc > @@ -16068,13 +16068,10 @@ 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()); > - } > + corpus_sptr corp(new corpus(ctxt.env(), ctxt.elf_path())); > + ctxt.current_corpus(corp); > + if (!ctxt.env()) > + ctxt.env(corp->get_environment()); > > // First set some mundane properties of the corpus gathered from > // ELF. > diff --git a/src/abg-reader.cc b/src/abg-reader.cc > index 255a200f2a25..0be45ec59a39 100644 > --- a/src/abg-reader.cc > +++ b/src/abg-reader.cc > @@ -25,6 +25,7 @@ > /// ABI Instrumentation file in libabigail native XML format. This > /// native XML format is named "abixml". > > +#include "abg-fwd.h" > #include "config.h" > #include > #include > @@ -1899,17 +1900,14 @@ 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); > - } > + corpus_sptr corp(new corpus(ctxt.get_environment(), "")); > + ctxt.set_corpus(corp); > > if (!ctxt.get_corpus_group()) > ctxt.clear_per_corpus_data(); > > - corpus& corp = *ctxt.get_corpus(); > - ctxt.set_exported_decls_builder(corp.get_exported_decls_builder().get()); > + ctxt.set_exported_decls_builder( > + corp->get_exported_decls_builder().get()); > > xml::xml_char_sptr path_str = XML_READER_GET_ATTRIBUTE(reader, "path"); > string path; > @@ -1917,13 +1915,13 @@ read_corpus_from_input(read_context& ctxt) > if (path_str) > { > path = reinterpret_cast(path_str.get()); > - corp.set_path(path); > + corp->set_path(path); > } > > xml::xml_char_sptr architecture_str = > XML_READER_GET_ATTRIBUTE(reader, "architecture"); > if (architecture_str) > - corp.set_architecture_name > + corp->set_architecture_name > (reinterpret_cast(architecture_str.get())); > > xml::xml_char_sptr soname_str = > @@ -1933,7 +1931,7 @@ read_corpus_from_input(read_context& ctxt) > if (soname_str) > { > soname = reinterpret_cast(soname_str.get()); > - corp.set_soname(soname); > + corp->set_soname(soname); > } > > // Apply suppression specifications here to honour: > @@ -1955,32 +1953,29 @@ read_corpus_from_input(read_context& ctxt) > } > else > { > - if (!ctxt.get_corpus()) > - { > - corpus_sptr c(new corpus(ctxt.get_environment(), "")); > - ctxt.set_corpus(c); > - } > + corpus_sptr corp(new corpus(ctxt.get_environment(), "")); > + ctxt.set_corpus(corp); > > if (!ctxt.get_corpus_group()) > ctxt.clear_per_corpus_data(); > > - corpus& corp = *ctxt.get_corpus(); > - ctxt.set_exported_decls_builder(corp.get_exported_decls_builder().get()); > + ctxt.set_exported_decls_builder( > + corp->get_exported_decls_builder().get()); > > xml::xml_char_sptr path_str = XML_NODE_GET_ATTRIBUTE(node, "path"); > if (path_str) > - corp.set_path(reinterpret_cast(path_str.get())); > + corp->set_path(reinterpret_cast(path_str.get())); > > xml::xml_char_sptr architecture_str = > XML_NODE_GET_ATTRIBUTE(node, "architecture"); > if (architecture_str) > - corp.set_architecture_name > + corp->set_architecture_name > (reinterpret_cast(architecture_str.get())); > > xml::xml_char_sptr soname_str = > XML_NODE_GET_ATTRIBUTE(node, "soname"); > if (soname_str) > - corp.set_soname(reinterpret_cast(soname_str.get())); > + corp->set_soname(reinterpret_cast(soname_str.get())); > } > > if (!node->children) > @@ -2093,14 +2088,10 @@ read_corpus_group_from_input(read_context& ctxt) > BAD_CAST("abi-corpus-group"))) > return nil; > > - if (!ctxt.get_corpus_group()) > - { > - corpus_group_sptr g(new corpus_group(ctxt.get_environment(), > - ctxt.get_path())); > - ctxt.set_corpus_group(g); > - } > + corpus_group_sptr group( > + new corpus_group(ctxt.get_environment(), ctxt.get_path())); > + ctxt.set_corpus_group(group); > > - corpus_group_sptr group = ctxt.get_corpus_group(); > xml::xml_char_sptr path_str = XML_READER_GET_ATTRIBUTE(reader, "path"); > if (path_str) > group->set_path(reinterpret_cast(path_str.get())); > @@ -2115,11 +2106,11 @@ read_corpus_group_from_input(read_context& ctxt) > > corpus_sptr corp; > while ((corp = read_corpus_from_input(ctxt))) > - ctxt.get_corpus_group()->add_corpus(corp); > + group->add_corpus(corp); > > xmlTextReaderNext(reader.get()); > > - return ctxt.get_corpus_group(); > + return group; > } > > /// De-serialize an ABI corpus group from an input XML document which > -- > 2.26.2.526.g744177e7f7-goog >