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

[Tom de Vries <tdevries@suse.de>]

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>;
 
 /* Start a new table on construction, and end the table on
    destruction.  */

base-commit: 50b7267f00c094614ab4efb43f7aac0507a436e7
-- 
2.51.0