[testsuite patch] Fix false FAIL in stap-probe.exp

[testsuite patch] Fix false FAIL in stap-probe.exp

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

Hi,

gcc-6.2.1-1.fc26.x86_64

    # Set a breakpoint with multiple probe locations.
    gdb_test "break -pstap test:two" \
        "Breakpoint \[0-9\]+ at $hex.*2 locations.*" \
        "set multi-location probe breakpoint (probe two)"
break -pstap test:two^M
Breakpoint 2 at 0x4004e0^M
(gdb) FAIL: gdb.base/stap-probe.exp: without semaphore, optimized: set multi-location probe breakpoint (probe two)

    # Set a breakpoint with multiple probe locations.
    # In this scenario, we may expect more than 2 locations because of
    # the optimizations (inlining, loop unrolling, etc).
    gdb_test "break -pstap test:two" \
        "Breakpoint .* at $hex.*\[0-9\]+ locations.*" \
        "set multi-location probe breakpoint (probe two)"
break -pstap test:two^M
Breakpoint 2 at 0x4004e0^M
(gdb) FAIL: gdb.base/stap-probe.exp: with semaphore, optimized: set multi-location probe breakpoint (probe two)

OK for check-in?


Jan

[-- Attachment #2: 1 --]
[-- Type: text/plain, Size: 841 bytes --]

gdb/testsuite/ChangeLog
2016-09-11  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/stap-probe.exp (stap_test_no_debuginfo): Try to use
	-fno-ipa-icf.

diff --git a/gdb/testsuite/gdb.base/stap-probe.exp b/gdb/testsuite/gdb.base/stap-probe.exp
index df46e80..9258926 100644
--- a/gdb/testsuite/gdb.base/stap-probe.exp
+++ b/gdb/testsuite/gdb.base/stap-probe.exp
@@ -97,8 +97,11 @@ proc stap_test_no_debuginfo {exec_name {arg ""}} {
     global testfile hex
 
     if {[prepare_for_testing ${testfile}.exp ${exec_name} ${testfile}.c \
-	   {$arg nodebug optimize=-O2}]} {
-	return -1
+	   {$arg nodebug optimize=-O2 "additional_flags=-fno-ipa-icf"}]} {
+	if {[prepare_for_testing ${testfile}.exp ${exec_name} ${testfile}.c \
+	       {$arg nodebug optimize=-O2}]} {
+	    return -1
+	}
     }
 
     if {[runto "-pstap test:user"]} {

Re: [testsuite patch] Fix false FAIL in stap-probe.exp

[Sergio Durigan Junior]

On Sunday, September 11 2016, Jan Kratochvil wrote:

> Hi,
>
> gcc-6.2.1-1.fc26.x86_64
>
>     # Set a breakpoint with multiple probe locations.
>     gdb_test "break -pstap test:two" \
>         "Breakpoint \[0-9\]+ at $hex.*2 locations.*" \
>         "set multi-location probe breakpoint (probe two)"
> break -pstap test:two^M
> Breakpoint 2 at 0x4004e0^M
> (gdb) FAIL: gdb.base/stap-probe.exp: without semaphore, optimized: set multi-location probe breakpoint (probe two)
>
>     # Set a breakpoint with multiple probe locations.
>     # In this scenario, we may expect more than 2 locations because of
>     # the optimizations (inlining, loop unrolling, etc).
>     gdb_test "break -pstap test:two" \
>         "Breakpoint .* at $hex.*\[0-9\]+ locations.*" \
>         "set multi-location probe breakpoint (probe two)"
> break -pstap test:two^M
> Breakpoint 2 at 0x4004e0^M
> (gdb) FAIL: gdb.base/stap-probe.exp: with semaphore, optimized: set multi-location probe breakpoint (probe two)
>
> OK for check-in?

Thanks for the patch.  While it does fix the problem, I'd prefer a
"compiler-agnostic" patch.  The problem is that m1 and m2 (the two
functions where probe 'two' is being defined) are exactly the same, so
they get optimized by GCC's ICF pass.  Since the contents of each
function are irrelevant (as long as there's a probe 'two' inside them),
I think declaring a dummy variable in one of the functions (probably
volatile) would solve the issue.  WDYT?

> Jan
>
> gdb/testsuite/ChangeLog
> 2016-09-11  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
> 	* gdb.base/stap-probe.exp (stap_test_no_debuginfo): Try to use
> 	-fno-ipa-icf.
>
> diff --git a/gdb/testsuite/gdb.base/stap-probe.exp b/gdb/testsuite/gdb.base/stap-probe.exp
> index df46e80..9258926 100644
> --- a/gdb/testsuite/gdb.base/stap-probe.exp
> +++ b/gdb/testsuite/gdb.base/stap-probe.exp
> @@ -97,8 +97,11 @@ proc stap_test_no_debuginfo {exec_name {arg ""}} {
>      global testfile hex
>  
>      if {[prepare_for_testing ${testfile}.exp ${exec_name} ${testfile}.c \
> -	   {$arg nodebug optimize=-O2}]} {
> -	return -1
> +	   {$arg nodebug optimize=-O2 "additional_flags=-fno-ipa-icf"}]} {
> +	if {[prepare_for_testing ${testfile}.exp ${exec_name} ${testfile}.c \
> +	       {$arg nodebug optimize=-O2}]} {
> +	    return -1
> +	}
>      }
>  
>      if {[runto "-pstap test:user"]} {
>

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [testsuite patch] Fix false FAIL in stap-probe.exp

[Jan Kratochvil]

On Sun, 11 Sep 2016 19:43:41 +0200, Sergio Durigan Junior wrote:
> Thanks for the patch.  While it does fix the problem, I'd prefer a
> "compiler-agnostic" patch.  The problem is that m1 and m2 (the two
> functions where probe 'two' is being defined) are exactly the same, so
> they get optimized by GCC's ICF pass.  Since the contents of each
> function are irrelevant (as long as there's a probe 'two' inside them),
> I think declaring a dummy variable in one of the functions (probably
> volatile) would solve the issue.  WDYT?

The problem is there are tons of optimizations the compiler can do.  If you
change anything there the compiler can do partial inlining, tail calls etc.

An unused dummy variable gets optimized out and so the problem remains.

A dummy variable would need to have __attribute__((used)) but (1) for a reason
unknown to me it does not work anyway
	gdb.base/stap-probe.c:56:2: warning: 'used' attribute ignored [-Wattributes]
and besides that (2) __attribute__ is also not much "compiler-agnostic" IMO.

My feeling from such cases is that if you try to outsmart the compiler the
next version of compiler will outsmart you some other way again.

Sure there are many ways to workaround it, I have proposed one of them.
If you want a different one you can either submit a different patch or to be
more specific how I should code the patch.  But given you should test your
idea first I guess it is more straighforward to just submit some other your
patch if you do not like the patch of mine.


Jan

Re: [testsuite patch] Fix false FAIL in stap-probe.exp

[Sergio Durigan Junior]

[-- Attachment #1: Type: text/plain, Size: 2005 bytes --]

On Sunday, September 11 2016, Jan Kratochvil wrote:

> On Sun, 11 Sep 2016 19:43:41 +0200, Sergio Durigan Junior wrote:
>> Thanks for the patch.  While it does fix the problem, I'd prefer a
>> "compiler-agnostic" patch.  The problem is that m1 and m2 (the two
>> functions where probe 'two' is being defined) are exactly the same, so
>> they get optimized by GCC's ICF pass.  Since the contents of each
>> function are irrelevant (as long as there's a probe 'two' inside them),
>> I think declaring a dummy variable in one of the functions (probably
>> volatile) would solve the issue.  WDYT?
>
> The problem is there are tons of optimizations the compiler can do.  If you
> change anything there the compiler can do partial inlining, tail calls etc.

Right, but if the compiler changes again then we'll have to go back and
change the test too, which means that we'll end up losing anyway.

> An unused dummy variable gets optimized out and so the problem remains.

No if we mark it as volatile and use it as the argument for one of the
probes.

> A dummy variable would need to have __attribute__((used)) but (1) for a reason
> unknown to me it does not work anyway
> 	gdb.base/stap-probe.c:56:2: warning: 'used' attribute ignored [-Wattributes]
> and besides that (2) __attribute__ is also not much "compiler-agnostic" IMO.
>
> My feeling from such cases is that if you try to outsmart the compiler the
> next version of compiler will outsmart you some other way again.
>
> Sure there are many ways to workaround it, I have proposed one of them.
> If you want a different one you can either submit a different patch or to be
> more specific how I should code the patch.  But given you should test your
> idea first I guess it is more straighforward to just submit some other your
> patch if you do not like the patch of mine.

What do you think of the patch below?

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-false-FAIL-on-gdb.base-stap-probe.exp-due-to-ICF.patch --]
[-- Type: text/x-patch, Size: 2376 bytes --]

From 3887cf94273288eeeb09b8dec41849422104e0d9 Mon Sep 17 00:00:00 2001
From: Sergio Durigan Junior <sergiodj@redhat.com>
Date: Sun, 11 Sep 2016 16:53:09 -0400
Subject: [PATCH] Fix false FAIL on gdb.base/stap-probe.exp, due to ICF
 optimization

GCC 6's ICF optimization pass is making the declaration of 'm1' and
'm2', on gdb.base/stap-probe.c, to be unified.  However, this leads to
only one instance of the probe 'two' being created, which causes a
failure on the testsuite (which expects a multi-location breakpoint to
be inserted on the probe).

This patch fixes this failure by declaring a dummy variable on 'm1',
and using it as an argument to m1's version of probe 'two'.  Since we
do not care about the contents of the functions nor about the
arguments of each probe 'two', this is OK.

gdb/testsuite/ChangeLog:
2016-09-11  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/stap-probe.c (m1): New variable 'dummy', necessary to
	make m1's definition to be different from m2's.  Use 'dummy' as an
	argument for probe 'two'.
---
 gdb/testsuite/ChangeLog             | 7 +++++++
 gdb/testsuite/gdb.base/stap-probe.c | 7 ++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 1ed9a21..c63ea72 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2016-09-11  Sergio Durigan Junior  <sergiodj@redhat.com>
+	    Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* gdb.base/stap-probe.c (m1): New variable 'dummy', necessary to
+	make m1's definition to be different from m2's.  Use 'dummy' as an
+	argument for probe 'two'.
+
 2016-09-10  Jon Beniston  <jon@beniston.com>
 
 	* lib/mi-support.exp (mi_gdb_target_load): Use target_sim_options
diff --git a/gdb/testsuite/gdb.base/stap-probe.c b/gdb/testsuite/gdb.base/stap-probe.c
index b728548..5a77435 100644
--- a/gdb/testsuite/gdb.base/stap-probe.c
+++ b/gdb/testsuite/gdb.base/stap-probe.c
@@ -53,8 +53,13 @@ struct funcs
 static void
 m1 (void)
 {
+  /* m1 and m2 are equivalent, but because of some compiler
+     optimizations we have to make each of them unique.  This is why
+     we have this dummy variable here.  */
+  volatile int dummy = 0;
+
   if (TEST2)
-    STAP_PROBE (test, two);
+    STAP_PROBE1 (test, two, dummy);
 }
 
 static void
-- 
2.7.4

Re: [testsuite patch] Fix false FAIL in stap-probe.exp

[Jan Kratochvil]

On Sun, 11 Sep 2016 22:57:43 +0200, Sergio Durigan Junior wrote:
> No if we mark it as volatile and use it as the argument for one of the
> probes.

I did not realize the two probe instances can have different parameters and
they still will become a multi-location probe.


> What do you think of the patch below?

I find it fine/better than the patch of mine.  I do not see relevance of my
name on your patch but I do not mind either way.


Thanks,
Jan

Re: [testsuite patch] Fix false FAIL in stap-probe.exp

[Sergio Durigan Junior]

On Sunday, September 11 2016, Jan Kratochvil wrote:

> On Sun, 11 Sep 2016 22:57:43 +0200, Sergio Durigan Junior wrote:
>> No if we mark it as volatile and use it as the argument for one of the
>> probes.
>
> I did not realize the two probe instances can have different parameters and
> they still will become a multi-location probe.

Yeah, the probe name is what matters for the location.

>> What do you think of the patch below?
>
> I find it fine/better than the patch of mine.  I do not see relevance of my
> name on your patch but I do not mind either way.

Thanks.  Checked in with your name in it, because you found the bug.

2c29df25b7c2ff006b45afd80ee6dd734ebbd47c

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/