Re: [PATCH v2] abidw: add --ignore-enumerator-names flag

[Giuliano Procida <gprocida@google.com>]

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
>