[PATCH 2/2] Determine the iteration count based on wallclock instead of user+system time.

[PATCH 2/2] Determine the iteration count based on wallclock instead of user+system time.

[Wei-cheng Wang]

Hi Ulrich,

This patch will combine with "Tracepoint for ppc64" into one patch.
Is this ok?

Thanks,
Wei-cheng

---

gdb/testsuite/ChangeLog

2015-08-23  Wei-cheng Wang  <cole945@gmail.com>

	* gdb.trace/tspeed.c (myclock): Return wallclock instead of
	user+system time.
	(trace_speed_test): Determine the iteration count for a time
	between 15..30 seconds.
---
 gdb/testsuite/gdb.trace/tspeed.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/tspeed.c b/gdb/testsuite/gdb.trace/tspeed.c
index ca4c7bf..c9e212c 100644
--- a/gdb/testsuite/gdb.trace/tspeed.c
+++ b/gdb/testsuite/gdb.trace/tspeed.c
@@ -56,13 +56,9 @@ int nspertp = 0;
 unsigned long long
 myclock ()
 {
-  struct timeval tm, tm2;
-  struct rusage ru;
-  getrusage (RUSAGE_SELF, &ru);
-  tm = ru.ru_utime;
-  tm2 = ru.ru_stime;
-  return (((unsigned long long) tm.tv_sec) * 1000000) + tm.tv_usec
-    + (((unsigned long long) tm2.tv_sec) * 1000000) + tm2.tv_usec;
+  struct timeval tm;
+  gettimeofday (&tm, NULL);
+  return (((unsigned long long) tm.tv_sec) * 1000000) + tm.tv_usec;
 }
 
 int
@@ -162,9 +158,9 @@ trace_speed_test (void)
     return -1;
 
   if (idelta > mindelta
-      /* Total test time should be between 2 and 5 seconds.  */
-      && (total1 + total2) > (2 * 1000000)
-      && (total1 + total2) < (5 * 1000000)) 
+      /* Total test time should be between 15 and 30 seconds.  */
+      && (total1 + total2) > (15 * 1000000)
+      && (total1 + total2) < (30 * 1000000))
     {
       nsdelta = (((unsigned long long) idelta) * 1000) / iters;
       printf ("Second loop took %d ns longer per iter than first\n", nsdelta);
-- 
1.9.1

[PATCH 1/2] Re-check fastpoint after reloading symbols.

[Wei-cheng Wang]

Hi Ulrich,

This is a separate patch and should go before "Tracepoint for ppc64" patch.
Is this ok?

Thanks,
Wei-cheng

---
Check fast tracepoints after symbols have been re-loaded.
For example, a pending tracepoint just becomes available after
a new shared object being loaded.  We didn't check it before,
because we have no idea where it is.

If the target rejects the tracepoint, an error is throw in
check_fast_tracepoint_sals, and we will disable the breakpoint.

The checking is deliberately put after the loop for adding location
to breakpoint, so users can check the address for the tracepoint
with `info trace'.  Otherwise, it will simply show <PENDING> instead
of the address.

(gdb) info trace
Num     Type            Disp Enb Address            What
1       fast tracepoint keep n   0x00003fffb7f507dc <pendfunc+20>
                                 ^^^^^^^^^^^^^^^^^^

--

gdb/ChangeLog

2015-08-23  Wei-cheng Wang  <cole945@gmail.com>

	* breakpoint.c (update_breakpoint_locations): Check
	fast tracepoints after reloading symbols.
---
 gdb/breakpoint.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 052aeb9..703b03a 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14056,15 +14056,6 @@ update_breakpoint_locations (struct breakpoint *b,
       return;
     }
 
-  /* If there's no new locations, and all existing locations are
-     pending, don't do anything.  This optimizes the common case where
-     all locations are in the same shared library, that was unloaded.
-     We'd like to retain the location, so that when the library is
-     loaded again, we don't loose the enabled/disabled status of the
-     individual locations.  */
-  if (all_locations_are_pending (existing_locations) && sals.nelts == 0)
-    return;
-
   b->loc = NULL;
 
   for (i = 0; i < sals.nelts; ++i)
@@ -14106,6 +14097,29 @@ update_breakpoint_locations (struct breakpoint *b,
 	}
     }
 
+  /* If there's no new locations, and all existing locations are
+     pending, don't do anything.  This optimizes the common case where
+     all locations are in the same shared library, that was unloaded.
+     We'd like to retain the location, so that when the library is
+     loaded again, we don't loose the enabled/disabled status of the
+     individual locations.  */
+  if (all_locations_are_pending (existing_locations) && sals.nelts == 0)
+    return;
+
+  if (b->type == bp_fast_tracepoint)
+    {
+      TRY
+	{
+	  check_fast_tracepoint_sals (get_current_arch(), &sals);
+	}
+      CATCH (e, RETURN_MASK_ERROR)
+	{
+	  b->enable_state = bp_disabled;
+	  throw_exception (e);
+	}
+      END_CATCH
+    }
+
   /* If possible, carry over 'disable' status from existing
      breakpoints.  */
   {
-- 
1.9.1

Re: [PATCH 1/2] Re-check fastpoint after reloading symbols.

[Ulrich Weigand]

Wei-cheng Wang wrote:

> Check fast tracepoints after symbols have been re-loaded.
> For example, a pending tracepoint just becomes available after
> a new shared object being loaded.  We didn't check it before,
> because we have no idea where it is.
> 
> If the target rejects the tracepoint, an error is throw in
> check_fast_tracepoint_sals, and we will disable the breakpoint.
> 
> The checking is deliberately put after the loop for adding location
> to breakpoint, so users can check the address for the tracepoint
> with `info trace'.  Otherwise, it will simply show <PENDING> instead
> of the address.
> 
> (gdb) info trace
> Num     Type            Disp Enb Address            What
> 1       fast tracepoint keep n   0x00003fffb7f507dc <pendfunc+20>
>                                  ^^^^^^^^^^^^^^^^^^

This makes sense to me.

However, we now have allowed a disabled breakpoint with an invalid
address in the list.  If the user simply enables the breakpoint now
and continues, we will run into the same internal error as before ...

So we probably ought to add the same check to enable_breakpoint.

> -  /* If there's no new locations, and all existing locations are
> -     pending, don't do anything.  This optimizes the common case where
> -     all locations are in the same shared library, that was unloaded.
> -     We'd like to retain the location, so that when the library is
> -     loaded again, we don't loose the enabled/disabled status of the
> -     individual locations.  */
> -  if (all_locations_are_pending (existing_locations) && sals.nelts == 0)
> -    return;

I don't understand why you're moving this piece of code down.  This just
seems to disable the optimization mentioned in the comment ...

> +  if (b->type == bp_fast_tracepoint)
> +    {
> +      TRY
> +	{
> +	  check_fast_tracepoint_sals (get_current_arch(), &sals);
This should use b->gdbarch instead of get_current_arch().

> +	}
> +      CATCH (e, RETURN_MASK_ERROR)
> +	{
> +	  b->enable_state = bp_disabled;
> +	  throw_exception (e);
> +	}
> +      END_CATCH
> +    }
> +
>    /* If possible, carry over 'disable' status from existing
>       breakpoints.  */

Hmm.  I think the check still needs to be moved further down.

Note that if there are other breakpoints, the have been re-set by the
loop above.  For those other breakpoints, we still should do the
actions done in the rest of this routine:
- carry over disable status
- notify observers
- update global location list

But if you run into an error during check_fast_tracepoint_sals,
you throw an exception, which skips all this code.  That's not
a good idea ...

I think we should move the check to the very end of the routine.
(In fact, we then might move it up into the caller, i.e. to the
end of tracepoint_re_set.)

Also, some test that all this works as expected would be good.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH 2/2] Determine the iteration count based on wallclock instead of user+system time.

[Ulrich Weigand]

Wei-cheng Wang wrote:

> 2015-08-23  Wei-cheng Wang  <cole945@gmail.com>
> 
> 	* gdb.trace/tspeed.c (myclock): Return wallclock instead of
> 	user+system time.
> 	(trace_speed_test): Determine the iteration count for a time
> 	between 15..30 seconds.

This is OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH 2/2] Determine the iteration count based on wallclock instead of user+system time.

[Marcin Kościelnicki]

On 20/08/15 19:32, Wei-cheng Wang wrote:
> Hi Ulrich,
>
> This patch will combine with "Tracepoint for ppc64" into one patch.
> Is this ok?
>
> Thanks,
> Wei-cheng
>

This patch has been accepted half a year ago, and the author has 
disappeared.  It fixes the same underlying issue as my patch at 
https://sourceware.org/ml/gdb-patches/2016-01/msg00716.html .  Since 
this patch seems to be a smarter fix, I'd like to abandon mine, and go 
with that one instead.  I've just checked it - works just fine and fixes 
the problem on s390{,x}-ibm-linux-gnu, no regressions on 
x86_64-unknown-linx-gnu.

OK to push?

> ---
>
> gdb/testsuite/ChangeLog
>
> 2015-08-23  Wei-cheng Wang  <cole945@gmail.com>
>
> 	* gdb.trace/tspeed.c (myclock): Return wallclock instead of
> 	user+system time.
> 	(trace_speed_test): Determine the iteration count for a time
> 	between 15..30 seconds.
> ---
>   gdb/testsuite/gdb.trace/tspeed.c | 16 ++++++----------
>   1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.trace/tspeed.c b/gdb/testsuite/gdb.trace/tspeed.c
> index ca4c7bf..c9e212c 100644
> --- a/gdb/testsuite/gdb.trace/tspeed.c
> +++ b/gdb/testsuite/gdb.trace/tspeed.c
> @@ -56,13 +56,9 @@ int nspertp = 0;
>   unsigned long long
>   myclock ()
>   {
> -  struct timeval tm, tm2;
> -  struct rusage ru;
> -  getrusage (RUSAGE_SELF, &ru);
> -  tm = ru.ru_utime;
> -  tm2 = ru.ru_stime;
> -  return (((unsigned long long) tm.tv_sec) * 1000000) + tm.tv_usec
> -    + (((unsigned long long) tm2.tv_sec) * 1000000) + tm2.tv_usec;
> +  struct timeval tm;
> +  gettimeofday (&tm, NULL);
> +  return (((unsigned long long) tm.tv_sec) * 1000000) + tm.tv_usec;
>   }
>
>   int
> @@ -162,9 +158,9 @@ trace_speed_test (void)
>       return -1;
>
>     if (idelta > mindelta
> -      /* Total test time should be between 2 and 5 seconds.  */
> -      && (total1 + total2) > (2 * 1000000)
> -      && (total1 + total2) < (5 * 1000000))
> +      /* Total test time should be between 15 and 30 seconds.  */
> +      && (total1 + total2) > (15 * 1000000)
> +      && (total1 + total2) < (30 * 1000000))
>       {
>         nsdelta = (((unsigned long long) idelta) * 1000) / iters;
>         printf ("Second loop took %d ns longer per iter than first\n", nsdelta);
>

Re: [PATCH 2/2] Determine the iteration count based on wallclock instead of user+system time.

[Ulrich Weigand]

> This patch has been accepted half a year ago, and the author has 
> disappeared.  It fixes the same underlying issue as my patch at 
> https://sourceware.org/ml/gdb-patches/2016-01/msg00716.html .  Since 
> this patch seems to be a smarter fix, I'd like to abandon mine, and go 
> with that one instead.  I've just checked it - works just fine and fixes 
> the problem on s390{,x}-ibm-linux-gnu, no regressions on 
> x86_64-unknown-linx-gnu.
> 
> OK to push?

This is OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH 2/2] Determine the iteration count based on wallclock instead of user+system time.

[Marcin Kościelnicki]

On 18/02/16 19:56, Ulrich Weigand wrote:
>> This patch has been accepted half a year ago, and the author has
>> disappeared.  It fixes the same underlying issue as my patch at
>> https://sourceware.org/ml/gdb-patches/2016-01/msg00716.html .  Since
>> this patch seems to be a smarter fix, I'd like to abandon mine, and go
>> with that one instead.  I've just checked it - works just fine and fixes
>> the problem on s390{,x}-ibm-linux-gnu, no regressions on
>> x86_64-unknown-linx-gnu.
>>
>> OK to push?
>
> This is OK.
>
> Thanks,
> Ulrich
>

Thanks, pushed.