[PATCH] gdbsupport: add debug assertions in gdb::optional::get

[PATCH] gdbsupport: add debug assertions in gdb::optional::get

[Simon Marchi]

The libstdc++ version of optional contains some runtime checks enabled
when _GLIBCXX_DEBUG is defined.  I think it would be useful if our
version contained similar checks.

Add checks in the two `get` methods, also conditional on _GLIBCXX_DEBUG.
I think it's simpler to use that macro rather than introducing a new
GDB-specific one, as I think that if somebody is interested in enabling
these runtime checks, they'll also be interested in enabling the
libstdc++ runtime checks (and vice-versa).

I implemented these checks using gdb_assert.  Note that gdb_assert
throws (after querying the user), and we are in noexcept methods.  That
means that std::terminate / abort will immediately be called.  I think
this is ok, since if those were "real" _GLIBCXX_DEBUG checks, abort
would be called straight away.

If I add a dummy failure, it looks like so:

    $ ./gdb -q -nx --data-directory=data-directory
    /home/simark/src/binutils-gdb/gdb/../gdbsupport/gdb_optional.h:206: internal-error: T& gdb::optional<T>::get() [with T = int]: Assertion `this->has_value ()' failed.
    A problem internal to GDB has been detected,
    further debugging may prove unreliable.
    Quit this debugging session? (y or n) n
    [1]    658767 abort (core dumped)  ./gdb -q -nx --data-directory=data-directory

Change-Id: Iadfdcd131425bd2ca6a2de30d7b22e9b3cc67793
---
 gdbsupport/gdb_optional.h | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/gdbsupport/gdb_optional.h b/gdbsupport/gdb_optional.h
index e79ba2c52e61..745b2ba74886 100644
--- a/gdbsupport/gdb_optional.h
+++ b/gdbsupport/gdb_optional.h
@@ -200,8 +200,20 @@ class optional
   }
 
   /* The get operations have m_instantiated as a precondition.  */
-  T &get () noexcept { return m_item; }
-  constexpr const T &get () const noexcept { return m_item; }
+  T &get () noexcept
+  {
+#if defined(_GLIBCXX_DEBUG)
+    gdb_assert (this->has_value ());
+#endif
+    return m_item;
+  }
+  constexpr const T &get () const noexcept
+  {
+#if defined(_GLIBCXX_DEBUG)
+    gdb_assert (this->has_value ());
+#endif
+    return m_item;
+  }
 
   /* The object.  */
   union
-- 
2.32.0

Re: [PATCH] gdbsupport: add debug assertions in gdb::optional::get

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> The libstdc++ version of optional contains some runtime checks enabled
Simon> when _GLIBCXX_DEBUG is defined.  I think it would be useful if our
Simon> version contained similar checks.

I agree.  The patch looks good to me.

Tom

Re: [PATCH] gdbsupport: add debug assertions in gdb::optional::get

[Simon Marchi]

On 2021-08-02 1:19 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> The libstdc++ version of optional contains some runtime checks enabled
> Simon> when _GLIBCXX_DEBUG is defined.  I think it would be useful if our
> Simon> version contained similar checks.
> 
> I agree.  The patch looks good to me.
> 
> Tom
> 

Thanks, pushed.

Simon

Re: [PATCH] gdbsupport: add debug assertions in gdb::optional::get

[Simon Marchi]

> Thanks, pushed.
> 
> Simon

Doh, I did run the testsuite for this patch but forgot to look at the
results.  This patch regresses the following tests:

    gdb.base/info-macros.exp
    gdb.base/macscp.exp
    gdb.base/style.exp
    gdb.compile/compile-cplus-print.exp
    gdb.compile/compile-cplus.exp
    gdb.compile/compile.exp
    gdb.dwarf2/mac-fileno.exp
    gdb.gdb/python-helper.exp
    gdb.gdb/complaints.exp:
    gdb.gdb/python-interrupts.exp
    gdb.gdb/selftest.exp
    gdb.linespec/macro-relative.exp

I'll see if the fix is simple, if not I'll revert.

Simon

[PATCH 1/3] gdb: fix typo in complaint in dwarf2/macro.c

[Simon Marchi]

I saw this complaint when my code had some bug, and spotted the typo.
Fix it, and while at it mention DW_MACRO as well (it would be confusing
to only see DW_MACINFO with a file that uses a DWARF 5 .debug_macro
section).  I contemplated the idea of passing the knowledge of whether
we are dealing with a .debug_macro section or .debug_macinfo section, to
print only the right one.  But in the end, I don't think that trouble is
necessary for a complaint nobody is going to see.

Change-Id: I276ce8da65c3eac5304f64a1e246358ed29cdbbc
---
 gdb/dwarf2/macro.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/dwarf2/macro.c b/gdb/dwarf2/macro.c
index 2ecebe6173c0..bf687daedd71 100644
--- a/gdb/dwarf2/macro.c
+++ b/gdb/dwarf2/macro.c
@@ -330,7 +330,7 @@ skip_unknown_opcode (unsigned int opcode,
 
   if (opcode_definitions[opcode] == NULL)
     {
-      complaint (_("unrecognized DW_MACFINO opcode 0x%x"),
+      complaint (_("unrecognized DW_MACINFO or DW_MACRO opcode 0x%x"),
 		 opcode);
       return NULL;
     }
-- 
2.32.0

[PATCH 2/3] gdb: avoid dereferencing empty str_offsets_base optional in dwarf_decode_macros

[Simon Marchi]

Since 4d7188abfdf2 ("gdbsupport: add debug assertions in
gdb::optional::get"), some macro-related tests fail on Ubuntu 20.04 with
the system gcc 9.3.0 compiler when building with _GLIBCXX_DEBUG.  For
example, gdb.base/info-macros.exp results in:

   (gdb) break -qualified main
   /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/gdb_optional.h:206: internal-error: T& gdb::optional<T>::get() [with T = long unsigned int]: Assertion `this->has_value ()' failed.

The binary contains DWARF 4 debug info and includes a pre-standard
(pre-DWARF 5) .debug_macro section.  The CU doesn't have a
DW_AT_str_offsets_base attribute (which doesn't exist in DWARF 4).  The
field dwarf2_cu::str_offsets_base is therefore empty.  At
dwarf2/read.c:24138, we unconditionally read the value in the optional,
which triggers the assertion shown above.

The same thing happens when building the test program with DWARF 5 with
the same gcc compiler, as that version of gcc doesn't use indirect
string forms, even with DWARF 5.  So it still doesn't add a
DW_AT_str_offsets_base attribute on the CU.

Fix that by propagating down a gdb::optional<ULONGEST> for the str
offsets base instead of ULONGEST.  That value is only used in
dwarf_decode_macro_bytes, when encountering an "strx" macro operation
(DW_MACRO_define_strx or DW_MACRO_undef_strx).  Add a check there that
we indeed have a value in the optional before reading it.  This is
unlikely to happen, but could happen in theory with an erroneous file
that uses DW_MACRO_define_strx but does not provide a
DW_AT_str_offsets_base (in practice, some things would probably have
failed before and stopped processing of debug info).  I tested the
complaint by inverting the condition and using a clang-compiled binary,
which uses the strx operators.  This is the result:

    During symbol reading: use of DW_MACRO_define_strx with unknown string offsets base [in module /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/info-macros/info-macros]

The test now passes cleanly with the setup mentioned above, and the
testsuite looks on par with how it was before 4d7188abfdf2.

Change-Id: I7ebd2724beb7b9b4178872374c2a177aea696e77
---
 gdb/dwarf2/macro.c | 21 +++++++++++++++++----
 gdb/dwarf2/macro.h |  2 +-
 gdb/dwarf2/read.c  |  4 ++--
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/gdb/dwarf2/macro.c b/gdb/dwarf2/macro.c
index bf687daedd71..082c4e9672ae 100644
--- a/gdb/dwarf2/macro.c
+++ b/gdb/dwarf2/macro.c
@@ -429,7 +429,7 @@ dwarf_decode_macro_bytes (dwarf2_per_objfile *per_objfile,
 			  unsigned int offset_size,
 			  struct dwarf2_section_info *str_section,
 			  struct dwarf2_section_info *str_offsets_section,
-			  ULONGEST str_offsets_base,
+			  gdb::optional<ULONGEST> str_offsets_base,
 			  htab_t include_hash)
 {
   struct objfile *objfile = per_objfile->objfile;
@@ -575,15 +575,27 @@ dwarf_decode_macro_bytes (dwarf2_per_objfile *per_objfile,
 	    int offset_index = read_unsigned_leb128 (abfd, mac_ptr, &bytes_read);
 	    mac_ptr += bytes_read;
 
+	    /* Use of the strx operators requires a DW_AT_str_offsets_base.  */
+	    if (!str_offsets_base.has_value ())
+	      {
+		complaint (_("use of %s with unknown string offsets base "
+			     "[in module %s]"),
+			   (macinfo_type == DW_MACRO_define_strx
+			    ? "DW_MACRO_define_strx"
+			    : "DW_MACRO_undef_strx"),
+			   objfile_name (objfile));
+		break;
+	      }
+
 	    str_offsets_section->read (objfile);
 	    const gdb_byte *info_ptr = (str_offsets_section->buffer
-					+ str_offsets_base
+					+ *str_offsets_base
 					+ offset_index * offset_size);
 
 	    const char *macinfo_str = (macinfo_type == DW_MACRO_define_strx ?
 				       "DW_MACRO_define_strx" : "DW_MACRO_undef_strx");
 
-	    if (str_offsets_base + offset_index * offset_size
+	    if (*str_offsets_base + offset_index * offset_size
 		>= str_offsets_section->size)
 	      {
 		complaint (_("%s pointing outside of .debug_str_offsets section "
@@ -767,7 +779,8 @@ dwarf_decode_macros (dwarf2_per_objfile *per_objfile,
 		     const struct line_header *lh, unsigned int offset_size,
 		     unsigned int offset, struct dwarf2_section_info *str_section,
 		     struct dwarf2_section_info *str_offsets_section,
-		     ULONGEST str_offsets_base, int section_is_gnu)
+		     gdb::optional<ULONGEST> str_offsets_base,
+		     int section_is_gnu)
 {
   bfd *abfd;
   const gdb_byte *mac_ptr, *mac_end;
diff --git a/gdb/dwarf2/macro.h b/gdb/dwarf2/macro.h
index 5e77c913cadd..5a0539231249 100644
--- a/gdb/dwarf2/macro.h
+++ b/gdb/dwarf2/macro.h
@@ -30,7 +30,7 @@ extern void dwarf_decode_macros (dwarf2_per_objfile *per_objfile,
 				 unsigned int offset,
 				 dwarf2_section_info *str_section,
 				 dwarf2_section_info *str_offsets_section,
-				 ULONGEST str_offsets_base,
+				 gdb::optional<ULONGEST> str_offsets_base,
 				 int section_is_gnu);
 
 #endif /* GDB_DWARF2_MACRO_H */
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 6f1b453ef455..acabee3315f8 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -24122,7 +24122,7 @@ dwarf_decode_macros (struct dwarf2_cu *cu, unsigned int offset,
 
   struct dwarf2_section_info *str_offsets_section;
   struct dwarf2_section_info *str_section;
-  ULONGEST str_offsets_base;
+  gdb::optional<ULONGEST> str_offsets_base;
 
   if (cu->dwo_unit != nullptr)
     {
@@ -24135,7 +24135,7 @@ dwarf_decode_macros (struct dwarf2_cu *cu, unsigned int offset,
     {
       str_offsets_section = &per_objfile->per_bfd->str_offsets;
       str_section = &per_objfile->per_bfd->str;
-      str_offsets_base = *cu->str_offsets_base;
+      str_offsets_base = cu->str_offsets_base;
     }
 
   dwarf_decode_macros (per_objfile, builder, section, lh,
-- 
2.32.0

[PATCH 3/3] gdb/testsuite: fix gdb.base/info-macros.exp with clang

[Simon Marchi]

The test gdb.base/info-macros.exp says that it doesn't pass the "debug"
option to prepare_for_testing because that would cause -g to appear
after -g3 on the command line, and that would cause some gcc versions to
not include macro info.  I don't know what gcc versions this refers to.
I tested with gcc 4.8, and that works fine with -g after -g3.

The current state is problematic when testing with CC_FOR_TARGET=clang,
because then only -fdebug-macro is included.  No -g switch if included,
meaning we get a binary without any debug info, and the test fails.

One way to fix it would be to add "debug" to the options when the
compiler is clang.

However, the solution I chose was to specify "debug" in any case, even
for gcc.  Other macro tests such as gdb.base/macscp.exp do perfectly
fine with it.  Also, this lets the test use the debug flag specified by
the board file.  For example, we can test with GCC and DWARF 5, with:

    $ make check RUNTESTFLAGS="--target_board unix/gdb:debug_flags=-gdwarf-5" TESTS="gdb.base/info-macros.exp"

With the hard-coded -g3, this wouldn't actually test with DWARF 5.

Change-Id: I33fa92ee545007d3ae9c52c4bb2d5be6b5b698f1
---
 gdb/testsuite/gdb.base/info-macros.exp | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/testsuite/gdb.base/info-macros.exp b/gdb/testsuite/gdb.base/info-macros.exp
index 538279fd309a..f1d912a75cfa 100644
--- a/gdb/testsuite/gdb.base/info-macros.exp
+++ b/gdb/testsuite/gdb.base/info-macros.exp
@@ -21,13 +21,13 @@ if [using_fission] {
     return -1
 }
 
+set options {debug}
+
 get_compiler_info
 if { [test_compiler_info gcc*] } {
-    # Don't use "debug" here.  Otherwise "-g" would be appended to the gcc
-    # command line, possibly overriding "-g3" (depending on gcc version).
-    set options "additional_flags=-g3"
+    lappend options "additional_flags=-g3"
 } elseif { [test_compiler_info clang*] } {
-    set options "additional_flags=-fdebug-macro"
+    lappend options "additional_flags=-fdebug-macro"
 } else {
     untested "no compiler info"
     return -1
-- 
2.32.0

Re: [PATCH 1/3] gdb: fix typo in complaint in dwarf2/macro.c

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> I saw this complaint when my code had some bug, and spotted the typo.
Simon> Fix it, and while at it mention DW_MACRO as well (it would be confusing
Simon> to only see DW_MACINFO with a file that uses a DWARF 5 .debug_macro
Simon> section).  I contemplated the idea of passing the knowledge of whether
Simon> we are dealing with a .debug_macro section or .debug_macinfo section, to
Simon> print only the right one.  But in the end, I don't think that trouble is
Simon> necessary for a complaint nobody is going to see.

Makes sense to me.

Tom

Re: [PATCH 2/3] gdb: avoid dereferencing empty str_offsets_base optional in dwarf_decode_macros

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Fix that by propagating down a gdb::optional<ULONGEST> for the str
Simon> offsets base instead of ULONGEST. [...]

Looks good.  Thank you.

Tom

Re: [PATCH 3/3] gdb/testsuite: fix gdb.base/info-macros.exp with clang

[Tom Tromey]

Simon> The test gdb.base/info-macros.exp says that it doesn't pass the "debug"
Simon> option to prepare_for_testing because that would cause -g to appear
Simon> after -g3 on the command line, and that would cause some gcc versions to
Simon> not include macro info.  I don't know what gcc versions this refers to.
Simon> I tested with gcc 4.8, and that works fine with -g after -g3.

IIRC, a long time ago "-g3 -g" meant just "-g", but that this changed at
some point.

Simon> However, the solution I chose was to specify "debug" in any case, even
Simon> for gcc.  Other macro tests such as gdb.base/macscp.exp do perfectly
Simon> fine with it.  Also, this lets the test use the debug flag specified by
Simon> the board file.

Seems fine to me.  Thank you.

Tom

Re: [PATCH 3/3] gdb/testsuite: fix gdb.base/info-macros.exp with clang

[Simon Marchi]

On 2021-08-04 12:15 p.m., Tom Tromey wrote:
> Simon> The test gdb.base/info-macros.exp says that it doesn't pass the "debug"
> Simon> option to prepare_for_testing because that would cause -g to appear
> Simon> after -g3 on the command line, and that would cause some gcc versions to
> Simon> not include macro info.  I don't know what gcc versions this refers to.
> Simon> I tested with gcc 4.8, and that works fine with -g after -g3.
> 
> IIRC, a long time ago "-g3 -g" meant just "-g", but that this changed at
> some point.

Makes sense, that would explain why this bit existed.

> Simon> However, the solution I chose was to specify "debug" in any case, even
> Simon> for gcc.  Other macro tests such as gdb.base/macscp.exp do perfectly
> Simon> fine with it.  Also, this lets the test use the debug flag specified by
> Simon> the board file.
> 
> Seems fine to me.  Thank you.

Thanks, all 3 patches pushed.

Simon