Re: [PATCH] [gdb/build] Fix Wmaybe-uninitialized false positive for ui_out_emit_type

[Andrew Burgess <aburgess@redhat.com>]

Tom de Vries <tdevries@suse.de> writes:

> With an aarch64-linux -O2 build with gcc 16.2.1, I run into:
> ...
> In file included from gdb/exceptions.h:23,
>                  from gdb/symtab.c:21:
> In destructor ‘ui_out_emit_type<Type>::~ui_out_emit_type() [with ui_out_type Type = ui_out_type_tuple]’,
>     inlined from ‘constexpr void std::_Optional_payload_base<_Tp>::_M_destroy() [with _Tp = ui_out_emit_type<ui_out_type_tuple>]’ at /usr/include/c++/16/optional:325:35,
>     inlined from ‘constexpr void std::_Optional_payload_base<_Tp>::_M_reset() [with _Tp = ui_out_emit_type<ui_out_type_tuple>]’ at /usr/include/c++/16/optional:361:14,
>     inlined from ‘constexpr void std::_Optional_payload_base<_Tp>::_M_reset() [with _Tp = ui_out_emit_type<ui_out_type_tuple>]’ at /usr/include/c++/16/optional:358:7,
>     inlined from ‘std::_Optional_payload<_Tp, false, _Copy, _Move>::~_Optional_payload() [with _Tp = ui_out_emit_type<ui_out_type_tuple>; bool _Copy = false; bool _Move = false]’ at /usr/include/c++/16/optional:484:65,
>     inlined from ‘std::_Optional_base<ui_out_emit_type<ui_out_type_tuple>, false, false>::~_Optional_base()’ at /usr/include/c++/16/optional:508:12,
>     inlined from ‘std::optional<ui_out_emit_type<ui_out_type_tuple> >::~optional()’ at /usr/include/c++/16/optional:812:11,
>     inlined from ‘void info_sources_worker(ui_out*, bool, const info_sources_filter&)’ at gdb/symtab.c:4544:1:
> gdb/ui-out.h:428:18: warning: ‘*(ui_out_emit_type<ui_out_type_tuple>*)((char*)&output_tuple + offsetof(std::optional<ui_out_emit_type<ui_out_type_tuple> >,std::optional<ui_out_emit_type<ui_out_type_tuple> >::<unnamed>.std::_Optional_base<ui_out_emit_type<ui_out_type_tuple>, false, false>::_M_payload.std::_Optional_payload<ui_out_emit_type<ui_out_type_tuple>, false, false, false>::<unnamed>.std::_Optional_payload<ui_out_emit_type<ui_out_type_tuple>, true, false, false>::<unnamed>.std::_Optional_payload_base<ui_out_emit_type<ui_out_type_tuple> >::_M_payload)).ui_out_emit_type<ui_out_type_tuple>::m_uiout’ may be used uninitialized [-Wmaybe-uninitialized]
>   428 |     m_uiout->end (Type);
>       |     ~~~~~~~~~~~~~^~~~~~
> gdb/symtab.c: In function ‘void info_sources_worker(ui_out*, bool, const info_sources_filter&)’:
> gdb/symtab.c:4477:36: note: ‘*(ui_out_emit_type<ui_out_type_tuple>*)((char*)&output_tuple + offsetof(std::optional<ui_out_emit_type<ui_out_type_tuple> >,std::optional<ui_out_emit_type<ui_out_type_tuple> >::<unnamed>.std::_Optional_base<ui_out_emit_type<ui_out_type_tuple>, false, false>::_M_payload.std::_Optional_payload<ui_out_emit_type<ui_out_type_tuple>, false, false, false>::<unnamed>.std::_Optional_payload<ui_out_emit_type<ui_out_type_tuple>, true, false, false>::<unnamed>.std::_Optional_payload_base<ui_out_emit_type<ui_out_type_tuple> >::_M_payload)).ui_out_emit_type<ui_out_type_tuple>::m_uiout’ was declared here
>  4477 |   std::optional<ui_out_emit_tuple> output_tuple;
>       |                                    ^~~~~~~~~~~~
> ...
>
> The code pattern triggering this is:
> ...
>   std::optional<ui_out_emit_tuple> output_tuple;
>   for (objfile &objfile : current_program_space->objfiles ())
>     {
>       if (group_by_objfile)
> 	output_tuple.emplace (uiout, nullptr);
>       ...
>       if (group_by_objfile)
>         output_tuple.reset ();
>     }
> ...
>
> This is a false positive.
>
> We can get rid of the warning by making changes to the function:
> - making the reset unconditional, or
> - moving the output_tuple declaration into the loop.
>
> But instead, consider that the class ui_out_emit_type wraps a pointer.
>
> An std::optional<void *> has three possible states: !has_value, nullptr or
> non-nullptr.  If we don't use the nullptr state, using std::optional<void *>
> is overkill, and we can use nullptr instead of !has_value.
>
> Rewrite ui_out_emit_type to provide some of the std::optional interface:
> - default constructor
> - reset
> - emplace
> - has_value
> with protected access, and derive an opt_ui_out_emit_type which inherits those
> with public access.
>
> As a side effect, we get rid of the warning.
>
> Tested on aarch64-linux.
>
> Tested on x86_64-linux with gcc 7.5.0, where all the Wmaybe-uninitialized
> reported in PR31150 are now fixed.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31150
> ---
>  gdb/breakpoint.c            | 11 +++----
>  gdb/disasm.c                |  8 ++---
>  gdb/mi/mi-cmd-stack.c       |  2 +-
>  gdb/mi/mi-main.c            |  2 +-
>  gdb/python/py-framefilter.c |  6 ++--
>  gdb/record-btrace.c         |  8 ++---
>  gdb/symtab.c                |  4 +--
>  gdb/thread.c                |  2 +-
>  gdb/ui-out.h                | 63 ++++++++++++++++++++++++++++++++++---
>  9 files changed, 80 insertions(+), 26 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 0f5360c4ce5..9995c6ab21b 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -7150,8 +7150,8 @@ print_one_breakpoint_location (struct breakpoint *b,
>  	(uiout->test_flags (fix_breakpoint_script_output)
>  	 || fix_breakpoint_script_output_globally);
>  
> -      std::optional<ui_out_emit_tuple> tuple_emitter;
> -      std::optional<ui_out_emit_list> list_emitter;
> +      opt_ui_out_emit_tuple tuple_emitter;
> +      opt_ui_out_emit_list list_emitter;
>  
>        if (use_fixed_output)
>  	list_emitter.emplace (uiout, "script");
> @@ -7224,8 +7224,7 @@ print_one_breakpoint (breakpoint *b, const bp_location **last_loc, int allflag)
>      = (uiout->test_flags (fix_multi_location_breakpoint_output)
>         || fix_multi_location_breakpoint_output_globally);
>  
> -  std::optional<ui_out_emit_tuple> bkpt_tuple_emitter (std::in_place, uiout,
> -						       "bkpt");
> +  opt_ui_out_emit_tuple bkpt_tuple_emitter (uiout, "bkpt");
>    bool printed = print_one_breakpoint_location (b, NULL, 0, last_loc,
>  						allflag, false);
>  
> @@ -7260,7 +7259,7 @@ print_one_breakpoint (breakpoint *b, const bp_location **last_loc, int allflag)
>  		      || !b->first_loc ().enabled
>  		      || b->first_loc ().disabled_by_cond))))
>  	{
> -	  std::optional<ui_out_emit_list> locations_list;
> +	  opt_ui_out_emit_list locations_list;
>  
>  	  /* For MI version <= 2, keep the behavior where GDB outputs an invalid
>  	     MI record.  For later versions, place breakpoint locations in a
> @@ -10278,7 +10277,7 @@ watchpoint::print_it (const bpstat *bs) const
>  
>    string_file stb;
>  
> -  std::optional<ui_out_emit_tuple> tuple_emitter;
> +  opt_ui_out_emit_tuple tuple_emitter;
>    switch (this->type)
>      {
>      case bp_watchpoint:
> diff --git a/gdb/disasm.c b/gdb/disasm.c
> index 01361fe4323..67f56faaecd 100644
> --- a/gdb/disasm.c
> +++ b/gdb/disasm.c
> @@ -610,8 +610,8 @@ do_mixed_source_and_assembly_deprecated
>  
>    ui_out_emit_list asm_insns_list (uiout, "asm_insns");
>  
> -  std::optional<ui_out_emit_tuple> outer_tuple_emitter;
> -  std::optional<ui_out_emit_list> inner_list_emitter;
> +  opt_ui_out_emit_tuple outer_tuple_emitter;
> +  opt_ui_out_emit_list inner_list_emitter;
>  
>    for (i = 0; i < newlines; i++)
>      {
> @@ -767,8 +767,8 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch,
>  
>    ui_out_emit_list asm_insns_emitter (uiout, "asm_insns");
>  
> -  std::optional<ui_out_emit_tuple> tuple_emitter;
> -  std::optional<ui_out_emit_list> list_emitter;
> +  opt_ui_out_emit_tuple tuple_emitter;
> +  opt_ui_out_emit_list list_emitter;
>  
>    last_symtab = NULL;
>    last_line = 0;
> diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
> index a3b577032ce..335f7e1fbef 100644
> --- a/gdb/mi/mi-cmd-stack.c
> +++ b/gdb/mi/mi-cmd-stack.c
> @@ -533,7 +533,7 @@ list_arg_or_local (const struct frame_arg *arg, enum what_to_list what,
>        == lang_vars_shadowing::HIDE && shadowed && already_printed)
>      return;
>  
> -  std::optional<ui_out_emit_tuple> tuple_emitter;
> +  opt_ui_out_emit_tuple tuple_emitter;
>    if (values != PRINT_NO_VALUES || what == all)
>      tuple_emitter.emplace (uiout, nullptr);
>  
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index d39b8eeab87..dcf76db39a3 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -2455,7 +2455,7 @@ print_variable_or_computed (const char *expression, enum print_values values)
>    else
>      val = expr->evaluate ();
>  
> -  std::optional<ui_out_emit_tuple> tuple_emitter;
> +  opt_ui_out_emit_tuple tuple_emitter;
>    if (values != PRINT_NO_VALUES)
>      tuple_emitter.emplace (uiout, nullptr);
>    uiout->field_string ("name", expression);
> diff --git a/gdb/python/py-framefilter.c b/gdb/python/py-framefilter.c
> index b67431cf8c9..9899470cc0f 100644
> --- a/gdb/python/py-framefilter.c
> +++ b/gdb/python/py-framefilter.c
> @@ -321,7 +321,7 @@ py_print_single_arg (struct ui_out *out,
>    else
>      val = fv;
>  
> -  std::optional<ui_out_emit_tuple> maybe_tuple;
> +  opt_ui_out_emit_tuple maybe_tuple;
>  
>    /*  MI has varying rules for tuples, but generally if there is only
>        one element in each item in the list, do not start a tuple.  The
> @@ -557,7 +557,7 @@ enumerate_locals (PyObject *iter,
>        struct symbol *sym;
>        const struct block *sym_block;
>        int local_indent = 8 + (8 * indent);
> -      std::optional<ui_out_emit_tuple> tuple;
> +      opt_ui_out_emit_tuple tuple;
>  
>        gdbpy_ref<> item (PyIter_Next (iter));
>        if (item == NULL)
> @@ -814,7 +814,7 @@ py_print_frame (PyObject *filter, frame_filter_flags flags,
>        return EXT_LANG_BT_OK;
>      }
>  
> -  std::optional<ui_out_emit_tuple> tuple;
> +  opt_ui_out_emit_tuple tuple;
>  
>    /* -stack-list-locals does not require a
>       wrapping frame attribute.  */
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index dae243ee2db..5c1bb606130 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -763,8 +763,8 @@ btrace_find_line_range (CORE_ADDR pc)
>  
>  static void
>  btrace_print_lines (struct btrace_line_range lines, struct ui_out *uiout,
> -		    std::optional<ui_out_emit_tuple> *src_and_asm_tuple,
> -		    std::optional<ui_out_emit_list> *asm_list,
> +		    opt_ui_out_emit_tuple *src_and_asm_tuple,
> +		    opt_ui_out_emit_list *asm_list,
>  		    gdb_disassembly_flags flags)
>  {
>    print_source_lines_flags psl_flags;
> @@ -803,8 +803,8 @@ btrace_insn_history (struct ui_out *uiout,
>  
>    ui_out_emit_list list_emitter (uiout, "asm_insns");
>  
> -  std::optional<ui_out_emit_tuple> src_and_asm_tuple;
> -  std::optional<ui_out_emit_list> asm_list;
> +  opt_ui_out_emit_tuple src_and_asm_tuple;
> +  opt_ui_out_emit_list asm_list;
>  
>    gdb_pretty_print_disassembler disasm (gdbarch, uiout);
>  
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index b082a488b88..18fdc66d1f7 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -4474,8 +4474,8 @@ info_sources_worker (struct ui_out *uiout,
>    output_source_filename_data data (uiout, filter);
>  
>    ui_out_emit_list results_emitter (uiout, "files");
> -  std::optional<ui_out_emit_tuple> output_tuple;
> -  std::optional<ui_out_emit_list> sources_list;
> +  opt_ui_out_emit_tuple output_tuple;
> +  opt_ui_out_emit_list sources_list;
>  
>    gdb_assert (group_by_objfile || uiout->is_mi_like_p ());
>  
> diff --git a/gdb/thread.c b/gdb/thread.c
> index e14254619e3..5fa2bb6915c 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -1296,7 +1296,7 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads,
>      /* For backward compatibility, we make a list for MI.  A table is
>         preferable for the CLI, though, because it shows table
>         headers.  */
> -    std::optional<ui_out_emit_list> list_emitter;
> +    opt_ui_out_emit_list list_emitter;
>      std::optional<ui_out_emit_table> table_emitter;
>  
>      /* We'll be switching threads temporarily below.  */
> diff --git a/gdb/ui-out.h b/gdb/ui-out.h
> index 26d4a6b7d0e..8717d93539d 100644
> --- a/gdb/ui-out.h
> +++ b/gdb/ui-out.h
> @@ -418,25 +418,80 @@ class ui_out_emit_type
>  public:
>  
>    ui_out_emit_type (struct ui_out *uiout, const char *id = nullptr)
> -    : m_uiout (uiout)
>    {
> -    uiout->begin (Type, id);
> +    emplace (uiout, id);
>    }
>  
>    ~ui_out_emit_type ()
>    {
> -    m_uiout->end (Type);
> +    reset ();
>    }
>  
>    DISABLE_COPY_AND_ASSIGN (ui_out_emit_type);
>  
> +protected:
> +
> +  ui_out_emit_type () = default;
> +
> +  /* Support the std::optional interface, see opt_ui_out_emit_type.  */
> +  ui_out_emit_type &emplace (struct ui_out *uiout, const char *id = nullptr)
> +  {
> +    reset ();
> +    uiout->begin (Type, id);
> +    m_uiout = uiout;
> +    return *this;
> +  }
> +
> +  /* Support the std::optional interface, see opt_ui_out_emit_type.  */
> +  void reset () noexcept
> +  {
> +    if (!has_value ())
> +      return;
> +
> +    m_uiout->end (Type);
> +    m_uiout = nullptr;
> +  }
> +
> +  /* Support the std::optional interface, see opt_ui_out_emit_type.  */
> +  bool has_value () const noexcept
> +  {
> +    return m_uiout != nullptr;
> +  }
> +
>  private:
> +  struct ui_out *m_uiout = nullptr;
> +};
>  
> -  struct ui_out *m_uiout;
> +/* Stand-in for std::optional<ui_out_emit_type<Type>>.
> +   Support (as needed) some of the std::optional interface, allowing it to be
> +   used as if it were std::optional<ui_out_emit_type<Type>>.  There are 2
> +   reasons why we don't want to use the std::optional form:
> +   - Wmaybe-uninitialized false positives, and
> +   - it's overkill.  With std::optional, we can have three states:
> +     - !has_value ()
> +     - has_value () && m_uiout == nullptr
> +     - has_value () && m_uiout != nullptr
> +     and we don't use the middle one.  */
> +template<ui_out_type Type>
> +class opt_ui_out_emit_type : ui_out_emit_type<Type>
> +{
> +  using base = ui_out_emit_type<Type>;
> +
> +public:
> +  opt_ui_out_emit_type () = default;
> +
> +  using base::base;
> +  using base::emplace;
> +  using base::reset;
> +  using base::has_value;
> +
> +  DISABLE_COPY_AND_ASSIGN (opt_ui_out_emit_type);
>  };
>  
>  using ui_out_emit_tuple = ui_out_emit_type<ui_out_type_tuple>;
> +using opt_ui_out_emit_tuple = opt_ui_out_emit_type<ui_out_type_tuple>;
>  using ui_out_emit_list = ui_out_emit_type<ui_out_type_list>;
> +using opt_ui_out_emit_list = opt_ui_out_emit_type<ui_out_type_list>;
>  

Did you consider adding something like this:

  namespace std
  {
  template<>
  class
  [[deprecated ("std::optional<ui_out_emit_tuple> is disabled; opt_ui_out_emit_tuple instead")]]
  optional<ui_out_emit_tuple>
  {
   public:
    /* Delete all constructors to prevent object creation.  */
    optional() = delete;
    optional(const optional&) = delete;
    optional(optional&&) = delete;
    template <typename... Args>
      optional(Args&&...) = delete;
  };
  }

This should be valid(ish) since C++14.  I say "ish" because I think
overriding things in std:: without providing an API compatible
replacement is I think itself bad.  But given we want this to not
compile I suspect this would be good enough.

You'd want to add something similar for ui_out_emit_list too.

The goal here is to enforce the use of your new std::optional
replacements.

thoughts?

Thanks,
Andrew