* [PATCH] gdbsupport: add debug assertions in gdb::optional::get @ 2021-07-29 18:42 Simon Marchi 2021-08-02 17:19 ` Tom Tromey 0 siblings, 1 reply; 11+ messages in thread From: Simon Marchi @ 2021-07-29 18:42 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] gdbsupport: add debug assertions in gdb::optional::get 2021-07-29 18:42 [PATCH] gdbsupport: add debug assertions in gdb::optional::get Simon Marchi @ 2021-08-02 17:19 ` Tom Tromey 2021-08-03 12:56 ` Simon Marchi 0 siblings, 1 reply; 11+ messages in thread From: Tom Tromey @ 2021-08-02 17:19 UTC (permalink / raw) To: Simon Marchi via Gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] gdbsupport: add debug assertions in gdb::optional::get 2021-08-02 17:19 ` Tom Tromey @ 2021-08-03 12:56 ` Simon Marchi 2021-08-03 13:14 ` Simon Marchi 0 siblings, 1 reply; 11+ messages in thread From: Simon Marchi @ 2021-08-03 12:56 UTC (permalink / raw) To: Tom Tromey, Simon Marchi via Gdb-patches 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] gdbsupport: add debug assertions in gdb::optional::get 2021-08-03 12:56 ` Simon Marchi @ 2021-08-03 13:14 ` Simon Marchi 2021-08-03 15:31 ` [PATCH 1/3] gdb: fix typo in complaint in dwarf2/macro.c Simon Marchi 0 siblings, 1 reply; 11+ messages in thread From: Simon Marchi @ 2021-08-03 13:14 UTC (permalink / raw) To: Tom Tromey, Simon Marchi via Gdb-patches > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] gdb: fix typo in complaint in dwarf2/macro.c 2021-08-03 13:14 ` Simon Marchi @ 2021-08-03 15:31 ` Simon Marchi 2021-08-03 15:31 ` [PATCH 2/3] gdb: avoid dereferencing empty str_offsets_base optional in dwarf_decode_macros Simon Marchi ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Simon Marchi @ 2021-08-03 15:31 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] gdb: avoid dereferencing empty str_offsets_base optional in dwarf_decode_macros 2021-08-03 15:31 ` [PATCH 1/3] gdb: fix typo in complaint in dwarf2/macro.c Simon Marchi @ 2021-08-03 15:31 ` Simon Marchi 2021-08-04 16:03 ` Tom Tromey 2021-08-03 15:31 ` [PATCH 3/3] gdb/testsuite: fix gdb.base/info-macros.exp with clang Simon Marchi 2021-08-04 15:52 ` [PATCH 1/3] gdb: fix typo in complaint in dwarf2/macro.c Tom Tromey 2 siblings, 1 reply; 11+ messages in thread From: Simon Marchi @ 2021-08-03 15:31 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] gdb: avoid dereferencing empty str_offsets_base optional in dwarf_decode_macros 2021-08-03 15:31 ` [PATCH 2/3] gdb: avoid dereferencing empty str_offsets_base optional in dwarf_decode_macros Simon Marchi @ 2021-08-04 16:03 ` Tom Tromey 0 siblings, 0 replies; 11+ messages in thread From: Tom Tromey @ 2021-08-04 16:03 UTC (permalink / raw) To: Simon Marchi via Gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] gdb/testsuite: fix gdb.base/info-macros.exp with clang 2021-08-03 15:31 ` [PATCH 1/3] gdb: fix typo in complaint in dwarf2/macro.c Simon Marchi 2021-08-03 15:31 ` [PATCH 2/3] gdb: avoid dereferencing empty str_offsets_base optional in dwarf_decode_macros Simon Marchi @ 2021-08-03 15:31 ` Simon Marchi 2021-08-04 16:15 ` Tom Tromey 2021-08-04 15:52 ` [PATCH 1/3] gdb: fix typo in complaint in dwarf2/macro.c Tom Tromey 2 siblings, 1 reply; 11+ messages in thread From: Simon Marchi @ 2021-08-03 15:31 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] gdb/testsuite: fix gdb.base/info-macros.exp with clang 2021-08-03 15:31 ` [PATCH 3/3] gdb/testsuite: fix gdb.base/info-macros.exp with clang Simon Marchi @ 2021-08-04 16:15 ` Tom Tromey 2021-08-04 19:39 ` Simon Marchi 0 siblings, 1 reply; 11+ messages in thread From: Tom Tromey @ 2021-08-04 16:15 UTC (permalink / raw) To: Simon Marchi via Gdb-patches 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] gdb/testsuite: fix gdb.base/info-macros.exp with clang 2021-08-04 16:15 ` Tom Tromey @ 2021-08-04 19:39 ` Simon Marchi 0 siblings, 0 replies; 11+ messages in thread From: Simon Marchi @ 2021-08-04 19:39 UTC (permalink / raw) To: Tom Tromey, Simon Marchi via Gdb-patches 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] gdb: fix typo in complaint in dwarf2/macro.c 2021-08-03 15:31 ` [PATCH 1/3] gdb: fix typo in complaint in dwarf2/macro.c Simon Marchi 2021-08-03 15:31 ` [PATCH 2/3] gdb: avoid dereferencing empty str_offsets_base optional in dwarf_decode_macros Simon Marchi 2021-08-03 15:31 ` [PATCH 3/3] gdb/testsuite: fix gdb.base/info-macros.exp with clang Simon Marchi @ 2021-08-04 15:52 ` Tom Tromey 2 siblings, 0 replies; 11+ messages in thread From: Tom Tromey @ 2021-08-04 15:52 UTC (permalink / raw) To: Simon Marchi via Gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-08-04 19:40 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-29 18:42 [PATCH] gdbsupport: add debug assertions in gdb::optional::get Simon Marchi 2021-08-02 17:19 ` Tom Tromey 2021-08-03 12:56 ` Simon Marchi 2021-08-03 13:14 ` Simon Marchi 2021-08-03 15:31 ` [PATCH 1/3] gdb: fix typo in complaint in dwarf2/macro.c Simon Marchi 2021-08-03 15:31 ` [PATCH 2/3] gdb: avoid dereferencing empty str_offsets_base optional in dwarf_decode_macros Simon Marchi 2021-08-04 16:03 ` Tom Tromey 2021-08-03 15:31 ` [PATCH 3/3] gdb/testsuite: fix gdb.base/info-macros.exp with clang Simon Marchi 2021-08-04 16:15 ` Tom Tromey 2021-08-04 19:39 ` Simon Marchi 2021-08-04 15:52 ` [PATCH 1/3] gdb: fix typo in complaint in dwarf2/macro.c Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).