From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vk1-xa2e.google.com (mail-vk1-xa2e.google.com [IPv6:2607:f8b0:4864:20::a2e]) by sourceware.org (Postfix) with ESMTPS id A4BAA393A424 for ; Wed, 12 Jan 2022 11:07:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A4BAA393A424 Received: by mail-vk1-xa2e.google.com with SMTP id w206so1379231vkd.10 for ; Wed, 12 Jan 2022 03:07:22 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=IFSKGfLHAkHzP6PPzBm6/OCzlu6j0e2t+Vrh/O87MtU=; b=3AgqvzYYkChca0VxLghc8NyxO2XhGZ2fQmxqGuHTazs/i19ulpGUIzBJqYkYviWhoK PjkS/Dr6iAVTbcd939x9r/fwRM8Ey+xJIFhIRwrPtAsFE78a+oOtwlK0733RBQUjhrVg Xb1YCG8dwfTdkvGgqb5eNOHcXsKKm1UV1rnyNhYI35o837Mr+L7/zcCycRKJ3egN+y+X MGPrekgWbosz8w68hWDvotWGDJqBE7nJxsxu6dIDdqxkP0y6DdtlKpq7fHudTNKv0p0W DmPeceeXSHE7i06fEBl7ODpi5UsMtVstvisbPayF+oPe4lGRlkU8pFLQJlrbVXaluDVN ckiw== X-Gm-Message-State: AOAM530bRHUelrqOgibKU8tIdzQz5TVBM+QpAVSiA5rplNbrmH4yk06A dxAVaBp1YXuXK+uO94NBN9uSgYWd7es+oSjY2fICtmoIS58MDQ== X-Google-Smtp-Source: ABdhPJx8ddvF3VpPEysccTVLN0j0WM0iv5gUTEG2ZAtQN0QWiufGOtpemBWYZPoXZcgpf3ZJvWQvf/i6hxJRK2A6h+Y= X-Received: by 2002:a1f:5702:: with SMTP id l2mr4039523vkb.33.1641985641665; Wed, 12 Jan 2022 03:07:21 -0800 (PST) MIME-Version: 1.0 References: <20220111161009.3008610-2-gprocida@google.com> <20220112104001.3169208-1-gprocida@google.com> In-Reply-To: <20220112104001.3169208-1-gprocida@google.com> From: Giuliano Procida Date: Wed, 12 Jan 2022 11:06:45 +0000 Message-ID: Subject: Re: [PATCH v2] abidw: add --ignore-enumerator-names flag To: libabigail@sourceware.org Cc: dodji@seketeli.org, kernel-team@android.com, maennich@google.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-27.3 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, 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.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 12 Jan 2022 11:07:24 -0000 This doesn't actually fix the problem - the environment default matters. I will follow up on Bug 28319. Giuliano. On Wed, 12 Jan 2022 at 10:40, Giuliano Procida wrote: > > Bug 28319 - abidw - regression in treatment of anonymous enums in structs > > This commit reinstates the abidw behaviour of comparing enumerator > names during duplicate enum type resolution which was lost by default > as of commit 1cfbff1b3037d1049bdff7e86de27c3a86af23b3. > > That commit addressed an issue with a certain library that contained > duplicate enum type definitions with variations in the enumerator > names but with the same enumerator values. > > Many projects will be able to avoid this issue by always compiling > from fresh and consistent sources. This commit avoids the risk of > conflating unrelated anonymous enum types. > > The previous behaviour can be requested with --ignore-enumerator-names > and this flag is now used for that specific library test case. > > Note that code that uses libabigail library facilities directly will > still continue to ignore enumerator names by default. This means that > all other tests are also unaffected by this code change. > > * doc/manuals/abidw.rst: Document --ignore-enumerator-names. > * include/abg-dwarf-reader.h (get_ignore_enumerator_names): > New function. > (set_ignore_enumerator_names): Likewise. > * src/abg-dwarf-reader.cc (read_context): Fix documentation > typos. Add ignore_enumerator_names_ member variable. > (read_context::initialize): Set ignore_enumerator_names_ to > false. > (read_context::ignore_enumerator_names): New getter and > setter methods. > (compare_before_canonicalisation): Set environment flag > use_enum_binary_only_equality according to the value of > ignore_enumerator_names_. > (get_ignore_enumerator_names): New function. > (set_ignore_enumerator_names): Likewise. > * src/abg-ir-priv.h (environment::priv::priv): Add comment to > default use_enum_binary_only_equality value. > * tests/test-alt-dwarf-file.cc: Add --ignore-enumerator-names > to rhbz1951526 test. > * tools/abidw.cc (options): Add ignore_enumerator_names_ > option, defaulted to false. > (parse_command_line): Parse --ignore-enumerator-names flag. > (load_corpus_and_write_abixml): Set ignore_enumerator_names > flag in DWARF reader context. > > Reviewed-by: Matthias Maennich > Signed-off-by: Giuliano Procida > --- > > This is really just for-the-record. This is the commit we are going to > land in AOSP. I've kept the impact on tests minimal to ease rebase, > revert etc. > > If you cannot find a better way forward on Bug 28319, then perhaps > take this commit, flip the default in src/abg-ir-priv.h and refresh > the few tests affected? > > That still leaves whoever cares about oddly-compiled libraries having > to pass a flag manually though. Could something be done with abidiff > suppressions instead? > > Regards, > Giuliano. > > doc/manuals/abidw.rst | 12 +++++++++ > include/abg-dwarf-reader.h | 7 ++++++ > src/abg-dwarf-reader.cc | 47 ++++++++++++++++++++++++++++++++++-- > src/abg-ir-priv.h | 2 +- > tests/test-alt-dwarf-file.cc | 2 +- > tools/abidw.cc | 7 ++++++ > 6 files changed, 73 insertions(+), 4 deletions(-) > > diff --git a/doc/manuals/abidw.rst b/doc/manuals/abidw.rst > index bdd6204d..53a0a18f 100644 > --- a/doc/manuals/abidw.rst > +++ b/doc/manuals/abidw.rst > @@ -182,6 +182,18 @@ Options > ELF binary. It thus considers functions and variables which are > defined and exported in the ELF sense. > > + * ``--ignore-enumerator-names`` > + > + During resolution of duplicate enum types, abidw can consider both > + enumerator names and values, or just enumerator values. > + > + The default is to consider enums, including anonymous enums, to be > + distinct if there any differences in their enumerators' names or > + values. > + > + This option causes abidw to only check for differences between > + sets of enumerator values. > + > * ``--check-alternate-debug-info`` <*elf-path*> > > If the debug info for the file *elf-path* contains a reference to > diff --git a/include/abg-dwarf-reader.h b/include/abg-dwarf-reader.h > index 4c2de779..1ecc7ac4 100644 > --- a/include/abg-dwarf-reader.h > +++ b/include/abg-dwarf-reader.h > @@ -142,6 +142,13 @@ void > set_drop_undefined_syms(read_context& ctxt, > bool f); > > +bool > +get_ignore_enumerator_names(read_context& ctxt); > + > +void > +set_ignore_enumerator_names(read_context& ctxt, > + bool f); > + > void > set_do_log(read_context& ctxt, bool f); > > diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc > index 3f716944..f34bc3ee 100644 > --- a/src/abg-dwarf-reader.cc > +++ b/src/abg-dwarf-reader.cc > @@ -1897,7 +1897,7 @@ struct dwarf_expr_eval_context > /// get some important data from it. > /// > /// When a new data member is added to this context, it must be > -/// initiliazed by the read_context::initiliaze() function. So please > +/// initialized by the read_context::initialize() function. So please > /// do not forget. > class read_context > { > @@ -2116,6 +2116,7 @@ public: > corpus::exported_decls_builder* exported_decls_builder_; > options_type options_; > bool drop_undefined_syms_; > + bool ignore_enumerator_names_; > read_context(); > > private: > @@ -2259,6 +2260,7 @@ public: > options_.load_in_linux_kernel_mode = linux_kernel_mode; > options_.load_all_types = load_all_types; > drop_undefined_syms_ = false; > + ignore_enumerator_names_ = false; > load_in_linux_kernel_mode(linux_kernel_mode); > } > > @@ -2346,6 +2348,23 @@ public: > drop_undefined_syms(bool f) > {drop_undefined_syms_ = f;} > > + /// Getter for the flag that tells if we should ignore enumerator > + /// names during duplicate enum type resolution. > + /// > + /// @return true iff we should ignore enumerator names during > + /// duplicate enum type resolution. > + bool > + ignore_enumerator_names() const > + {return ignore_enumerator_names_;} > + > + /// Setter for the flag that tells if we should ignore enumerator > + /// names during duplicate enum type resolution. > + /// > + /// @param f the new value of the flag. > + void > + ignore_enumerator_names(bool f) > + {ignore_enumerator_names_ = f;} > + > /// Getter of the suppression specifications to be used during > /// ELF/DWARF parsing. > /// > @@ -4151,7 +4170,7 @@ public: > bool s0 = e->decl_only_class_equals_definition(); > bool s1 = e->use_enum_binary_only_equality(); > e->decl_only_class_equals_definition(true); > - e->use_enum_binary_only_equality(true); > + e->use_enum_binary_only_equality(ignore_enumerator_names_); > bool equal = l == r; > e->decl_only_class_equals_definition(s0); > e->use_enum_binary_only_equality(s1); > @@ -6221,6 +6240,30 @@ void > set_drop_undefined_syms(read_context& ctxt, bool f) > {ctxt.drop_undefined_syms(f);} > > +/// Getter of the "ignore_enumerator_names" flag. > +/// > +/// This flag tells if we should ignore enumerator names during > +/// duplicate enum type resolution. > +/// > +/// @param ctx the read context to consider for this flag. > +/// > +/// @return the value of the flag. > +bool > +get_ignore_enumerator_names(read_context& ctxt) > +{return ctxt.ignore_enumerator_names();} > + > +/// Setter of the "ignore_enumerator_names" flag. > +/// > +/// This flag tells if we should ignore enumerator names during > +/// duplicate enum type resolution. > +/// > +/// @param ctxt the read context to consider for this flag. > +/// > +/// @param f the value of the flag. > +void > +set_ignore_enumerator_names(read_context& ctxt, bool f) > +{ctxt.ignore_enumerator_names(f);} > + > /// Setter of the "do_log" flag. > /// > /// This flag tells if we should emit verbose logs for various > diff --git a/src/abg-ir-priv.h b/src/abg-ir-priv.h > index a01a1b2c..79b5e2dd 100644 > --- a/src/abg-ir-priv.h > +++ b/src/abg-ir-priv.h > @@ -409,7 +409,7 @@ struct environment::priv > : canonicalization_is_done_(), > do_on_the_fly_canonicalization_(true), > decl_only_class_equals_definition_(false), > - use_enum_binary_only_equality_(true) > + use_enum_binary_only_equality_(true) // NOTE: != abidw default > #ifdef WITH_DEBUG_SELF_COMPARISON > , > self_comparison_debug_on_(false) > diff --git a/tests/test-alt-dwarf-file.cc b/tests/test-alt-dwarf-file.cc > index 315296ab..13ccb961 100644 > --- a/tests/test-alt-dwarf-file.cc > +++ b/tests/test-alt-dwarf-file.cc > @@ -56,7 +56,7 @@ InOutSpec in_out_specs[] = > { > "data/test-alt-dwarf-file/rhbz1951526/usr/bin/gimp-2.10", > "data/test-alt-dwarf-file/rhbz1951526/usr/lib/debug", > - "--abidiff", > + "--abidiff --ignore-enumerator-names", > "data/test-alt-dwarf-file/rhbz1951526/rhbz1951526-report-0.txt", > "output/test-alt-dwarf-file/rhbz1951526/rhbz1951526-report-0.txt" > }, > diff --git a/tools/abidw.cc b/tools/abidw.cc > index f7a8937d..ea2a4ad5 100644 > --- a/tools/abidw.cc > +++ b/tools/abidw.cc > @@ -95,6 +95,7 @@ struct options > bool default_sizes; > bool load_all_types; > bool linux_kernel_mode; > + bool ignore_enumerator_names; > bool corpus_group_for_linux; > bool show_stats; > bool noout; > @@ -132,6 +133,7 @@ struct options > default_sizes(true), > load_all_types(), > linux_kernel_mode(true), > + ignore_enumerator_names(false), > corpus_group_for_linux(false), > show_stats(), > noout(), > @@ -206,6 +208,8 @@ display_usage(const string& prog_name, ostream& out) > "vmlinux and its modules\n" > << " --vmlinux the path to the vmlinux binary to consider to emit " > "the ABI of the union of vmlinux and its modules\n" > + << " --ignore-enumerator-names only consider enumerator value sets when " > + "deciding whether identically-named or anonymous enums are the same\n" > << " --abidiff compare the loaded ABI against itself\n" > #ifdef WITH_DEBUG_SELF_COMPARISON > << " --debug-abidiff debug the process of comparing the loaded ABI against itself\n" > @@ -367,6 +371,8 @@ parse_command_line(int argc, char* argv[], options& opts) > opts.drop_undefined_syms = true; > else if (!strcmp(argv[i], "--no-linux-kernel-mode")) > opts.linux_kernel_mode = false; > + else if (!strcmp(argv[i], "--ignore-enumerator-names")) > + opts.ignore_enumerator_names = true; > else if (!strcmp(argv[i], "--abidiff")) > opts.abidiff = true; > #ifdef WITH_DEBUG_SELF_COMPARISON > @@ -562,6 +568,7 @@ load_corpus_and_write_abixml(char* argv[], > opts.linux_kernel_mode); > dwarf_reader::read_context& ctxt = *c; > set_drop_undefined_syms(ctxt, opts.drop_undefined_syms); > + set_ignore_enumerator_names(ctxt, opts.ignore_enumerator_names); > set_show_stats(ctxt, opts.show_stats); > set_suppressions(ctxt, opts); > abigail::dwarf_reader::set_do_log(ctxt, opts.do_log); > -- > 2.34.1.575.g55b058a8bb-goog >