[PATCH] Initialize target description early in IPA

[PATCH] Initialize target description early in IPA

[Yao Qi]

Target descriptions are allocated lazily, that is fine in GDBserver,
but it is not safe to call malloc in gdb_collect in IPA, because we
can set a fast tracepoint in malloc, and when the tracepoint is hit,
gdb_collect/malloc is called, deadlock or memory corruption may be
triggered.

 #0  0xf7cfc200 in malloc ()
 #1  0xf7efdc07 in operator new(unsigned int) ()
 #2  0xf7ef7636 in allocate_target_description() ()
 #3  0xf7efcbe1 in i386_create_target_description(unsigned long long, bool) ()
 #4  0xf7efb474 in i386_linux_read_description(unsigned long long) ()
 #5  0xf7efb190 in get_ipa_tdesc(int) ()
 #6  0xf7ef9baa in gdb_collect ()

The fix is to initialize all target descriptions earlier, when the
IPA is loaded.  In order to guarantee malloc is not called in IPA
in gdb_collect, I change the test to set a breakpoint on malloc, if
IPA gdb_collect calls malloc, program will hit the breakpoint, and
test fail.

continue
Continuing.

Thread 1 "" hit Breakpoint 5, 0xf7cfc200 in malloc ()
(gdb) FAIL: gdb.trace/ftrace.exp: advance through tracing

gdb/gdbserver:

2017-12-07  Yao Qi  <yao.qi@linaro.org>

	* linux-aarch64-ipa.c (initialize_low_tracepoint): Call
	aarch64_linux_read_description.
	* linux-amd64-ipa.c (idx2mask): New array.
	(get_ipa_tdesc): Move idx2mask out.
	(initialize_low_tracepoint): Initialize target descriptions.
	* linux-i386-ipa.c (idx2mask): New array.
	(get_ipa_tdesc): Move idx2mask out.
	(initialize_low_tracepoint): Initialize target descriptions.

gdb/testsuite:

2017-12-07  Yao Qi  <yao.qi@linaro.org>

	* gdb.trace/ftrace.exp (run_trace_experiment): Set breakpoint on
	malloc and catch syscall.
---
 gdb/gdbserver/linux-aarch64-ipa.c  |  1 +
 gdb/gdbserver/linux-amd64-ipa.c    | 32 +++++++++++++++++++++-----------
 gdb/gdbserver/linux-i386-ipa.c     | 25 +++++++++++++------------
 gdb/testsuite/gdb.trace/ftrace.exp | 33 ++++++++++++++++++++++++++++++---
 4 files changed, 65 insertions(+), 26 deletions(-)

diff --git a/gdb/gdbserver/linux-aarch64-ipa.c b/gdb/gdbserver/linux-aarch64-ipa.c
index b240100..c39b7ec 100644
--- a/gdb/gdbserver/linux-aarch64-ipa.c
+++ b/gdb/gdbserver/linux-aarch64-ipa.c
@@ -204,4 +204,5 @@ alloc_jump_pad_buffer (size_t size)
 void
 initialize_low_tracepoint (void)
 {
+  aarch64_linux_read_description ();
 }
diff --git a/gdb/gdbserver/linux-amd64-ipa.c b/gdb/gdbserver/linux-amd64-ipa.c
index 85d0d45..5209dab 100644
--- a/gdb/gdbserver/linux-amd64-ipa.c
+++ b/gdb/gdbserver/linux-amd64-ipa.c
@@ -169,6 +169,19 @@ supply_static_tracepoint_registers (struct regcache *regcache,
 
 #endif /* HAVE_UST */
 
+#if !defined __ILP32__
+/* Map the tdesc index to xcr0 mask.  */
+uint64_t idx2mask[X86_TDESC_LAST] = {
+  X86_XSTATE_X87_MASK,
+  X86_XSTATE_SSE_MASK,
+  X86_XSTATE_AVX_MASK,
+  X86_XSTATE_MPX_MASK,
+  X86_XSTATE_AVX_MPX_MASK,
+  X86_XSTATE_AVX_AVX512_MASK,
+  X86_XSTATE_AVX_MPX_AVX512_PKU_MASK,
+};
+#endif
+
 /* Return target_desc to use for IPA, given the tdesc index passed by
    gdbserver.  */
 
@@ -194,17 +207,6 @@ get_ipa_tdesc (int idx)
       break;
     }
 #else
-  /* Map the tdesc index to xcr0 mask.  */
-  uint64_t idx2mask[X86_TDESC_LAST] = {
-    X86_XSTATE_X87_MASK,
-    X86_XSTATE_SSE_MASK,
-    X86_XSTATE_AVX_MASK,
-    X86_XSTATE_MPX_MASK,
-    X86_XSTATE_AVX_MPX_MASK,
-    X86_XSTATE_AVX_AVX512_MASK,
-    X86_XSTATE_AVX_MPX_AVX512_PKU_MASK,
-  };
-
   return amd64_linux_read_description (idx2mask[idx], false);
 #endif
 
@@ -276,4 +278,12 @@ alloc_jump_pad_buffer (size_t size)
 void
 initialize_low_tracepoint (void)
 {
+#if defined __ILP32__
+  amd64_linux_read_description (X86_XSTATE_SSE_MASK, true);
+  amd64_linux_read_description (X86_XSTATE_AVX_MASK, true);
+  amd64_linux_read_description (X86_XSTATE_AVX_AVX512_MASK, true);
+#else
+  for (auto i = 0; i < X86_TDESC_LAST; i++)
+    amd64_linux_read_description (idx2mask[i], false);
+#endif
 }
diff --git a/gdb/gdbserver/linux-i386-ipa.c b/gdb/gdbserver/linux-i386-ipa.c
index 785a63e..8ff91b8 100644
--- a/gdb/gdbserver/linux-i386-ipa.c
+++ b/gdb/gdbserver/linux-i386-ipa.c
@@ -245,6 +245,17 @@ initialize_fast_tracepoint_trampoline_buffer (void)
     }
 }
 
+/* Map the tdesc index to xcr0 mask.  */
+uint64_t idx2mask[X86_TDESC_LAST] = {
+  X86_XSTATE_X87_MASK,
+  X86_XSTATE_SSE_MASK,
+  X86_XSTATE_AVX_MASK,
+  X86_XSTATE_MPX_MASK,
+  X86_XSTATE_AVX_MPX_MASK,
+  X86_XSTATE_AVX_AVX512_MASK,
+  X86_XSTATE_AVX_MPX_AVX512_PKU_MASK,
+};
+
 /* Return target_desc to use for IPA, given the tdesc index passed by
    gdbserver.  */
 
@@ -256,18 +267,6 @@ get_ipa_tdesc (int idx)
       internal_error (__FILE__, __LINE__,
 		      "unknown ipa tdesc index: %d", idx);
     }
-
-  /* Map the tdesc index to xcr0 mask.  */
-  uint64_t idx2mask[X86_TDESC_LAST] = {
-    X86_XSTATE_X87_MASK,
-    X86_XSTATE_SSE_MASK,
-    X86_XSTATE_AVX_MASK,
-    X86_XSTATE_MPX_MASK,
-    X86_XSTATE_AVX_MPX_MASK,
-    X86_XSTATE_AVX_AVX512_MASK,
-    X86_XSTATE_AVX_MPX_AVX512_PKU_MASK,
-  };
-
   return i386_linux_read_description (idx2mask[idx]);
 }
 
@@ -290,4 +289,6 @@ void
 initialize_low_tracepoint (void)
 {
   initialize_fast_tracepoint_trampoline_buffer ();
+  for (auto i = 0; i < X86_TDESC_LAST; i++)
+    i386_linux_read_description (idx2mask[i]);
 }
diff --git a/gdb/testsuite/gdb.trace/ftrace.exp b/gdb/testsuite/gdb.trace/ftrace.exp
index b862680..6f24256 100644
--- a/gdb/testsuite/gdb.trace/ftrace.exp
+++ b/gdb/testsuite/gdb.trace/ftrace.exp
@@ -63,9 +63,36 @@ proc run_trace_experiment {} {
 
     gdb_test_no_output "tstart" "start trace experiment"
 
-    gdb_test "continue" \
-	".*Breakpoint \[0-9\]+, end .*" \
-	"advance through tracing"
+    # Fast tracepoint can be set in signal handler, so gdb_collect in
+    # IPA shouldn't call any non-async-signal-safe functions.  It is
+    # impractical to list all non-async-signal-safe functions, and set
+    # breakpoints on them, so choose malloc only in this test.
+    gdb_test "b -q malloc"
+
+    # Performance-wise, gdb_collect in IPA shouldn't call any syscall
+    # in order to keep fast tracepoint fast enough.
+    global gdb_prompt
+    set test "catch syscall"
+    gdb_test_multiple $test $test {
+	-re "The feature \'catch syscall\' is not supported.*\r\n$gdb_prompt $" {
+	}
+	-re ".*$gdb_prompt $" {
+	    pass $test
+	}
+    }
+
+    global decimal
+    set test "advance through tracing"
+    gdb_test_multiple "continue" $test {
+	-re "Thread 2 .* hit Catchpoint $decimal \\(call to syscall .*\\).*\r\n$gdb_prompt $" {
+	    # IPA starts a helper thread, which calls accept.  Ignore it.
+	    send_gdb "continue\n"
+	    exp_continue
+	}
+	-re "Breakpoint \[0-9\]+, end .*$gdb_prompt $" {
+	    pass $test
+	}
+    }
 
     gdb_test "tstatus" ".*Trace .*" "check on trace status"
 
-- 
1.9.1

Re: [PATCH] Initialize target description early in IPA

[Pedro Alves]

Hi Yao,

Thanks for the fix.  This is OK.  A couple minor nits below.

On 12/07/2017 10:01 AM, Yao Qi wrote:

> +#if !defined __ILP32__
> +/* Map the tdesc index to xcr0 mask.  */
> +uint64_t idx2mask[X86_TDESC_LAST] = {

"static" ?

> +/* Map the tdesc index to xcr0 mask.  */
> +uint64_t idx2mask[X86_TDESC_LAST] = {

"static" ?

> +    global decimal
> +    set test "advance through tracing"
> +    gdb_test_multiple "continue" $test {
> +	-re "Thread 2 .* hit Catchpoint $decimal \\(call to syscall .*\\).*\r\n$gdb_prompt $" {
> +	    # IPA starts a helper thread, which calls accept.  Ignore it.
> +	    send_gdb "continue\n"
> +	    exp_continue
> +	}

(Bah, it's unfortunate that we don't have syntax to make
catchpoints be thread specific... )

> +	-re "Breakpoint \[0-9\]+, end .*$gdb_prompt $" {

You could use $decimal here too.

> +	    pass $test
> +	}
> +    }
>  
>      gdb_test "tstatus" ".*Trace .*" "check on trace status"

Thanks,
Pedro Alves

Re: [PATCH] Initialize target description early in IPA

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Thanks for the fix.  This is OK.  A couple minor nits below.

These comments are addressed.  Patch below is pushed in.

-- 
Yao (齐尧)
From a8806230241d201f808d856eaae4d44088117b0c Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Thu, 7 Dec 2017 17:07:01 +0000
Subject: [PATCH] Initialize target description early in IPA

Target descriptions are allocated lazily, that is fine in GDBserver,
but it is not safe to call malloc in gdb_collect in IPA, because we
can set a fast tracepoint in malloc, and when the tracepoint is hit,
gdb_collect/malloc is called, deadlock or memory corruption may be
triggered.

 #0  0xf7cfc200 in malloc ()
 #1  0xf7efdc07 in operator new(unsigned int) ()
 #2  0xf7ef7636 in allocate_target_description() ()
 #3  0xf7efcbe1 in i386_create_target_description(unsigned long long, bool) ()
 #4  0xf7efb474 in i386_linux_read_description(unsigned long long) ()
 #5  0xf7efb190 in get_ipa_tdesc(int) ()
 #6  0xf7ef9baa in gdb_collect ()

The fix is to initialize all target descriptions earlier, when the
IPA is loaded.  In order to guarantee malloc is not called in IPA
in gdb_collect, I change the test to set a breakpoint on malloc, if
IPA gdb_collect calls malloc, program will hit the breakpoint, and
test fail.

continue
Continuing.

Thread 1 "" hit Breakpoint 5, 0xf7cfc200 in malloc ()
(gdb) FAIL: gdb.trace/ftrace.exp: advance through tracing

gdb/gdbserver:

2017-12-07  Yao Qi  <yao.qi@linaro.org>

	* linux-aarch64-ipa.c (initialize_low_tracepoint): Call
	aarch64_linux_read_description.
	* linux-amd64-ipa.c (idx2mask): New array.
	(get_ipa_tdesc): Move idx2mask out.
	(initialize_low_tracepoint): Initialize target descriptions.
	* linux-i386-ipa.c (idx2mask): New array.
	(get_ipa_tdesc): Move idx2mask out.
	(initialize_low_tracepoint): Initialize target descriptions.

gdb/testsuite:

2017-12-07  Yao Qi  <yao.qi@linaro.org>

	* gdb.trace/ftrace.exp (run_trace_experiment): Set breakpoint on
	malloc and catch syscall.

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 171bc53..c84dcac 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,14 @@
+2017-12-07  Yao Qi  <yao.qi@linaro.org>
+
+	* linux-aarch64-ipa.c (initialize_low_tracepoint): Call
+	aarch64_linux_read_description.
+	* linux-amd64-ipa.c (idx2mask): New array.
+	(get_ipa_tdesc): Move idx2mask out.
+	(initialize_low_tracepoint): Initialize target descriptions.
+	* linux-i386-ipa.c (idx2mask): New array.
+	(get_ipa_tdesc): Move idx2mask out.
+	(initialize_low_tracepoint): Initialize target descriptions.
+
 2017-12-05  Simon Marchi  <simon.marchi@polymtl.ca>
 
 	* tdesc.c (struct tdesc_type): Change return type.
diff --git a/gdb/gdbserver/linux-aarch64-ipa.c b/gdb/gdbserver/linux-aarch64-ipa.c
index b240100..c39b7ec 100644
--- a/gdb/gdbserver/linux-aarch64-ipa.c
+++ b/gdb/gdbserver/linux-aarch64-ipa.c
@@ -204,4 +204,5 @@ alloc_jump_pad_buffer (size_t size)
 void
 initialize_low_tracepoint (void)
 {
+  aarch64_linux_read_description ();
 }
diff --git a/gdb/gdbserver/linux-amd64-ipa.c b/gdb/gdbserver/linux-amd64-ipa.c
index 85d0d45..81070df 100644
--- a/gdb/gdbserver/linux-amd64-ipa.c
+++ b/gdb/gdbserver/linux-amd64-ipa.c
@@ -169,6 +169,19 @@ supply_static_tracepoint_registers (struct regcache *regcache,
 
 #endif /* HAVE_UST */
 
+#if !defined __ILP32__
+/* Map the tdesc index to xcr0 mask.  */
+static uint64_t idx2mask[X86_TDESC_LAST] = {
+  X86_XSTATE_X87_MASK,
+  X86_XSTATE_SSE_MASK,
+  X86_XSTATE_AVX_MASK,
+  X86_XSTATE_MPX_MASK,
+  X86_XSTATE_AVX_MPX_MASK,
+  X86_XSTATE_AVX_AVX512_MASK,
+  X86_XSTATE_AVX_MPX_AVX512_PKU_MASK,
+};
+#endif
+
 /* Return target_desc to use for IPA, given the tdesc index passed by
    gdbserver.  */
 
@@ -194,17 +207,6 @@ get_ipa_tdesc (int idx)
       break;
     }
 #else
-  /* Map the tdesc index to xcr0 mask.  */
-  uint64_t idx2mask[X86_TDESC_LAST] = {
-    X86_XSTATE_X87_MASK,
-    X86_XSTATE_SSE_MASK,
-    X86_XSTATE_AVX_MASK,
-    X86_XSTATE_MPX_MASK,
-    X86_XSTATE_AVX_MPX_MASK,
-    X86_XSTATE_AVX_AVX512_MASK,
-    X86_XSTATE_AVX_MPX_AVX512_PKU_MASK,
-  };
-
   return amd64_linux_read_description (idx2mask[idx], false);
 #endif
 
@@ -276,4 +278,12 @@ alloc_jump_pad_buffer (size_t size)
 void
 initialize_low_tracepoint (void)
 {
+#if defined __ILP32__
+  amd64_linux_read_description (X86_XSTATE_SSE_MASK, true);
+  amd64_linux_read_description (X86_XSTATE_AVX_MASK, true);
+  amd64_linux_read_description (X86_XSTATE_AVX_AVX512_MASK, true);
+#else
+  for (auto i = 0; i < X86_TDESC_LAST; i++)
+    amd64_linux_read_description (idx2mask[i], false);
+#endif
 }
diff --git a/gdb/gdbserver/linux-i386-ipa.c b/gdb/gdbserver/linux-i386-ipa.c
index 785a63e..a1a35db 100644
--- a/gdb/gdbserver/linux-i386-ipa.c
+++ b/gdb/gdbserver/linux-i386-ipa.c
@@ -245,6 +245,17 @@ initialize_fast_tracepoint_trampoline_buffer (void)
     }
 }
 
+/* Map the tdesc index to xcr0 mask.  */
+static uint64_t idx2mask[X86_TDESC_LAST] = {
+  X86_XSTATE_X87_MASK,
+  X86_XSTATE_SSE_MASK,
+  X86_XSTATE_AVX_MASK,
+  X86_XSTATE_MPX_MASK,
+  X86_XSTATE_AVX_MPX_MASK,
+  X86_XSTATE_AVX_AVX512_MASK,
+  X86_XSTATE_AVX_MPX_AVX512_PKU_MASK,
+};
+
 /* Return target_desc to use for IPA, given the tdesc index passed by
    gdbserver.  */
 
@@ -256,18 +267,6 @@ get_ipa_tdesc (int idx)
       internal_error (__FILE__, __LINE__,
 		      "unknown ipa tdesc index: %d", idx);
     }
-
-  /* Map the tdesc index to xcr0 mask.  */
-  uint64_t idx2mask[X86_TDESC_LAST] = {
-    X86_XSTATE_X87_MASK,
-    X86_XSTATE_SSE_MASK,
-    X86_XSTATE_AVX_MASK,
-    X86_XSTATE_MPX_MASK,
-    X86_XSTATE_AVX_MPX_MASK,
-    X86_XSTATE_AVX_AVX512_MASK,
-    X86_XSTATE_AVX_MPX_AVX512_PKU_MASK,
-  };
-
   return i386_linux_read_description (idx2mask[idx]);
 }
 
@@ -290,4 +289,6 @@ void
 initialize_low_tracepoint (void)
 {
   initialize_fast_tracepoint_trampoline_buffer ();
+  for (auto i = 0; i < X86_TDESC_LAST; i++)
+    i386_linux_read_description (idx2mask[i]);
 }
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 87547cb..7becd10 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2017-12-07  Yao Qi  <yao.qi@linaro.org>
+
+	* gdb.trace/ftrace.exp (run_trace_experiment): Set breakpoint on
+	malloc and catch syscall.
+
 2017-12-07  Phil Muldoon  <pmuldoon@redhat.com>
 
        * gdb.python/py-breakpoint.exp (test_bkpt_explicit_loc): Add new
diff --git a/gdb/testsuite/gdb.trace/ftrace.exp b/gdb/testsuite/gdb.trace/ftrace.exp
index b862680..3aa8883 100644
--- a/gdb/testsuite/gdb.trace/ftrace.exp
+++ b/gdb/testsuite/gdb.trace/ftrace.exp
@@ -63,9 +63,36 @@ proc run_trace_experiment {} {
 
     gdb_test_no_output "tstart" "start trace experiment"
 
-    gdb_test "continue" \
-	".*Breakpoint \[0-9\]+, end .*" \
-	"advance through tracing"
+    # Fast tracepoint can be set in signal handler, so gdb_collect in
+    # IPA shouldn't call any non-async-signal-safe functions.  It is
+    # impractical to list all non-async-signal-safe functions, and set
+    # breakpoints on them, so choose malloc only in this test.
+    gdb_test "b -q malloc"
+
+    # Performance-wise, gdb_collect in IPA shouldn't call any syscall
+    # in order to keep fast tracepoint fast enough.
+    global gdb_prompt
+    set test "catch syscall"
+    gdb_test_multiple $test $test {
+	-re "The feature \'catch syscall\' is not supported.*\r\n$gdb_prompt $" {
+	}
+	-re ".*$gdb_prompt $" {
+	    pass $test
+	}
+    }
+
+    global decimal
+    set test "advance through tracing"
+    gdb_test_multiple "continue" $test {
+	-re "Thread 2 .* hit Catchpoint $decimal \\(call to syscall .*\\).*\r\n$gdb_prompt $" {
+	    # IPA starts a helper thread, which calls accept.  Ignore it.
+	    send_gdb "continue\n"
+	    exp_continue
+	}
+	-re "Breakpoint $decimal, end .*$gdb_prompt $" {
+	    pass $test
+	}
+    }
 
     gdb_test "tstatus" ".*Trace .*" "check on trace status"