Fix GDB build when using --disable-gdbmi

Message ID 20190510183512.9955-1-simon.marchi@efficios.com
State New
Headers show
Series
  • Fix GDB build when using --disable-gdbmi
Related show

Commit Message

Simon Marchi May 10, 2019, 6:35 p.m.
Since commit

    b4be1b064860 ("Fix MI output for multi-location breakpoints")

we get this error when building with --disable-gdbmi:

      CXXLD  gdb
    /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:6358: error: undefined reference to 'mi_multi_location_breakpoint_output_fixed(ui_out*)'

This is due to breakpoint.c using a function defined in mi/mi-main.c, even
though mi/mi-main.c isn't included in the build.

To fix it, I added a config.h macro, HAVE_GDBMI, defined if we build
with GDB/MI support.  We can then use it in breakpoint.c to
conditionally use mi_multi_location_breakpoint_output_fixed.  If
building without GDB/MI, the value of use_fixed_output is not really
important, as it is only used to fix an issue when printing breakpoints
in MI.

gdb/ChangeLog:

	* configure.ac: Define HAVE_GDBMI if building with GDB/MI
	support.
	* configure, config.in: Re-generate.
	* breakpoint.c (print_one_breakpoint): Use
	mi_multi_location_breakpoint_output_fixed only if HAVE_GDBMI is
	defined.
---
 gdb/breakpoint.c | 7 ++++++-
 gdb/config.in    | 3 +++
 gdb/configure    | 3 +++
 gdb/configure.ac | 1 +
 4 files changed, 13 insertions(+), 1 deletion(-)

-- 
2.21.0

Comments

Tom Tromey May 10, 2019, 6:47 p.m. | #1
>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:


Simon> To fix it, I added a config.h macro, HAVE_GDBMI, defined if we build
Simon> with GDB/MI support.  We can then use it in breakpoint.c to
Simon> conditionally use mi_multi_location_breakpoint_output_fixed.  If
Simon> building without GDB/MI, the value of use_fixed_output is not really
Simon> important, as it is only used to fix an issue when printing breakpoints
Simon> in MI.

I'm not sure if it's better or not, but ui_out has a "flags" feature
that could be applied to this.  This would mean not having to have a #if.

Tom
Simon Marchi May 10, 2019, 6:57 p.m. | #2
On 2019-05-10 2:47 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

> 

> Simon> To fix it, I added a config.h macro, HAVE_GDBMI, defined if we build

> Simon> with GDB/MI support.  We can then use it in breakpoint.c to

> Simon> conditionally use mi_multi_location_breakpoint_output_fixed.  If

> Simon> building without GDB/MI, the value of use_fixed_output is not really

> Simon> important, as it is only used to fix an issue when printing breakpoints

> Simon> in MI.

> 

> I'm not sure if it's better or not, but ui_out has a "flags" feature

> that could be applied to this.  This would mean not having to have a #if.


I don't think this will work (or I don't understand what you suggest).

If we do

  bool use_fixed_output = ui_out->test_flags(ui_out_flag::fixed_breakpoint_output);

That would require the mi_ui_out object to be created with that flag.  However,
this option can be toggled on after the mi_ui_out object has been created (through
the MI command -fix-multi-location-breakpoint-output).

One way to make it work would be, when the -fix-multi-location-breakpoint-output
command is issued, we go through all existing ui_out objects and set that flag.
But I don't think there's an easy way of going through all existing ui_out objects.

If you have any ideas for a more elegant way to make it work, I would be up for it,
because it's true that sprinkling #ifdefs is not the best idea.

Simon
Tom Tromey May 10, 2019, 7:11 p.m. | #3
>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:


Simon> That would require the mi_ui_out object to be created with that flag.  However,
Simon> this option can be toggled on after the mi_ui_out object has been created (through
Simon> the MI command -fix-multi-location-breakpoint-output).

Ah, I didn't realize that.

Simon> One way to make it work would be, when the -fix-multi-location-breakpoint-output
Simon> command is issued, we go through all existing ui_out objects and set that flag.
Simon> But I don't think there's an easy way of going through all existing ui_out objects.

Simon> If you have any ideas for a more elegant way to make it work, I would be up for it,
Simon> because it's true that sprinkling #ifdefs is not the best idea.

It's maybe strange that -fix-multi-location-breakpoint-output affects
all MI channels and not just the ones associated with the request.  It
seems like if there are multiple MI channels, then this could confuse
some client.  But then, making it per-channel would make it not work
nicely if a new MI-out is created for a specific command (which I guess
is done by mi_load_progress, not sure about things like interpreter-exec).

I guess maybe if there's going to be a global, it could just be stuck in
breakpoint.c and then flags used for the ui-out version check currently
done in mi_multi_location_breakpoint_output_fixed.  I'm not really sure
this is any better -- globals are bad, but it does avoid the #if.

Tom
Simon Marchi May 10, 2019, 8:19 p.m. | #4
On 2019-05-10 3:11 p.m., Tom Tromey wrote:
> It's maybe strange that -fix-multi-location-breakpoint-output affects

> all MI channels and not just the ones associated with the request.  It

> seems like if there are multiple MI channels, then this could confuse

> some client.  But then, making it per-channel would make it not work

> nicely if a new MI-out is created for a specific command (which I guess

> is done by mi_load_progress, not sure about things like interpreter-exec).


I think the reason it applies globally is that it's not worth the trouble.  That
command only exists to help MI clients that don't want to upgrade to mi3, but want
the fixed output.  Supporting multiple MI clients with different requirements w.r.t.
whether they want the fixed output would be a bit stretched.

> I guess maybe if there's going to be a global, it could just be stuck in

> breakpoint.c and then flags used for the ui-out version check currently

> done in mi_multi_location_breakpoint_output_fixed.  I'm not really sure

> this is any better -- globals are bad, but it does avoid the #if.


I tried this, and I prefer the result.  WDYT?


From 29d88013830589140f395975c43687053aa30dce Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>

Date: Fri, 10 May 2019 12:32:41 -0400
Subject: [PATCH] Fix GDB build when using --disable-gdbmi

Since commit

    b4be1b064860 ("Fix MI output for multi-location breakpoints")

we get this error when building with --disable-gdbmi:

      CXXLD  gdb
    /home/smarchi/src/binutils-gdb/gdb/breakpoint.c:6358: error: undefined reference to 'mi_multi_location_breakpoint_output_fixed(ui_out*)'

This is due to breakpoint.c using a function defined in mi/mi-main.c, even
though mi/mi-main.c isn't included in the build.

To fix it, use the flags feature of ui_out.  mi_ui_out has the new
fix_multi_location_breakpoint_output flag set for versions >= 3.  Also,
move the global variable fix_multi_location_breakpoint_output to
breakpoint.c, so it can be read there even when we build without MI.  I
renamed it to fix_multi_location_breakpoint_output_globally so it
doesn't clash with the new enumerator.

gdb/ChangeLog:

	* breakpoint.h (fix_multi_location_breakpoint_output_globally):
	New variable declaration.
	* breakpoint.c (fix_multi_location_breakpoint_output_globally):
	New variable.
	(print_one_breakpoint): Use ui_out::test_flags and new global
	variable to compute use_fixed_output.
	* mi/mi-main.h (mi_multi_location_breakpoint_output_fixed):
	Remove.
	* mi/mi-main.c (fix_multi_location_breakpoint_output): Remove.
	(mi_multi_location_breakpoint_output_fixed): Remove.
	(mi_cmd_fix_multi_location_breakpoint_output): Adjust to set the
	new variable.
	* mi/mi-out.c (mi_ui_out::mi_ui_out): Set
	fix_multi_location_breakpoint_output flag if version >= 3.
	* ui-out.h (enum ui_out_flag)
	<fix_multi_location_breakpoint_output>: New enumerator.
---
 gdb/breakpoint.c |  8 +++++++-
 gdb/breakpoint.h |  5 +++++
 gdb/mi/mi-main.c | 20 +-------------------
 gdb/mi/mi-main.h |  8 --------
 gdb/mi/mi-out.c  |  4 +++-
 gdb/ui-out.h     |  7 ++++---
 6 files changed, 20 insertions(+), 32 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f6d2f36d0a40..3612318646c0 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6349,13 +6349,19 @@ print_one_breakpoint_location (struct breakpoint *b,
     }
 }

+/* See breakpoint.h. */
+
+bool fix_multi_location_breakpoint_output_globally = false;
+
 static void
 print_one_breakpoint (struct breakpoint *b,
 		      struct bp_location **last_loc,
 		      int allflag)
 {
   struct ui_out *uiout = current_uiout;
-  bool use_fixed_output = mi_multi_location_breakpoint_output_fixed (uiout);
+  bool use_fixed_output
+    = (uiout->test_flags (fix_multi_location_breakpoint_output)
+       || fix_multi_location_breakpoint_output_globally);

   gdb::optional<ui_out_emit_tuple> bkpt_tuple_emitter (gdb::in_place, uiout, "bkpt");
   print_one_breakpoint_location (b, NULL, 0, last_loc, allflag);
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index a91e3e334cfd..3646ea63cb81 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1670,4 +1670,9 @@ extern void print_breakpoint (breakpoint *bp);
 /* Command element for the 'commands' command.  */
 extern cmd_list_element *commands_cmd_element;

+/* Whether to use the fixed output when printing information about a
+   multi-location breakpoint (see PR 9659).  */
+
+extern bool fix_multi_location_breakpoint_output_globally;
+
 #endif /* !defined (BREAKPOINT_H) */
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 2b9883cb99f4..01786c3c1e84 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -2699,31 +2699,13 @@ mi_cmd_trace_frame_collected (const char *command, char **argv, int argc)
   }
 }

-/* Whether to use the fixed output when printing information about a
-   multi-location breakpoint (see PR 9659).  */
-
-static bool fix_multi_location_breakpoint_output = false;
-
 /* See mi/mi-main.h.  */

 void
 mi_cmd_fix_multi_location_breakpoint_output (const char *command, char **argv,
 					     int argc)
 {
-  fix_multi_location_breakpoint_output = true;
-}
-
-/* See mi/mi-main.h.  */
-
-bool
-mi_multi_location_breakpoint_output_fixed (ui_out *uiout)
-{
-  mi_ui_out *mi_uiout = dynamic_cast<mi_ui_out *> (uiout);
-
-  if (mi_uiout == nullptr)
-    return false;
-
-  return mi_uiout->version () >= 3 || fix_multi_location_breakpoint_output;
+  fix_multi_location_breakpoint_output_globally = true;
 }

 void
diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
index 72c4e594287a..1986228d22bd 100644
--- a/gdb/mi/mi-main.h
+++ b/gdb/mi/mi-main.h
@@ -59,12 +59,4 @@ extern struct mi_suppress_notification mi_suppress_notification;
 extern void mi_cmd_fix_multi_location_breakpoint_output (const char *command,
 							 char **argv, int argc);

-/* Return whether -break-list, -break-insert, =breakpoint-created and
-   =breakpoint-modified should use the "fixed" output format (see PR
-   9659).
-
-   Return false if UIOUT is not an MI UI.  */
-
-extern bool mi_multi_location_breakpoint_output_fixed (ui_out *uiout);
-
 #endif /* MI_MI_MAIN_H */
diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c
index e485beef69cd..d8bee0f39279 100644
--- a/gdb/mi/mi-out.c
+++ b/gdb/mi/mi-out.c
@@ -280,7 +280,9 @@ mi_ui_out::version ()
 /* Constructor for an `mi_out_data' object.  */

 mi_ui_out::mi_ui_out (int mi_version)
-: m_suppress_field_separator (false),
+: ui_out (mi_version >= 3
+	  ? fix_multi_location_breakpoint_output : (ui_out_flag) 0),
+  m_suppress_field_separator (false),
   m_suppress_output (false),
   m_mi_version (mi_version)
 {
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 8d183060b533..9eba70eedac6 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -49,9 +49,10 @@ enum ui_align

 /* flags enum */
 enum ui_out_flag
-  {
-    ui_source_list = (1 << 0),
-  };
+{
+  ui_source_list = (1 << 0),
+  fix_multi_location_breakpoint_output = (1 << 1),
+};

 DEF_ENUM_FLAGS_TYPE (ui_out_flag, ui_out_flags);

-- 
2.21.0
Tom Tromey May 10, 2019, 9:16 p.m. | #5
>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:


Simon> I tried this, and I prefer the result.  WDYT?

Thanks for doing this.  It looks good to me.

Tom
Simon Marchi May 10, 2019, 10:07 p.m. | #6
On 2019-05-10 5:16 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

> 

> Simon> I tried this, and I prefer the result.  WDYT?

> 

> Thanks for doing this.  It looks good to me.

> 

> Tom

> 


Thanks, I pushed it.

Simon

Patch

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f6d2f36d0a40..8f75c7658080 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6355,7 +6355,12 @@  print_one_breakpoint (struct breakpoint *b,
 		      int allflag)
 {
   struct ui_out *uiout = current_uiout;
-  bool use_fixed_output = mi_multi_location_breakpoint_output_fixed (uiout);
+  bool use_fixed_output
+#ifdef HAVE_GDBMI
+    = mi_multi_location_breakpoint_output_fixed (uiout);
+#else
+    = true;
+#endif
 
   gdb::optional<ui_out_emit_tuple> bkpt_tuple_emitter (gdb::in_place, uiout, "bkpt");
   print_one_breakpoint_location (b, NULL, 0, last_loc, allflag);
diff --git a/gdb/config.in b/gdb/config.in
index c0291fbd9c53..e5a19bda6bd6 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -174,6 +174,9 @@ 
 /* Define if <sys/procfs.h> has fpregset_t. */
 #undef HAVE_FPREGSET_T
 
+/* Define to 1 if building with GDB/MI support. */
+#undef HAVE_GDBMI
+
 /* Define to 1 if you have the `getauxval' function. */
 #undef HAVE_GETAUXVAL
 
diff --git a/gdb/configure b/gdb/configure
index 15a96afcca8a..ca797473bfc1 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -6825,6 +6825,9 @@  if test x"$enable_gdbmi" = xyes; then
     CONFIG_DEPS="$CONFIG_DEPS \$(SUBDIR_MI_DEPS)"
     CONFIG_SRCS="$CONFIG_SRCS \$(SUBDIR_MI_SRCS)"
     ENABLE_CFLAGS="$ENABLE_CFLAGS \$(SUBDIR_MI_CFLAGS)"
+
+$as_echo "#define HAVE_GDBMI 1" >>confdefs.h
+
   fi
 fi
 
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 1318c8d00873..66f72c5158d4 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -315,6 +315,7 @@  if test x"$enable_gdbmi" = xyes; then
     CONFIG_DEPS="$CONFIG_DEPS \$(SUBDIR_MI_DEPS)"
     CONFIG_SRCS="$CONFIG_SRCS \$(SUBDIR_MI_SRCS)"
     ENABLE_CFLAGS="$ENABLE_CFLAGS \$(SUBDIR_MI_CFLAGS)"
+    AC_DEFINE([HAVE_GDBMI], 1, [Define to 1 if building with GDB/MI support.])
   fi
 fi