[PATCH 1/2] Fix write endianness/size problem for fast tracepoint enabled flag

[PATCH 1/2] Fix write endianness/size problem for fast tracepoint enabled flag

[Simon Marchi]

From: Par Olsson <par.olsson@windriver.com>

I am sending this fix on behalf of Par Olsson, as a follow-up of this
one:

https://www.sourceware.org/ml/gdb-patches/2015-10/msg00196.html

This problem is exposed when enabling/disabling fast tracepoints on big
endian machines.  The flag is defined as an int8_t, but is written from
gdbserver as an integer (usually 32 bits).  When the agent code reads it
as an int8_t, it only considers the most significant byte, which is
always 0.

Also, we were writing 32 bits in an 8 bits field, so the write would
overflow, but since the following bytes are padding (the next field is
an uint64_t), it luckily didn't cause any issue on little endian
systems.

The fix was originally tested on ARM big endian systems, but I don't
have access to such a system.  However, thanks to Marcin's PowerPC fast
tracepoint patches and gcc110 (big endian Power7) on the gcc compile
farm, I was able to reproduce the problem, test the fix and write a
test (the following patch).

gdb/gdbserver/ChangeLog:

YYYY-MM-DD  Par Olsson  <par.olsson@windriver.com>

	* tracepoint.c (write_inferior_int8): New function.
	(cmd_qtenable_disable): Write enable flag using
	write_inferior_int8.
---
 gdb/gdbserver/tracepoint.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 7c20612..3e0d57a 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -449,6 +449,12 @@ write_inferior_integer (CORE_ADDR symaddr, int val)
 }
 
 static int
+write_inferior_int8 (CORE_ADDR symaddr, int8_t val)
+{
+  return write_inferior_memory (symaddr, (unsigned char *) &val, sizeof (val));
+}
+
+static int
 write_inferior_uinteger (CORE_ADDR symaddr, unsigned int val)
 {
   return write_inferior_memory (symaddr, (unsigned char *) &val, sizeof (val));
@@ -2784,7 +2790,7 @@ cmd_qtenable_disable (char *own_buf, int enable)
 	      return;
 	    }
 
-	  ret = write_inferior_integer (obj_addr, enable);
+	  ret = write_inferior_int8 (obj_addr, enable);
 	  done_accessing_memory ();
 	  
 	  if (ret)
-- 
2.8.0

[PATCH 2/2] Add test for fast tracepoint enable/disable

[Simon Marchi]

This patch adds a test for fast tracepoints enabling/disabling, which
didn't work properly on big endian systems.

gdb/testsuite/ChangeLog:

	* gdb.trace/ftrace-enable-disable.exp: New file.
	* gdb.trace/ftrace-enable-disable.c: New file.
---
 gdb/testsuite/gdb.trace/ftrace-enable-disable.c   |  38 +++++++
 gdb/testsuite/gdb.trace/ftrace-enable-disable.exp | 126 ++++++++++++++++++++++
 2 files changed, 164 insertions(+)
 create mode 100644 gdb/testsuite/gdb.trace/ftrace-enable-disable.c
 create mode 100644 gdb/testsuite/gdb.trace/ftrace-enable-disable.exp

diff --git a/gdb/testsuite/gdb.trace/ftrace-enable-disable.c b/gdb/testsuite/gdb.trace/ftrace-enable-disable.c
new file mode 100644
index 0000000..23453db
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/ftrace-enable-disable.c
@@ -0,0 +1,38 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2016 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "trace-common.h"
+
+static void
+break_here (void)
+{
+}
+
+int
+main (void)
+{
+  int i;
+
+  for (i = 0; i < 100; i++)
+    {
+      FAST_TRACEPOINT_LABEL(point_a);
+      FAST_TRACEPOINT_LABEL(point_b);
+      break_here ();
+    }
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.trace/ftrace-enable-disable.exp b/gdb/testsuite/gdb.trace/ftrace-enable-disable.exp
new file mode 100644
index 0000000..883b5b9
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/ftrace-enable-disable.exp
@@ -0,0 +1,126 @@
+# Copyright 2015-2016 Free Software Foundation, Inc.
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib "trace-support.exp"
+
+standard_testfile
+set executable $testfile
+set expfile $testfile.exp
+
+# Some targets have leading underscores on assembly symbols.
+set options [list debug [gdb_target_symbol_prefix_flags]]
+
+# Check that the target supports trace.
+if { [gdb_compile_pthreads "$srcdir/$subdir/$srcfile" $binfile executable $options] != "" } {
+    untested "Couldn't compile test program"
+    return -1
+}
+
+clean_restart ${testfile}
+
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "target does not support trace"
+    return -1
+}
+
+# Compile the test case with the in-process agent library.
+set libipa [get_in_proc_agent]
+gdb_load_shlibs $libipa
+
+lappend options shlib=$libipa
+
+if { [gdb_compile_pthreads "$srcdir/$subdir/$srcfile" $binfile executable $options] != "" } {
+    untested "Couldn't compile test program with in-process agent library"
+    return -1
+}
+
+# This test makes sure that disabling and enabling fast tracepoints works
+# correctly.
+
+proc test_ftrace_enable_disable { } {
+    with_test_prefix "test_ftrace_enable_disable" {
+	global executable binfile decimal
+	clean_restart ${executable}
+
+	set expected 0
+
+	if ![runto_main] {
+	    fail "Can't run to main."
+	    return -1
+	}
+
+	gdb_test "ftrace point_a" "Fast tracepoint 2.*"
+	gdb_test "ftrace point_b" "Fast tracepoint 3.*"
+	gdb_breakpoint "break_here"
+	gdb_test_no_output "tstart"
+
+	# Test that tracepoints start enabled
+	with_test_prefix "start" {
+	    gdb_continue "break_here"
+	    incr expected 2
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	# Test that disabling works
+	with_test_prefix "disable #1" {
+	    gdb_test_no_output "disable 2"
+	    gdb_continue "break_here"
+	    incr expected 1
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	with_test_prefix "disable #2" {
+	    gdb_test_no_output "disable 3"
+	    gdb_continue "break_here"
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	# Test that disabling an already disabled tracepoint works
+	with_test_prefix "disable #3" {
+	    gdb_test_no_output "disable 2"
+	    gdb_continue "break_here"
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	# Test that enabling works
+	with_test_prefix "enable #1" {
+	    gdb_test_no_output "enable 2"
+	    gdb_continue "break_here"
+	    incr expected 1
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	with_test_prefix "enable #2" {
+	    gdb_test_no_output "enable 3"
+	    gdb_continue "break_here"
+	    incr expected 2
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	# Test that enabling an already enabled tracepoint works
+	with_test_prefix "enable #3" {
+	    gdb_test_no_output "enable 3"
+	    gdb_continue "break_here"
+	    incr expected 2
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+    }
+}
+
+test_ftrace_enable_disable
-- 
2.8.0

Re: [PATCH 2/2] Add test for fast tracepoint enable/disable

[Yao Qi]

Simon Marchi <simon.marchi@ericsson.com> writes:

> +# Check that the target supports trace.
> +if { [gdb_compile_pthreads "$srcdir/$subdir/$srcfile" $binfile executable $options] != "" } {
> +    untested "Couldn't compile test program"
> +    return -1
> +}

Why do you compile program with pthread?

> +
> +clean_restart ${testfile}
> +
> +if ![runto_main] {
> +    fail "Can't run to main to check for trace support"
> +    return -1
> +}
> +
> +if ![gdb_target_supports_trace] {
> +    unsupported "target does not support trace"
> +    return -1
> +}
> +
> +# Compile the test case with the in-process agent library.
> +set libipa [get_in_proc_agent]
> +gdb_load_shlibs $libipa
> +
> +lappend options shlib=$libipa
> +
> +if { [gdb_compile_pthreads "$srcdir/$subdir/$srcfile" $binfile executable $options] != "" } {
> +    untested "Couldn't compile test program with in-process agent library"
> +    return -1
> +}

Likewise.

> +
> +# This test makes sure that disabling and enabling fast tracepoints works
> +# correctly.
> +
> +proc test_ftrace_enable_disable { } {

Can you extend the test to cover normal tracepoint as well?  It
shouldn't be hard.  You can just pass argument "type" to "proc
test_trace_enable_disable" like,

proc test_trace_enable_disable { type } {

and the type can be "trace" and "ftrace".

-- 
Yao (齐尧)

Re: [PATCH 2/2] Add test for fast tracepoint enable/disable

[Simon Marchi]

On 16-03-31 06:58 AM, Yao Qi wrote:
> Simon Marchi <simon.marchi@ericsson.com> writes:
> 
>> +# Check that the target supports trace.
>> +if { [gdb_compile_pthreads "$srcdir/$subdir/$srcfile" $binfile executable $options] != "" } {
>> +    untested "Couldn't compile test program"
>> +    return -1
>> +}
> 
> Why do you compile program with pthread?

Good question.  Because the setup for testing fast tracepoints is tedious, I always
use ftrace-lock.exp as a template, because it is relatively short.  ftrace-lock.exp
uses gdb_compile_pthreads.  I think it makes sense, because the IPA starts a helper
thread with pthread_create.

Note that it still builds fine with gdb_compile instead of gdb_compile_pthreads,
probably because libinproctrace.so has a DT_NEEDED for libpthreads.so, so it gets
pulled in.  But using gdb_compile_pthreads should add -pthread to the CFLAGS, which
may impact compilation (e.g. choose re-entrant versions of functions).

However, I just noticed that the command line includes -lpthreads instead of -pthreads:

spawn gcc -Wl,--no-as-needed /home/emaisin/build/binutils-gdb/gdb/testsuite/../../../../src/binutils-gdb/gdb/testsuite/gdb.trace/ftrace-enable-disable.c \
  /home/emaisin/build/binutils-gdb/gdb/testsuite/../gdbserver/libinproctrace.so -g \
  -lpthread -lm -o \
  /home/emaisin/build/binutils-gdb/gdb/testsuite/outputs/gdb.trace/ftrace-enable-disable/ftrace-enable-disable

I think that to be more "correct", we should try -pthread before -lpthread in
gdb_compile_pthreads...

>> +
>> +clean_restart ${testfile}
>> +
>> +if ![runto_main] {
>> +    fail "Can't run to main to check for trace support"
>> +    return -1
>> +}
>> +
>> +if ![gdb_target_supports_trace] {
>> +    unsupported "target does not support trace"
>> +    return -1
>> +}
>> +
>> +# Compile the test case with the in-process agent library.
>> +set libipa [get_in_proc_agent]
>> +gdb_load_shlibs $libipa
>> +
>> +lappend options shlib=$libipa
>> +
>> +if { [gdb_compile_pthreads "$srcdir/$subdir/$srcfile" $binfile executable $options] != "" } {
>> +    untested "Couldn't compile test program with in-process agent library"
>> +    return -1
>> +}
> 
> Likewise.

Same response :)

>> +
>> +# This test makes sure that disabling and enabling fast tracepoints works
>> +# correctly.
>> +
>> +proc test_ftrace_enable_disable { } {
> 
> Can you extend the test to cover normal tracepoint as well?  It
> shouldn't be hard.  You can just pass argument "type" to "proc
> test_trace_enable_disable" like,
> 
> proc test_trace_enable_disable { type } {
> 
> and the type can be "trace" and "ftrace".

I'll try to do that.

Thanks,

Simon

Re: [PATCH 2/2] Add test for fast tracepoint enable/disable

[Simon Marchi]

On 16-03-31 02:01 PM, Simon Marchi wrote:
>> Can you extend the test to cover normal tracepoint as well?  It
>> shouldn't be hard.  You can just pass argument "type" to "proc
>> test_trace_enable_disable" like,
>>
>> proc test_trace_enable_disable { type } {
>>
>> and the type can be "trace" and "ftrace".
> 
> I'll try to do that.
> 
> Thanks,
> 
> Simon

Here is an updated patch that tests both trace and ftrace.  I left gdb_compile_pthreads
for now, while we discuss.

One thing that could be improved is that the test should be organized so that it tests
normal tracepoints even if fast tracepoints are not supported by the architecture.
Right now, I think it will do both or none at all.


From 09ccbccb0176feaaf8a385adad28dab6ff357f33 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Wed, 30 Mar 2016 10:24:05 -0400
Subject: [PATCH] Add test for tracepoint enable/disable

This patch adds a test for tracepoints enabling/disabling, which
didn't work properly for fast tracepoints on big endian systems.

gdb/testsuite/ChangeLog:

	* gdb.trace/trace-enable-disable.exp: New file.
	* gdb.trace/trace-enable-disable.c: New file.
---
 gdb/testsuite/gdb.trace/trace-enable-disable.c   |  38 +++++++
 gdb/testsuite/gdb.trace/trace-enable-disable.exp | 128 +++++++++++++++++++++++
 2 files changed, 166 insertions(+)
 create mode 100644 gdb/testsuite/gdb.trace/trace-enable-disable.c
 create mode 100644 gdb/testsuite/gdb.trace/trace-enable-disable.exp

diff --git a/gdb/testsuite/gdb.trace/trace-enable-disable.c b/gdb/testsuite/gdb.trace/trace-enable-disable.c
new file mode 100644
index 0000000..23453db
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/trace-enable-disable.c
@@ -0,0 +1,38 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2016 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "trace-common.h"
+
+static void
+break_here (void)
+{
+}
+
+int
+main (void)
+{
+  int i;
+
+  for (i = 0; i < 100; i++)
+    {
+      FAST_TRACEPOINT_LABEL(point_a);
+      FAST_TRACEPOINT_LABEL(point_b);
+      break_here ();
+    }
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.trace/trace-enable-disable.exp b/gdb/testsuite/gdb.trace/trace-enable-disable.exp
new file mode 100644
index 0000000..2d9d6c7
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/trace-enable-disable.exp
@@ -0,0 +1,128 @@
+# Copyright 2015-2016 Free Software Foundation, Inc.
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib "trace-support.exp"
+
+standard_testfile
+set executable $testfile
+set expfile $testfile.exp
+
+# Some targets have leading underscores on assembly symbols.
+set options [list debug [gdb_target_symbol_prefix_flags]]
+
+# Check that the target supports trace.
+if { [gdb_compile_pthreads "$srcdir/$subdir/$srcfile" $binfile executable $options] != "" } {
+    untested "Couldn't compile test program"
+    return -1
+}
+
+clean_restart ${testfile}
+
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "target does not support trace"
+    return -1
+}
+
+# Compile the test case with the in-process agent library.
+set libipa [get_in_proc_agent]
+gdb_load_shlibs $libipa
+
+lappend options shlib=$libipa
+
+if { [gdb_compile_pthreads "$srcdir/$subdir/$srcfile" $binfile executable $options] != "" } {
+    untested "Couldn't compile test program with in-process agent library"
+    return -1
+}
+
+# This test makes sure that disabling and enabling tracepoints works
+# correctly.  TRACEPOINT_CMD is the command used to set tracepoints
+# (e.g. trace or ftrace).
+
+proc test_tracepoint_enable_disable { tracepoint_cmd } {
+    with_test_prefix "test_tracepoint_enable_disable" {
+	global executable binfile decimal
+	clean_restart ${executable}
+
+	set expected 0
+
+	if ![runto_main] {
+	    fail "Can't run to main."
+	    return -1
+	}
+
+	gdb_test "$tracepoint_cmd point_a" "(Tracepoint|Fast tracepoint) 2.*"
+	gdb_test "$tracepoint_cmd point_b" "(Tracepoint|Fast tracepoint) 3.*"
+	gdb_breakpoint "break_here"
+	gdb_test_no_output "tstart"
+
+	# Test that tracepoints start enabled
+	with_test_prefix "start" {
+	    gdb_continue "break_here"
+	    incr expected 2
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	# Test that disabling works
+	with_test_prefix "disable #1" {
+	    gdb_test_no_output "disable 2"
+	    gdb_continue "break_here"
+	    incr expected 1
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	with_test_prefix "disable #2" {
+	    gdb_test_no_output "disable 3"
+	    gdb_continue "break_here"
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	# Test that disabling an already disabled tracepoint works
+	with_test_prefix "disable #3" {
+	    gdb_test_no_output "disable 2"
+	    gdb_continue "break_here"
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	# Test that enabling works
+	with_test_prefix "enable #1" {
+	    gdb_test_no_output "enable 2"
+	    gdb_continue "break_here"
+	    incr expected 1
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	with_test_prefix "enable #2" {
+	    gdb_test_no_output "enable 3"
+	    gdb_continue "break_here"
+	    incr expected 2
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	# Test that enabling an already enabled tracepoint works
+	with_test_prefix "enable #3" {
+	    gdb_test_no_output "enable 3"
+	    gdb_continue "break_here"
+	    incr expected 2
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+    }
+}
+
+test_tracepoint_enable_disable trace
+test_tracepoint_enable_disable ftrace
-- 
2.8.0

Re: [PATCH 2/2] Add test for fast tracepoint enable/disable

[Yao Qi]

Simon Marchi <simon.marchi@ericsson.com> writes:

> Good question.  Because the setup for testing fast tracepoints is
> tedious, I always
> use ftrace-lock.exp as a template, because it is relatively short.
> ftrace-lock.exp
> uses gdb_compile_pthreads.  I think it makes sense, because the IPA
> starts a helper
> thread with pthread_create.

gdb_compile_pthreads is used in ftrace-lock.exp because ftrace-lock.c is
a multi-threaded program, but ftrace-enable-disable.c isn't.  The
program to be tested is running in one thread, and it doesn't have any
interaction with the helper thread.

>
> Note that it still builds fine with gdb_compile instead of gdb_compile_pthreads,
> probably because libinproctrace.so has a DT_NEEDED for libpthreads.so,
> so it gets
> pulled in.  But using gdb_compile_pthreads should add -pthread to the
> CFLAGS, which
> may impact compilation (e.g. choose re-entrant versions of functions).

Using IPA/fast tracepoint shouldn't affect how the program is compiled.
User is writing and debugging a single thread program, and he/she wants
to use fast tracepoint to debug.  He/She shouldn't recompile the program
with -lpthreads or -pthreads, what needed here is to let IPA be loaded with
the program.

>
> However, I just noticed that the command line includes -lpthreads
> instead of -pthreads:
>
> spawn gcc -Wl,--no-as-needed
> /home/emaisin/build/binutils-gdb/gdb/testsuite/../../../../src/binutils-gdb/gdb/testsuite/gdb.trace/ftrace-enable-disable.c
> \
>   /home/emaisin/build/binutils-gdb/gdb/testsuite/../gdbserver/libinproctrace.so
> -g \
>   -lpthread -lm -o \
>   /home/emaisin/build/binutils-gdb/gdb/testsuite/outputs/gdb.trace/ftrace-enable-disable/ftrace-enable-disable
>
> I think that to be more "correct", we should try -pthread before -lpthread in
> gdb_compile_pthreads...

I am not sure, it is a separate issue, this applies to all test cases
compiled with gdb_compile_pthreads.

-- 
Yao (齐尧)

Re: [PATCH 2/2] Add test for fast tracepoint enable/disable

[Simon Marchi]

On 16-04-01 09:36 AM, Yao Qi wrote:
> Simon Marchi <simon.marchi@ericsson.com> writes:
> 
>> Good question.  Because the setup for testing fast tracepoints is
>> tedious, I always
>> use ftrace-lock.exp as a template, because it is relatively short.
>> ftrace-lock.exp
>> uses gdb_compile_pthreads.  I think it makes sense, because the IPA
>> starts a helper
>> thread with pthread_create.
> 
> gdb_compile_pthreads is used in ftrace-lock.exp because ftrace-lock.c is
> a multi-threaded program, but ftrace-enable-disable.c isn't.  The
> program to be tested is running in one thread, and it doesn't have any
> interaction with the helper thread.
> 
>>
>> Note that it still builds fine with gdb_compile instead of gdb_compile_pthreads,
>> probably because libinproctrace.so has a DT_NEEDED for libpthreads.so,
>> so it gets
>> pulled in.  But using gdb_compile_pthreads should add -pthread to the
>> CFLAGS, which
>> may impact compilation (e.g. choose re-entrant versions of functions).
> 
> Using IPA/fast tracepoint shouldn't affect how the program is compiled.
> User is writing and debugging a single thread program, and he/she wants
> to use fast tracepoint to debug.  He/She shouldn't recompile the program
> with -lpthreads or -pthreads, what needed here is to let IPA be loaded with
> the program.

That totally makes sense, I'll change it to gdb_compile.

>>
>> However, I just noticed that the command line includes -lpthreads
>> instead of -pthreads:
>>
>> spawn gcc -Wl,--no-as-needed
>> /home/emaisin/build/binutils-gdb/gdb/testsuite/../../../../src/binutils-gdb/gdb/testsuite/gdb.trace/ftrace-enable-disable.c
>> \
>>   /home/emaisin/build/binutils-gdb/gdb/testsuite/../gdbserver/libinproctrace.so
>> -g \
>>   -lpthread -lm -o \
>>   /home/emaisin/build/binutils-gdb/gdb/testsuite/outputs/gdb.trace/ftrace-enable-disable/ftrace-enable-disable
>>
>> I think that to be more "correct", we should try -pthread before -lpthread in
>> gdb_compile_pthreads...
> 
> I am not sure, it is a separate issue, this applies to all test cases
> compiled with gdb_compile_pthreads.
> 

Re: [PATCH 2/2] Add test for fast tracepoint enable/disable

[Simon Marchi]

On 16-03-31 06:58 AM, Yao Qi wrote:
> Simon Marchi <simon.marchi@ericsson.com> writes:
> 
>> +# Check that the target supports trace.
>> +if { [gdb_compile_pthreads "$srcdir/$subdir/$srcfile" $binfile executable $options] != "" } {
>> +    untested "Couldn't compile test program"
>> +    return -1
>> +}
> 
> Why do you compile program with pthread?
> 
>> +
>> +clean_restart ${testfile}
>> +
>> +if ![runto_main] {
>> +    fail "Can't run to main to check for trace support"
>> +    return -1
>> +}
>> +
>> +if ![gdb_target_supports_trace] {
>> +    unsupported "target does not support trace"
>> +    return -1
>> +}
>> +
>> +# Compile the test case with the in-process agent library.
>> +set libipa [get_in_proc_agent]
>> +gdb_load_shlibs $libipa
>> +
>> +lappend options shlib=$libipa
>> +
>> +if { [gdb_compile_pthreads "$srcdir/$subdir/$srcfile" $binfile executable $options] != "" } {
>> +    untested "Couldn't compile test program with in-process agent library"
>> +    return -1
>> +}
> 
> Likewise.
> 
>> +
>> +# This test makes sure that disabling and enabling fast tracepoints works
>> +# correctly.
>> +
>> +proc test_ftrace_enable_disable { } {
> 
> Can you extend the test to cover normal tracepoint as well?  It
> shouldn't be hard.  You can just pass argument "type" to "proc
> test_trace_enable_disable" like,
> 
> proc test_trace_enable_disable { type } {
> 
> and the type can be "trace" and "ftrace".
> 


Hi Yao,

Here's the v2.  Changes:

  * gdb_compile instead of gdb_compile_pthreads.
  * Test with both trace and ftrace commands.



From 991319e45356adcdb85c5db1e377e72fcd978f07 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Wed, 30 Mar 2016 10:24:05 -0400
Subject: [PATCH] Add test for tracepoint enable/disable

This patch adds a test for tracepoints enabling/disabling, which
didn't work properly for fast tracepoints on big endian systems.

gdb/testsuite/ChangeLog:

	* gdb.trace/trace-enable-disable.exp: New file.
	* gdb.trace/trace-enable-disable.c: New file.
---
 gdb/testsuite/gdb.trace/trace-enable-disable.c   |  38 +++++++
 gdb/testsuite/gdb.trace/trace-enable-disable.exp | 128 +++++++++++++++++++++++
 2 files changed, 166 insertions(+)
 create mode 100644 gdb/testsuite/gdb.trace/trace-enable-disable.c
 create mode 100644 gdb/testsuite/gdb.trace/trace-enable-disable.exp

diff --git a/gdb/testsuite/gdb.trace/trace-enable-disable.c b/gdb/testsuite/gdb.trace/trace-enable-disable.c
new file mode 100644
index 0000000..23453db
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/trace-enable-disable.c
@@ -0,0 +1,38 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2016 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "trace-common.h"
+
+static void
+break_here (void)
+{
+}
+
+int
+main (void)
+{
+  int i;
+
+  for (i = 0; i < 100; i++)
+    {
+      FAST_TRACEPOINT_LABEL(point_a);
+      FAST_TRACEPOINT_LABEL(point_b);
+      break_here ();
+    }
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.trace/trace-enable-disable.exp b/gdb/testsuite/gdb.trace/trace-enable-disable.exp
new file mode 100644
index 0000000..0c35c92
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/trace-enable-disable.exp
@@ -0,0 +1,128 @@
+# Copyright 2015-2016 Free Software Foundation, Inc.
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib "trace-support.exp"
+
+standard_testfile
+set executable $testfile
+set expfile $testfile.exp
+
+# Some targets have leading underscores on assembly symbols.
+set options [list debug [gdb_target_symbol_prefix_flags]]
+
+# Check that the target supports trace.
+if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile executable $options] != "" } {
+    untested "Couldn't compile test program"
+    return -1
+}
+
+clean_restart ${testfile}
+
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "target does not support trace"
+    return -1
+}
+
+# Compile the test case with the in-process agent library.
+set libipa [get_in_proc_agent]
+gdb_load_shlib $libipa
+
+lappend options shlib=$libipa
+
+if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile executable $options] != "" } {
+    untested "Couldn't compile test program with in-process agent library"
+    return -1
+}
+
+# This test makes sure that disabling and enabling tracepoints works
+# correctly.  TRACEPOINT_CMD is the command used to set tracepoints
+# (e.g. trace or ftrace).
+
+proc test_tracepoint_enable_disable { tracepoint_cmd } {
+    with_test_prefix "test_tracepoint_enable_disable $tracepoint_cmd" {
+	global executable
+	clean_restart ${executable}
+
+	set expected 0
+
+	if ![runto_main] {
+	    fail "Can't run to main."
+	    return -1
+	}
+
+	gdb_test "$tracepoint_cmd point_a" "(Tracepoint|Fast tracepoint) 2.*"
+	gdb_test "$tracepoint_cmd point_b" "(Tracepoint|Fast tracepoint) 3.*"
+	gdb_breakpoint "break_here"
+	gdb_test_no_output "tstart"
+
+	# Test that tracepoints start enabled
+	with_test_prefix "start" {
+	    gdb_continue "break_here"
+	    incr expected 2
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	# Test that disabling works
+	with_test_prefix "disable #1" {
+	    gdb_test_no_output "disable 2"
+	    gdb_continue "break_here"
+	    incr expected 1
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	with_test_prefix "disable #2" {
+	    gdb_test_no_output "disable 3"
+	    gdb_continue "break_here"
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	# Test that disabling an already disabled tracepoint works
+	with_test_prefix "disable #3" {
+	    gdb_test_no_output "disable 2"
+	    gdb_continue "break_here"
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	# Test that enabling works
+	with_test_prefix "enable #1" {
+	    gdb_test_no_output "enable 2"
+	    gdb_continue "break_here"
+	    incr expected 1
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	with_test_prefix "enable #2" {
+	    gdb_test_no_output "enable 3"
+	    gdb_continue "break_here"
+	    incr expected 2
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+
+	# Test that enabling an already enabled tracepoint works
+	with_test_prefix "enable #3" {
+	    gdb_test_no_output "enable 3"
+	    gdb_continue "break_here"
+	    incr expected 2
+	    gdb_test "tstatus" "Collected $expected trace frames.*"
+	}
+    }
+}
+
+test_tracepoint_enable_disable trace
+test_tracepoint_enable_disable ftrace
-- 
2.8.1

Re: [PATCH 2/2] Add test for fast tracepoint enable/disable

[Yao Qi]

Simon Marchi <simon.marchi@ericsson.com> writes:

> This patch adds a test for tracepoints enabling/disabling, which
> didn't work properly for fast tracepoints on big endian systems.
>
> gdb/testsuite/ChangeLog:
>
> 	* gdb.trace/trace-enable-disable.exp: New file.
> 	* gdb.trace/trace-enable-disable.c: New file.

This is good to me.

-- 
Yao (齐尧)

Re: [PATCH 2/2] Add test for fast tracepoint enable/disable

[Simon Marchi]

On 16-04-28 12:03 PM, Yao Qi wrote:
> Simon Marchi <simon.marchi@ericsson.com> writes:
> 
>> This patch adds a test for tracepoints enabling/disabling, which
>> didn't work properly for fast tracepoints on big endian systems.
>>
>> gdb/testsuite/ChangeLog:
>>
>> 	* gdb.trace/trace-enable-disable.exp: New file.
>> 	* gdb.trace/trace-enable-disable.c: New file.
> 
> This is good to me.
> 

Thanks, both patches are pushed.

Re: [PATCH 1/2] Fix write endianness/size problem for fast tracepoint enabled flag

[Yao Qi]

Simon Marchi <simon.marchi@ericsson.com> writes:

> I am sending this fix on behalf of Par Olsson, as a follow-up of this
> one:
>
> https://www.sourceware.org/ml/gdb-patches/2015-10/msg00196.html

Hi Simon,
The patch in the url above is to change 'enabled' field to uint32_t, so
I guess you got the patch below from Par Olsson off list?

The reason I ask this is that I find the author doesn't have the copyright
assignment.  We can take it as a tiny patch, if no one has objections.

>  
>  static int
> +write_inferior_int8 (CORE_ADDR symaddr, int8_t val)
> +{
> +  return write_inferior_memory (symaddr, (unsigned char *) &val, sizeof (val));

This line is too long.

> +}

The patch itself is good to me.

-- 
Yao (齐尧)

Re: [PATCH 1/2] Fix write endianness/size problem for fast tracepoint enabled flag

[Pedro Alves]

On 03/31/2016 11:51 AM, Yao Qi wrote:
> Simon Marchi <simon.marchi@ericsson.com> writes:
> 
>> I am sending this fix on behalf of Par Olsson, as a follow-up of this
>> one:
>>
>> https://www.sourceware.org/ml/gdb-patches/2015-10/msg00196.html
> 
> Hi Simon,
> The patch in the url above is to change 'enabled' field to uint32_t, so
> I guess you got the patch below from Par Olsson off list?
> 
> The reason I ask this is that I find the author doesn't have the copyright
> assignment.  We can take it as a tiny patch, if no one has objections.

Actually Wind River has a blanket copyright assignment:

BINUTILS DEJAGNU GCC GDB        Wind River Systems      1997-09-04
Assigns past and future changes submitted to the FSF
(gcc, g++, gdb, binutils, libg++, libstdc++, dejagnu,
GNU C++ testsuite, GNU C testsuite).

Thanks,
Pedro Alves

Re: [PATCH 1/2] Fix write endianness/size problem for fast tracepoint enabled flag

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Actually Wind River has a blanket copyright assignment:
>
> BINUTILS DEJAGNU GCC GDB        Wind River Systems      1997-09-04
> Assigns past and future changes submitted to the FSF
> (gcc, g++, gdb, binutils, libg++, libstdc++, dejagnu,
> GNU C++ testsuite, GNU C testsuite).

Oh, right, I searched "windriver" (no space), and only find one record for
one person.

-- 
Yao (齐尧)

Re: [PATCH 1/2] Fix write endianness/size problem for fast tracepoint enabled flag

[Simon Marchi]

On 16-03-31 06:51 AM, Yao Qi wrote:
> Simon Marchi <simon.marchi@ericsson.com> writes:
> 
>> I am sending this fix on behalf of Par Olsson, as a follow-up of this
>> one:
>>
>> https://www.sourceware.org/ml/gdb-patches/2015-10/msg00196.html
> 
> Hi Simon,
> The patch in the url above is to change 'enabled' field to uint32_t, so
> I guess you got the patch below from Par Olsson off list?
> 
> The reason I ask this is that I find the author doesn't have the copyright
> assignment.  We can take it as a tiny patch, if no one has objections.

Par indeed sent us this off-list.  But as Pedro mentioned, Wind River is covered.

>  
>>  static int
>> +write_inferior_int8 (CORE_ADDR symaddr, int8_t val)
>> +{
>> +  return write_inferior_memory (symaddr, (unsigned char *) &val, sizeof (val));
> 
> This line is too long.

It's 79 characters long, no?  It's the same line as the line in write_inferior_integer,
I think they are both ok.

> The patch itself is good to me.

Ok thanks, I'll push it when the corresponding test is accepted.

Re: [PATCH 1/2] Fix write endianness/size problem for fast tracepoint enabled flag

[Yao Qi]

Simon Marchi <simon.marchi@ericsson.com> writes:

Hi Simon,

> gdb/gdbserver/ChangeLog:
>
> YYYY-MM-DD  Par Olsson  <par.olsson@windriver.com>
>
> 	* tracepoint.c (write_inferior_int8): New function.
> 	(cmd_qtenable_disable): Write enable flag using
> 	write_inferior_int8.

The format of committed changelog entry looks odd to me,

2016-04-28  Par Olsson  <par.olsson@windriver.com>
2016-04-28  Simon Marchi  <simon.marchi@ericsson.com>

	* tracepoint.c (write_inferior_int8): New function.
	(cmd_qtenable_disable): Write enable flag using
	write_inferior_int8.

-- 
Yao (齐尧)

RE: [PATCH 1/2] Fix write endianness/size problem for fast tracepoint enabled flag

[Simon Marchi]

>> gdb/gdbserver/ChangeLog:
>>
>> YYYY-MM-DD  Par Olsson  <par.olsson@windriver.com>
>>
>>       * tracepoint.c (write_inferior_int8): New function.
>>       (cmd_qtenable_disable): Write enable flag using
>>       write_inferior_int8.
>
> The format of committed changelog entry looks odd to me,

Fixed, thanks.