diff mbox

[v2,5/7] btrace, gdbserver: remove the to_supports_btrace target method

Message ID A78C989F6D9628469189715575E55B236964BE94@IRSMSX104.ger.corp.intel.com
State New
Headers show

Commit Message

Metzger, Markus T Feb. 26, 2018, 1:08 p.m. UTC
Hello Maciej,

> Process .../gdb/testsuite/outputs/gdb.base/advance/advance created; pid =
> 25519 Listening on port 2346 target remote 1.2.3.4:2346 Remote debugging using
> 1.2.3.4:2346 Reading symbols from .../lib/ld.so.1...done.
> 0x77fc8de0 in __start () from .../lib/ld.so.1 Protocol error: qXfer:btrace-conf
> (read-btrace-conf) conflicting enabled responses.
> (gdb) continue
> The program is not being run.
> (gdb) FAIL: gdb.base/advance.exp: can't run to main
> 
> See the attached RSP packet exchange log for details.  Please investigate.

Below is a patch to address this.  I tested it on IA by undefining HAVE_LINUX_BTRACE.
Does it fix the issue you reported?

thanks,
Markus.

---

commit c4d017399e2cc62cfed793bb93967bd6b3d573bb
Author: Markus Metzger <markus.t.metzger@intel.com>
Date:   Mon Feb 26 11:59:43 2018 +0100

    btrace, gdbserver: check btrace target pointers
    
    By removing the supports_btrace gdbserver target method we relied on GDB trying
    to enable branch tracing and failing on the attempt.
    
    For targets that do not provide the btrace methods, however, an initial request
    from GDB for the branch trace configuration to detect whether gdbserver is
    already recording resulted in a protocol error.
    
    Have the btrace target methods throw a "Target does not suppor branch tracing"
    error and be prepared to handle exceptions in all functions that call btrace
    target methods.  We therefore turn the target_* macros into static inline
    functions.
    
    Also remove the additional btrace target method checks that resulted in the
    above protocol error.
    
    Thanks to Maciej W. Rozycki <macro@mips.com> for reporting this.
    
    Signed-off-by: Markus Metzger  <markus.t.metzger@intel.com>
    
    gdbserver/
    	* target.h (target_enable_btrace, target_disable_btrace,
    	target_read_btrace, target_read_btrace_conf): Turn macro into inline
    	function.  Throw error if target method not defined.
    	* server.c (handle_qxfer_btrace, handle_qxfer_btrace_conf): Remove
    	check for btrace target method.  Be prepared to handle exceptions
    	from btrace target methods.


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Comments

Andreas Arnez Feb. 26, 2018, 7:30 p.m. UTC | #1
On Mon, Feb 26 2018, Metzger, Markus T wrote:

> Hello Maciej,
>
>> Process .../gdb/testsuite/outputs/gdb.base/advance/advance created; pid =
>> 25519 Listening on port 2346 target remote 1.2.3.4:2346 Remote debugging using
>> 1.2.3.4:2346 Reading symbols from .../lib/ld.so.1...done.
>> 0x77fc8de0 in __start () from .../lib/ld.so.1 Protocol error: qXfer:btrace-conf
>> (read-btrace-conf) conflicting enabled responses.
>> (gdb) continue
>> The program is not being run.
>> (gdb) FAIL: gdb.base/advance.exp: can't run to main
>> 
>> See the attached RSP packet exchange log for details.  Please investigate.

For the record, the same happens on s390x.  It seems that you broke all
targets without HAVE_LINUX_BTRACE.

> Below is a patch to address this.  I tested it on IA by undefining HAVE_LINUX_BTRACE.
> Does it fix the issue you reported?

I've tested your patch on s390x, and it seems to fix the problem.

--
Andreas
Maciej W. Rozycki Feb. 26, 2018, 9:48 p.m. UTC | #2
Hi Markus,

 Thank you for the quick action on this problem.

On Mon, 26 Feb 2018, Andreas Arnez wrote:

> >> Process .../gdb/testsuite/outputs/gdb.base/advance/advance created; pid =
> >> 25519 Listening on port 2346 target remote 1.2.3.4:2346 Remote debugging using
> >> 1.2.3.4:2346 Reading symbols from .../lib/ld.so.1...done.
> >> 0x77fc8de0 in __start () from .../lib/ld.so.1 Protocol error: qXfer:btrace-conf
> >> (read-btrace-conf) conflicting enabled responses.
> >> (gdb) continue
> >> The program is not being run.
> >> (gdb) FAIL: gdb.base/advance.exp: can't run to main
> >> 
> >> See the attached RSP packet exchange log for details.  Please investigate.
> 
> For the record, the same happens on s390x.  It seems that you broke all
> targets without HAVE_LINUX_BTRACE.

 I suspected that might be the case.  With your future changes that touch 
generic code would you please regression-test them with another target, or 
at least ask people to do so before committing them, so that such a wide 
breakage is avoided?  Having several versions not working at all makes it 
a pain to bisect changes that may have caused regressions meanwhile.

 Also did you verify that old-GDB/new-gdbserver and new-GDB/old-gdbserver 
combinations work correctly?

> > Below is a patch to address this.  I tested it on IA by undefining HAVE_LINUX_BTRACE.
> > Does it fix the issue you reported?
> 
> I've tested your patch on s390x, and it seems to fix the problem.

 I have smoke-tested your change with `gdb.base/advance.exp' and one of 
the configurations that failed previously, and it has scored all-pass now.  
I'll schedule full testing overnight.

  Maciej
Metzger, Markus T Feb. 27, 2018, 10:57 a.m. UTC | #3
Hello Maciej, Andreas,

Sorry about the breakage.  I hope everything's working again with that patch:

https://sourceware.org/ml/gdb-patches/2018-02/msg00420.html


It's the same I sent inline to the email with a recurring white-space error fixed.

Andreas already indicated that the patch fixes the problem on s390x.  Did the full
test show any further issues on your side, Maciej?


>  Also did you verify that old-GDB/new-gdbserver and new-GDB/old-gdbserver
> combinations work correctly?

That was discussed here:
https://sourceware.org/ml/gdb-patches/2018-02/msg00117.html

regards,
markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Maciej W. Rozycki Feb. 27, 2018, 2:56 p.m. UTC | #4
Hi Markus,

> https://sourceware.org/ml/gdb-patches/2018-02/msg00420.html
> 
> 
> It's the same I sent inline to the email with a recurring white-space 
> error fixed.

 With the change description you have also overrun our 74-column limit: 
<https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Column_limits>. 
NB due to how `git log' etc. indents descriptions I prefer to stay within 
72 columns with my own changes for a better visual effect, though you are 
of course free to use your own judgement here as long as you're within 74 
columns.

 Same with ChangeLog entries.  Also as per the the GNU Coding Standard:
<https://www.gnu.org/prep/standards/standards.html#Style-of-Change-Logs>
long function lists, etc. use `)' as the closing character, so:

	* target.h (target_enable_btrace, target_disable_btrace)
	(target_read_btrace, target_read_btrace_conf): Turn macro into 
	inline function.  Throw error if target method not defined.

 You need to mark new error messages for translation, so:

+    error (_("Target does not support branch tracing."));

etc.

 This:

+static inline int
+target_read_btrace (struct btrace_target_info *tinfo, struct buffer *buffer,
+		    enum btrace_read_type type)

also overruns the 74-column limit.

 The formatting appears otherwise OK to me and I'll leave the matter 
itself of this change up to someone with suitable expertise in this area.

> Andreas already indicated that the patch fixes the problem on s390x.  Did the full
> test show any further issues on your side, Maciej?

 We had a failure last night in the lab literally as I have scheduled the 
tests and consequently they didn't run.  I'll schedule them again as soon 
as possible, but the target system I have been using for testing is not 
back up yet, so it may not happen before tomorrow.  Sorry.

> >  Also did you verify that old-GDB/new-gdbserver and new-GDB/old-gdbserver
> > combinations work correctly?
> 
> That was discussed here:
> https://sourceware.org/ml/gdb-patches/2018-02/msg00117.html

 Thanks for confirming.  Is there going to be any difference here for 
non-x86 targets that needs to be verified?

  Maciej
Metzger, Markus T Feb. 27, 2018, 6:14 p.m. UTC | #5
Hello Maciej,

>  With the change description you have also overrun our 74-column limit:
> <https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-
> Standards#Column_limits>.
> NB due to how `git log' etc. indents descriptions I prefer to stay within
> 72 columns with my own changes for a better visual effect, though you are of
> course free to use your own judgement here as long as you're within 74 columns.

Actually, the hard limit is 80 columns.  I've been using that, so far, and from
looking at other files in gdb/ it looks like others were using the 80 columns limit,
as well.


>  Same with ChangeLog entries.  Also as per the the GNU Coding Standard:
> <https://www.gnu.org/prep/standards/standards.html#Style-of-Change-Logs>
> long function lists, etc. use `)' as the closing character, so:
> 
> 	* target.h (target_enable_btrace, target_disable_btrace)
> 	(target_read_btrace, target_read_btrace_conf): Turn macro into
> 	inline function.  Throw error if target method not defined.
> 
>  You need to mark new error messages for translation, so:
> 
> +    error (_("Target does not support branch tracing."));

Thanks for pointing those out.  I fixed them locally.

  
> > >  Also did you verify that old-GDB/new-gdbserver and
> > > new-GDB/old-gdbserver combinations work correctly?
> >
> > That was discussed here:
> > https://sourceware.org/ml/gdb-patches/2018-02/msg00117.html
> 
>  Thanks for confirming.  Is there going to be any difference here for
> non-x86 targets that needs to be verified?

They're supposed to work the same way.  I.e. older gdbservers won't
advertise the packets and newer GDB's hence won't use them.  And
newer gdbservers would advertise the packets and, with this patch,
fail on every request with "E.Target does not support branch tracing".

Let me check that once for both directions just to be sure.

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Maciej W. Rozycki Feb. 28, 2018, 9:55 a.m. UTC | #6
Hi Markus,

> >  With the change description you have also overrun our 74-column limit:
> > <https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-
> > Standards#Column_limits>.
> > NB due to how `git log' etc. indents descriptions I prefer to stay within
> > 72 columns with my own changes for a better visual effect, though you are of
> > course free to use your own judgement here as long as you're within 74 columns.
> 
> Actually, the hard limit is 80 columns.  I've been using that, so far, and from
> looking at other files in gdb/ it looks like others were using the 80 columns limit,
> as well.

 Well other people doing it wrong is not an excuse I am afraid.  With 
source files it is at least bearable, however as I wrote `git log' etc. 
indent descriptions by 4 characters, which means that as soon as you are 
beyond 76 columns lines get wrapped making text really hard to follow.

 Take your recent commit de65820cd69a as an example.  Its description 
renders like this on my screen:

    In gdb.btrace/buffer-size.exp we explicitly ask for the BTS recording format
.
    This may lead to spurious fails on systems where PT is being used by some ot
her
    process at the same time.

    Set both PT and BTS buffer sizes to 1 and check that whatever recording form
at
    is used will use a 4KB buffer.

-- is this how you wanted it to look like?

 If you disagree, then please discuss and get a consensus on an update to 
the rules set in our wiki.  They are there for a reason and please look 
for past discussions as to why they have been set like they are now.

> >  Thanks for confirming.  Is there going to be any difference here for
> > non-x86 targets that needs to be verified?
> 
> They're supposed to work the same way.  I.e. older gdbservers won't
> advertise the packets and newer GDB's hence won't use them.  And
> newer gdbservers would advertise the packets and, with this patch,
> fail on every request with "E.Target does not support branch tracing".
> 
> Let me check that once for both directions just to be sure.

 Great, thanks!

  Maciej
Maciej W. Rozycki Feb. 28, 2018, 10:29 a.m. UTC | #7
Hi Markus,

On Wed, 28 Feb 2018, Maciej W. Rozycki wrote:

>  Well other people doing it wrong is not an excuse I am afraid.  With 
> source files it is at least bearable, however as I wrote `git log' etc. 
> indent descriptions by 4 characters, which means that as soon as you are 
> beyond 76 columns lines get wrapped making text really hard to follow.

 A follow-up to myself just for avoidance of doubt: you are indeed allowed 
to exceed the soft limit in certain circumstances, but you still need to 
have "a reasonable reason", you can't just do this on a regular basis with 
no apparent reason whatsoever.

 I wouldn't care for example if you had a full stop in column 75 in one 
line in an otherwise well formatted description.  Otherwise you need to 
justify yourself.

  Maciej
Metzger, Markus T Feb. 28, 2018, 11:24 a.m. UTC | #8
Hello Maciej,

I added

	(setq fill-column 74)

for COMMIT_EDITMSG and for *.[hc] files.  This excludes .exp and .texinfo
(and others), which are still at 80 columns.

Also for command help-text strings I believe it makes sense to stay at 80
columns.

Are you OK with that?

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
diff mbox

Patch

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index cb02b58..4fd274f 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -1818,7 +1818,7 @@  handle_qxfer_btrace (const char *annex,
   enum btrace_read_type type;
   int result;
 
-  if (the_target->read_btrace == NULL || writebuf != NULL)
+  if (writebuf != NULL)
     return -2;
 
   if (ptid_equal (general_thread, null_ptid)
@@ -1857,12 +1857,21 @@  handle_qxfer_btrace (const char *annex,
     {
       buffer_free (&cache);
 
-      result = target_read_btrace (thread->btrace, &cache, type);
-      if (result != 0)
+      TRY
 	{
-	  memcpy (own_buf, cache.buffer, cache.used_size);
-	  return -3;
+	  result = target_read_btrace (thread->btrace, &cache, type);
+	  if (result != 0)
+	    memcpy (own_buf, cache.buffer, cache.used_size);
 	}
+      CATCH (exception, RETURN_MASK_ERROR)
+	{
+	  sprintf (own_buf, "E.%s", exception.message);
+	  result = -1;
+	}
+      END_CATCH
+
+      if (result != 0)
+	return -3;
     }
   else if (offset > cache.used_size)
     {
@@ -1889,7 +1898,7 @@  handle_qxfer_btrace_conf (const char *annex,
   struct thread_info *thread;
   int result;
 
-  if (the_target->read_btrace_conf == NULL || writebuf != NULL)
+  if (writebuf != NULL)
     return -2;
 
   if (annex[0] != '\0')
@@ -1919,12 +1928,21 @@  handle_qxfer_btrace_conf (const char *annex,
     {
       buffer_free (&cache);
 
-      result = target_read_btrace_conf (thread->btrace, &cache);
-      if (result != 0)
+      TRY
 	{
-	  memcpy (own_buf, cache.buffer, cache.used_size);
-	  return -3;
+	  result = target_read_btrace_conf (thread->btrace, &cache);
+	  if (result != 0)
+	    memcpy (own_buf, cache.buffer, cache.used_size);
 	}
+      CATCH (exception, RETURN_MASK_ERROR)
+	{
+	  sprintf (own_buf, "E.%s", exception.message);
+	  result = -1;
+	}
+      END_CATCH
+
+      if (result != 0)
+	return -3;
     }
   else if (offset > cache.used_size)
     {
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index dcefe1a..923a63b 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -620,17 +620,42 @@  int kill_inferior (int);
   (the_target->supports_agent ? \
    (*the_target->supports_agent) () : 0)
 
-#define target_enable_btrace(ptid, conf) \
-  (*the_target->enable_btrace) (ptid, conf)
+static inline struct btrace_target_info *
+target_enable_btrace(ptid_t ptid, const struct btrace_config *conf)
+{
+  if (the_target->enable_btrace == nullptr)
+    error ("Target does not support branch tracing.");
+
+  return (*the_target->enable_btrace) (ptid, conf);
+}
+
+static inline int
+target_disable_btrace(struct btrace_target_info *tinfo)
+{
+  if (the_target->disable_btrace == nullptr)
+    error ("Target does not support branch tracing.");
 
-#define target_disable_btrace(tinfo) \
-  (*the_target->disable_btrace) (tinfo)
+  return (*the_target->disable_btrace) (tinfo);
+}
 
-#define target_read_btrace(tinfo, buffer, type)	\
-  (*the_target->read_btrace) (tinfo, buffer, type)
+static inline int
+target_read_btrace(struct btrace_target_info *tinfo, struct buffer *buffer,
+		   enum btrace_read_type type)
+{
+  if (the_target->read_btrace == nullptr)
+    error ("Target does not support branch tracing.");
+
+  return (*the_target->read_btrace) (tinfo, buffer, type);
+}
+
+static inline int
+target_read_btrace_conf(struct btrace_target_info *tinfo, struct buffer *buffer)
+{
+  if (the_target->read_btrace_conf == nullptr)
+    error ("Target does not support branch tracing.");
 
-#define target_read_btrace_conf(tinfo, buffer)	\
-  (*the_target->read_btrace_conf) (tinfo, buffer)
+  return (*the_target->read_btrace_conf) (tinfo, buffer);
+}
 
 #define target_supports_range_stepping() \
   (the_target->supports_range_stepping ? \