From: Simon Marchi Date: Wed, 20 Aug 2025 16:50:08 +0000 (-0400) Subject: gdbsupport: remove xmalloc in format_pieces X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=bd21dd6807893c76b94da522b6af14c6b8c745a8;p=thirdparty%2Fbinutils-gdb.git gdbsupport: remove xmalloc in format_pieces Remove the use of xmalloc (and the arbitrary allocation size) in format_pieces. This turned out a bit more involved than expected, but not too bad. format_pieces::m_storage is a buffer with multiple concatenated null-terminated strings, referenced by format_piece::string. Change this to an std::string, while keeping its purpose (use the std::string as a buffer with embedded null characters). However, because the std::string's internal buffer can be reallocated as it grows, and I do not want to hardcode a big reserved size like we have now, it's not possible to store the direct pointer to the string in format_piece::string. Those pointers would become stale as the buffer gets reallocated. Therefore, change format_piece to hold an index into the storage instead. Add format_pieces::piece_str for the callers to be able to access the piece's string. This requires changing the few callers, but in a trivial way. The selftest also needs to be updated. I want to keep the test cases as-is, where the expected pieces contain the expected string, and not hard-code an expected index. To achieve this, add the expected_format_piece structure. Note that the previous format_piece::operator== didn't compare the n_int_args fields, while the test provides expected values for that field. I guess that was a mistake. The new code checks it, and the test still passes. Change-Id: I80630ff60e01c8caaa800ae22f69a9a7660bc9e9 Reviewed-By: Keith Seitz --- diff --git a/gdb/printcmd.c b/gdb/printcmd.c index 53827ef115b..661d721fe6f 100644 --- a/gdb/printcmd.c +++ b/gdb/printcmd.c @@ -2703,7 +2703,6 @@ ui_printf (const char *arg, struct ui_file *stream) { int nargs_wanted; int i; - const char *current_substring; nargs_wanted = 0; for (auto &&piece : fpieces) @@ -2732,7 +2731,8 @@ ui_printf (const char *arg, struct ui_file *stream) i = 0; for (auto &&piece : fpieces) { - current_substring = piece.string; + const char *current_substring = fpieces.piece_str (piece); + switch (piece.argclass) { case string_arg: diff --git a/gdb/ui-out.c b/gdb/ui-out.c index 106a896c07f..7683927b9d5 100644 --- a/gdb/ui-out.c +++ b/gdb/ui-out.c @@ -586,7 +586,7 @@ ui_out::vmessage (const ui_file_style &in_style, const char *format, for (auto &&piece : fpieces) { - const char *current_substring = piece.string; + const char *current_substring = fpieces.piece_str (piece); gdb_assert (piece.n_int_args >= 0 && piece.n_int_args <= 2); int intvals[2] = { 0, 0 }; diff --git a/gdb/unittests/format_pieces-selftests.c b/gdb/unittests/format_pieces-selftests.c index c7d8ff012ac..2deff0f2ec3 100644 --- a/gdb/unittests/format_pieces-selftests.c +++ b/gdb/unittests/format_pieces-selftests.c @@ -29,17 +29,35 @@ namespace selftests { namespace format_pieces { +/* Like format_piece, but with the expected string hardcoded instead of an + index. */ + +struct expected_format_piece +{ + std::string_view str; + enum argclass argclass; + int n_int_args; +}; + /* Verify that parsing STR gives pieces equal to EXPECTED_PIECES. */ static void -check (const char *str, const std::vector &expected_pieces, +check (const char *str, + const std::vector &expected_pieces, bool gdb_format = false) { ::format_pieces pieces (&str, gdb_format); SELF_CHECK ((pieces.end () - pieces.begin ()) == expected_pieces.size ()); - SELF_CHECK (std::equal (pieces.begin (), pieces.end (), - expected_pieces.begin ())); + + for (auto it = pieces.begin (); it != pieces.end (); ++it) + { + auto &expected = expected_pieces[it - pieces.begin ()]; + + SELF_CHECK (expected.str == pieces.piece_str (*it)); + SELF_CHECK (expected.argclass == it->argclass); + SELF_CHECK (expected.n_int_args == it->n_int_args); + } } static void @@ -47,7 +65,7 @@ test_escape_sequences () { check ("This is an escape sequence: \\e", { - format_piece ("This is an escape sequence: \e", literal_piece, 0), + {"This is an escape sequence: \e", literal_piece, 0}, }); } @@ -58,11 +76,11 @@ test_format_specifier () see a trailing empty literal piece. */ check ("Hello\\t %d%llx%%d%d", /* ARI: %ll */ { - format_piece ("Hello\t ", literal_piece, 0), - format_piece ("%d", int_arg, 0), - format_piece ("%" LL "x", long_long_arg, 0), - format_piece ("%%d", literal_piece, 0), - format_piece ("%d", int_arg, 0), + {"Hello\t ", literal_piece, 0}, + {"%d", int_arg, 0}, + {"%" LL "x", long_long_arg, 0}, + {"%%d", literal_piece, 0}, + {"%d", int_arg, 0}, }); } @@ -71,13 +89,13 @@ test_gdb_formats () { check ("Hello\\t \"%p[%pF%ps%*.*d%p]\"", { - format_piece ("Hello\\t \"", literal_piece, 0), - format_piece ("%p[", ptr_arg, 0), - format_piece ("%pF", ptr_arg, 0), - format_piece ("%ps", ptr_arg, 0), - format_piece ("%*.*d", int_arg, 2), - format_piece ("%p]", ptr_arg, 0), - format_piece ("\"", literal_piece, 0), + {"Hello\\t \"", literal_piece, 0}, + {"%p[", ptr_arg, 0}, + {"%pF", ptr_arg, 0}, + {"%ps", ptr_arg, 0}, + {"%*.*d", int_arg, 2}, + {"%p]", ptr_arg, 0}, + {"\"", literal_piece, 0}, }, true); } @@ -89,38 +107,38 @@ test_format_int_sizes () { check ("Hello\\t %hu%lu%llu%zu", /* ARI: %ll */ { - format_piece ("Hello\t ", literal_piece, 0), - format_piece ("%hu", int_arg, 0), - format_piece ("%lu", long_arg, 0), - format_piece ("%" LL "u", long_long_arg, 0), - format_piece ("%zu", size_t_arg, 0) + {"Hello\t ", literal_piece, 0}, + {"%hu", int_arg, 0}, + {"%lu", long_arg, 0}, + {"%" LL "u", long_long_arg, 0}, + {"%zu", size_t_arg, 0}, }); check ("Hello\\t %hx%lx%llx%zx", /* ARI: %ll */ { - format_piece ("Hello\t ", literal_piece, 0), - format_piece ("%hx", int_arg, 0), - format_piece ("%lx", long_arg, 0), - format_piece ("%" LL "x", long_long_arg, 0), - format_piece ("%zx", size_t_arg, 0) + {"Hello\t ", literal_piece, 0}, + {"%hx", int_arg, 0}, + {"%lx", long_arg, 0}, + {"%" LL "x", long_long_arg, 0}, + {"%zx", size_t_arg, 0}, }); check ("Hello\\t %ho%lo%llo%zo", /* ARI: %ll */ { - format_piece ("Hello\t ", literal_piece, 0), - format_piece ("%ho", int_arg, 0), - format_piece ("%lo", long_arg, 0), - format_piece ("%" LL "o", long_long_arg, 0), - format_piece ("%zo", size_t_arg, 0) + {"Hello\t ", literal_piece, 0}, + {"%ho", int_arg, 0}, + {"%lo", long_arg, 0}, + {"%" LL "o", long_long_arg, 0}, + {"%zo", size_t_arg, 0}, }); check ("Hello\\t %hd%ld%lld%zd", /* ARI: %ll */ { - format_piece ("Hello\t ", literal_piece, 0), - format_piece ("%hd", int_arg, 0), - format_piece ("%ld", long_arg, 0), - format_piece ("%" LL "d", long_long_arg, 0), - format_piece ("%zd", size_t_arg, 0) + {"Hello\t ", literal_piece, 0}, + {"%hd", int_arg, 0}, + {"%ld", long_arg, 0}, + {"%" LL "d", long_long_arg, 0}, + {"%zd", size_t_arg, 0}, }); } @@ -129,8 +147,8 @@ test_windows_formats () { check ("rc%I64d", { - format_piece ("rc", literal_piece, 0), - format_piece ("%I64d", long_long_arg, 0), + {"rc", literal_piece, 0}, + {"%I64d", long_long_arg, 0}, }); } diff --git a/gdbserver/ax.cc b/gdbserver/ax.cc index 567ef7fb476..18fc8944c69 100644 --- a/gdbserver/ax.cc +++ b/gdbserver/ax.cc @@ -817,7 +817,6 @@ ax_printf (CORE_ADDR fn, CORE_ADDR chan, const char *format, { const char *f = format; int i; - const char *current_substring; int nargs_wanted; ax_debug ("Printf of \"%s\" with %d args", format, nargs); @@ -835,7 +834,8 @@ ax_printf (CORE_ADDR fn, CORE_ADDR chan, const char *format, i = 0; for (auto &&piece : fpieces) { - current_substring = piece.string; + const char *current_substring = fpieces.piece_str (piece); + ax_debug ("current substring is '%s', class is %d", current_substring, piece.argclass); switch (piece.argclass) diff --git a/gdbsupport/format.cc b/gdbsupport/format.cc index 9d538e983ac..145c8765dd6 100644 --- a/gdbsupport/format.cc +++ b/gdbsupport/format.cc @@ -97,10 +97,6 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions, *arg = s; } - /* Need extra space for the '\0's. Doubling the size is sufficient. */ - char *current_substring = (char *) xmalloc (strlen (string) * 2 + 1000); - m_storage.reset (current_substring); - /* Now scan the string for %-specs and see what kinds of args they want. argclass classifies the %-specs so we can give printf-type functions something of the right size. */ @@ -126,13 +122,12 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions, continue; } - const char *sub_start = current_substring; + std::string::size_type sub_start = m_storage.size (); - strncpy (current_substring, prev_start, f - 1 - prev_start); - current_substring += f - 1 - prev_start; - *current_substring++ = '\0'; + m_storage.append (prev_start, f - 1 - prev_start); + m_storage += '\0'; - if (*sub_start != '\0') + if (m_storage[sub_start] != '\0') m_pieces.emplace_back (sub_start, literal_piece, 0); const char *percent_loc = f - 1; @@ -374,7 +369,7 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions, f++; - sub_start = current_substring; + sub_start = m_storage.size (); if (lcount > 1 && !seen_i64 && USE_PRINTF_I64) { @@ -382,11 +377,9 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions, Convert %lld to %I64d. */ int length_before_ll = f - percent_loc - 1 - lcount; - strncpy (current_substring, percent_loc, length_before_ll); - strcpy (current_substring + length_before_ll, "I64"); - current_substring[length_before_ll + 3] = - percent_loc[length_before_ll + lcount]; - current_substring += length_before_ll + 4; + m_storage.append (percent_loc, length_before_ll); + m_storage += "I64"; + m_storage += percent_loc[length_before_ll + lcount]; } else if (this_argclass == wide_string_arg || this_argclass == wide_char_arg) @@ -394,18 +387,13 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions, /* Convert %ls or %lc to %s. */ int length_before_ls = f - percent_loc - 2; - strncpy (current_substring, percent_loc, length_before_ls); - strcpy (current_substring + length_before_ls, "s"); - current_substring += length_before_ls + 2; + m_storage.append (percent_loc, length_before_ls); + m_storage += "s"; } else - { - strncpy (current_substring, percent_loc, f - percent_loc); - current_substring += f - percent_loc; - } - - *current_substring++ = '\0'; + m_storage.append (percent_loc, f - percent_loc); + m_storage += '\0'; prev_start = f; m_pieces.emplace_back (sub_start, this_argclass, n_int_args); @@ -415,11 +403,9 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions, if (f > prev_start) { - const char *sub_start = current_substring; - - strncpy (current_substring, prev_start, f - prev_start); - current_substring += f - prev_start; - *current_substring++ = '\0'; + std::string::size_type sub_start = m_storage.size (); + m_storage.append (prev_start, f - prev_start); + /* No need for a final '\0', std::string already has one. */ m_pieces.emplace_back (sub_start, literal_piece, 0); } diff --git a/gdbsupport/format.h b/gdbsupport/format.h index 118b947c2d4..46dae2290a2 100644 --- a/gdbsupport/format.h +++ b/gdbsupport/format.h @@ -20,8 +20,6 @@ #ifndef GDBSUPPORT_FORMAT_H #define GDBSUPPORT_FORMAT_H -#include - #if defined(__MINGW32__) && !defined(PRINTF_HAS_LONG_LONG) # define USE_PRINTF_I64 1 # define PRINTF_HAS_LONG_LONG @@ -51,21 +49,15 @@ enum argclass struct format_piece { - format_piece (const char *str, enum argclass argc, int n) - : string (str), + format_piece (std::string::size_type start, enum argclass argc, int n) + : start (start), argclass (argc), n_int_args (n) - { - gdb_assert (str != nullptr); - } + {} - bool operator== (const format_piece &other) const - { - return (this->argclass == other.argclass - && std::string_view (this->string) == other.string); - } + /* Where this piece starts, within FORMAT_PIECES::M_STORAGE. */ + std::string::size_type start; - const char *string; enum argclass argclass; /* Count the number of preceding 'int' arguments that must be passed along. This is used for a width or precision of '*'. Note that @@ -95,10 +87,17 @@ public: return m_pieces.end (); } + /* Return the string associated to PIECE. */ + const char *piece_str (const format_piece &piece) + { return &m_storage[piece.start]; } + private: std::vector m_pieces; - gdb::unique_xmalloc_ptr m_storage; + + /* This is used as a buffer of concatenated null-terminated strings. The + individual strings are referenced by FORMAT_PIECE::START. */ + std::string m_storage; }; #endif /* GDBSUPPORT_FORMAT_H */