[PATCH v2] abidw: add --ignore-enumerator-names flag
Giuliano Procida
gprocida@google.com
Wed Jan 12 11:06:45 GMT 2022
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 <gprocida@google.com> 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 <maennich@google.com>
> Signed-off-by: Giuliano Procida <gprocida@google.com>
> ---
>
> 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 <path> 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
>
More information about the Libabigail
mailing list