This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
ui_out format strings for fields and styles (Re: [PATCH] Style "pwd" output)
On 6/5/19 4:21 PM, Pedro Alves wrote:
> On 6/5/19 2:42 PM, Tom Tromey wrote:
>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>>
>> Pedro> I wish we didn't have to split the lines across different calls though.
>> Pedro> I know we don't officially do i18n yet (*), but these kinds of changes will
>> Pedro> only make it more difficult to get there.
>>
>> Pedro> Maybe with something like:
>>
>> Pedro> if (strcmp (cwd.get (), current_directory) != 0)
>> Pedro> printf_unfiltered (_("Working directory <style=filename>%s<style/>\n (canonically <style=filename>%s<style/>).\n"),
>> Pedro> current_directory, cwd.get ());
>> Pedro> else
>> Pedro> printf_unfiltered (_("Working directory <style=filename>%s<style/>.\n"), current_directory);
>>
>> Pedro> I'm sure you've considered something like that; I think we've discussed it
>> Pedro> before. What are your current thoughts?
>>
>> I think what would be nice is a gdb-specific printf extension for this,
>> e.g.:
>>
>> printf_unfiltered ("Working directory %<%s%>\n",
>> ui_out_style_kind::FILE,
>> current_directory);
>>
>> %< takes a style argument and %> does not.
>>
>> However, for that to work, we'd need a GCC enhancement to let gdb modify
>> the printf format checking.
>
> While not so good looking, instead of a GCC enhancement, we could maybe
> do it the Linux way, like binutils did too:
>
> https://sourceware.org/ml/binutils/2018-02/msg00306.html
>
> See 871b3ab29e87c.
>
> So e.g., %pS would be a style, a %pN would be no style.
>
> printf_unfiltered ("Working directory %pS%s%pN\n",
> ui_out_style_kind::FILE,
> current_directory);
>
>>
>>
>> Note that if we want to be serious about i18n then there is also the
>> problem that the whole MI approach is not very i18n-friendly. There are
>> spots that split calls like this, but which can't easily be unsplit
>> (even with a printf extension) due to MI. So maybe something bigger is
>> needed.
>
> Right.
>
> So instead of e.g.:
>
> uiout->text ("\nWatchpoint ");
> uiout->field_int ("wpnum", b->number);
> uiout->text (" deleted because the program has left the block in\n"
> "which its expression is valid.\n");
>
> We could have:
>
> uiout->field_fmt ("\nWatchpoint %pF deleted because the program "
> "has left the block in\n"
> "which its expression is valid.\n"",
> int_field ("wpnum", b->number).ptr ());
>
> With ui_out_field being something like
>
> struct int_field
> {
> int_field (const char *field_name, int val);
>
> // need this because we can't pass a reference via va_args.
> void *ptr () { return this; }
>
> const char *m_field_name;
> int m_val;
> };
>
> There would then be another class for CORE_ADDR, another for strings,
> etc, matching the ui_out::field_int, ui_out::field_string, etc. methods.
> Or alternatively, we could have a single uiout_field class that records
> its type depending on which ctor was called.
>
Prototype time!
So I looked around a bit how to prototype this, and borrowed format_pieces,
and some code from printf_command.
I hooked up "%pF" for ui_out fields, and added that int_field struct above.
I reserved "%pS" and "%pN" for styling, but haven't really prototyped that.
Is there a global current style stack we can push a style to/from?
I tested this by loading a program under gdb, and using "start", and
checking that the breakpoint number came out as expected, both in
CLI mode:
Temporary breakpoint 1, main () at threads.c:57
57 long i = 0;
and in MI mode:
*stopped,reason="breakpoint-hit",disp="del",bkptno="1", ....
Pushed to the users/palves/format_strings branch.
>From bef62542b4f0f650e65ddcb72611116e73df9269 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 5 Jun 2019 09:17:16 +0100
Subject: [PATCH] Make ui_out::message support %pF, %pS, %pN
---
gdb/breakpoint.c | 25 ++++++++--------
gdb/common/format.c | 15 +++++++++-
gdb/common/format.h | 2 +-
gdb/ui-out.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
gdb/ui-out.h | 21 +++++++++++++
5 files changed, 133 insertions(+), 16 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 08083645e1e..c4270064275 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4927,10 +4927,10 @@ watchpoint_check (bpstat bs)
if (uiout->is_mi_like_p ())
uiout->field_string
("reason", async_reason_lookup (EXEC_ASYNC_WATCHPOINT_SCOPE));
- uiout->text ("\nWatchpoint ");
- uiout->field_int ("wpnum", b->number);
- uiout->text (" deleted because the program has left the block in\n"
- "which its expression is valid.\n");
+ uiout->message ("\nWatchpoint %pF deleted because the program has "
+ "left the block in\n"
+ "which its expression is valid.\n",
+ int_field ("wpnum", b->number).ptr ());
}
/* Make sure the watchpoint's commands aren't executed. */
@@ -6245,9 +6245,8 @@ print_one_breakpoint_location (struct breakpoint *b,
if (!part_of_multiple && b->ignore_count)
{
annotate_field (8);
- uiout->text ("\tignore next ");
- uiout->field_int ("ignore", b->ignore_count);
- uiout->text (" hits\n");
+ uiout->message ("\tignore next %pF hits\n",
+ int_field ("ignore", b->ignore_count).ptr ());
}
/* Note that an enable count of 1 corresponds to "enable once"
@@ -12428,18 +12427,18 @@ bkpt_print_it (bpstat bs)
annotate_breakpoint (b->number);
maybe_print_thread_hit_breakpoint (uiout);
- if (bp_temp)
- uiout->text ("Temporary breakpoint ");
- else
- uiout->text ("Breakpoint ");
if (uiout->is_mi_like_p ())
{
uiout->field_string ("reason",
async_reason_lookup (EXEC_ASYNC_BREAKPOINT_HIT));
uiout->field_string ("disp", bpdisp_text (b->disposition));
}
- uiout->field_int ("bkptno", b->number);
- uiout->text (", ");
+ if (bp_temp)
+ uiout->message ("Temporary breakpoint %pF, ",
+ int_field ("bkptno", b->number).ptr ());
+ else
+ uiout->message ("Breakpoint %pF, ",
+ int_field ("bkptno", b->number).ptr ());
return PRINT_SRC_AND_LOC;
}
diff --git a/gdb/common/format.c b/gdb/common/format.c
index fb3421e62bf..d33eab2b2b0 100644
--- a/gdb/common/format.c
+++ b/gdb/common/format.c
@@ -20,7 +20,7 @@
#include "common-defs.h"
#include "format.h"
-format_pieces::format_pieces (const char **arg)
+format_pieces::format_pieces (const char **arg, bool gdb_extensions)
{
const char *s;
char *f, *string;
@@ -251,6 +251,19 @@ format_pieces::format_pieces (const char **arg)
bad = 1;
if (seen_hash || seen_zero || seen_space || seen_plus)
bad = 1;
+
+ if (gdb_extensions)
+ {
+ switch (f[1])
+ {
+ case 'S':
+ case 'F':
+ case 'N':
+ f++;
+ break;
+ }
+ }
+
break;
case 's':
diff --git a/gdb/common/format.h b/gdb/common/format.h
index 058844cec7b..52c0b152bcd 100644
--- a/gdb/common/format.h
+++ b/gdb/common/format.h
@@ -70,7 +70,7 @@ class format_pieces
{
public:
- format_pieces (const char **arg);
+ format_pieces (const char **arg, bool gdb_extensions = false);
~format_pieces () = default;
DISABLE_COPY_AND_ASSIGN (format_pieces);
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 6851fd29c6a..9c987a6cfa6 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -24,6 +24,7 @@
#include "expression.h" /* For language.h */
#include "language.h"
#include "ui-out.h"
+#include "common/format.h"
#include <vector>
#include <memory>
@@ -547,7 +548,7 @@ ui_out::text (const char *string)
}
void
-ui_out::message (const char *format, ...)
+ui_out::call_do_message (const char *format, ...)
{
va_list args;
@@ -556,6 +557,89 @@ ui_out::message (const char *format, ...)
va_end (args);
}
+void
+ui_out::message (const char *format, ...)
+{
+ format_pieces fpieces (&format, true);
+
+ va_list args;
+ va_start (args, format);
+
+ for (auto &&piece : fpieces)
+ {
+ const char *current_substring = piece.string;
+
+ switch (piece.argclass)
+ {
+ case string_arg:
+ call_do_message (current_substring, va_arg (args, const char *));
+ break;
+ case wide_string_arg:
+ /* FIXME */
+ break;
+ case wide_char_arg:
+ /* FIXME */
+ break;
+ case long_long_arg:
+#ifdef PRINTF_HAS_LONG_LONG
+ call_do_message (current_substring, va_arg (args, long long));
+ break;
+#else
+ error (_("long long not supported in ui_out::message"));
+#endif
+ case int_arg:
+ call_do_message (current_substring, va_arg (args, int));
+ break;
+ case long_arg:
+ call_do_message (current_substring, va_arg (args, long));
+ break;
+ /* Handle floating-point values. */
+ case double_arg:
+ case long_double_arg:
+ case dec32float_arg:
+ case dec64float_arg:
+ case dec128float_arg:
+ /* FIXME */
+ break;
+ case ptr_arg:
+ switch (current_substring[2])
+ {
+ case 'F':
+ {
+ int_field *field = va_arg (args, int_field *);
+ field_int (field->name (), field->val ());
+ }
+ break;
+ case 'S':
+ /* Push style on stack? */
+ break;
+ case 'N':
+ /* Pop style from stack? */
+ break;
+ default:
+ call_do_message (current_substring, va_arg (args, void *));
+ break;
+ }
+ break;
+ case literal_piece:
+ /* Print a portion of the format string that has no
+ directives. Note that this will not include any ordinary
+ %-specs, but it might include "%%". That is why we use
+ printf_filtered and not puts_filtered here. Also, we
+ pass a dummy argument because some platforms have
+ modified GCC to include -Wformat-security by default,
+ which will warn here if there is no argument. */
+ call_do_message (current_substring, 0);
+ break;
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("failed internal consistency check"));
+ }
+ }
+
+ va_end (args);
+}
+
void
ui_out::wrap_hint (const char *identstring)
{
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 9eba70eedac..b20d9508330 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -83,6 +83,26 @@ enum class ui_out_style_kind
ADDRESS
};
+struct int_field
+{
+ int_field (const char *name, int val)
+ : m_name (name),
+ m_val (val)
+ {
+ }
+
+ /* We need this because we can't pass a reference via
+ va_args. */
+ const int_field *ptr () const { return this; }
+
+ const char *name () const {return m_name; }
+ int val () const {return m_val; }
+
+private:
+ const char *m_name;
+ int m_val;
+};
+
class ui_out
{
public:
@@ -181,6 +201,7 @@ class ui_out
{ return false; }
private:
+ void call_do_message (const char *format, ...);
ui_out_flags m_flags;
--
2.14.5