[PATCH] Change handling of '!' operator in stap probes

[PATCH] Change handling of '!' operator in stap probes

[Tom Tromey]

While working on the expression rewrite, I noticed that the '!'
operator is handled as if it were a binary operator.  However, this
seems incorrect.  Instead, I think it should be handled alongside the
other unary operators in stap_parse_single_operand.

This patch makes this change.

Tested on x86-64 Fedora 32.  However, I am not completely certain
whether the test suite manages to test this code.

gdb/ChangeLog
2020-12-31  Tom Tromey  <tom@tromey.com>

	* stap-probe.c (enum stap_operand_prec): Update comment.
	(stap_get_operator_prec): Don't mention UNOP_LOGICAL_NOT.
	(stap_get_opcode): Don't handle '!'.
	(stap_parse_single_operand): Handle '!' here.
---
 gdb/ChangeLog    |  7 +++++++
 gdb/stap-probe.c | 12 ++++--------
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index ffcded3c317..366bba093d9 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -253,8 +253,7 @@ enum stap_operand_prec
      greater-than, etc) operands.  */
   STAP_OPERAND_PREC_ADD_CMP,
 
-  /* Precedence of bitwise operands (bitwise OR, XOR, bitwise AND,
-     logical NOT).  */
+  /* Precedence of bitwise operands (bitwise OR, XOR, bitwise AND).  */
   STAP_OPERAND_PREC_BITWISE,
 
   /* Precedence of multiplicative operands (multiplication, division,
@@ -306,7 +305,6 @@ stap_get_operator_prec (enum exp_opcode op)
     case BINOP_BITWISE_IOR:
     case BINOP_BITWISE_AND:
     case BINOP_BITWISE_XOR:
-    case UNOP_LOGICAL_NOT:
       return STAP_OPERAND_PREC_BITWISE;
 
     case BINOP_MUL:
@@ -402,10 +400,6 @@ stap_get_opcode (const char **s)
       op = BINOP_BITWISE_XOR;
       break;
 
-    case '!':
-      op = UNOP_LOGICAL_NOT;
-      break;
-
     case '+':
       op = BINOP_ADD;
       break;
@@ -870,7 +864,7 @@ stap_parse_single_operand (struct stap_parse_info *p)
       return;
     }
 
-  if (*p->arg == '-' || *p->arg == '~' || *p->arg == '+')
+  if (*p->arg == '-' || *p->arg == '~' || *p->arg == '+' || *p->arg == '!')
     {
       char c = *p->arg;
       /* We use this variable to do a lookahead.  */
@@ -924,6 +918,8 @@ stap_parse_single_operand (struct stap_parse_info *p)
 	    write_exp_elt_opcode (&p->pstate, UNOP_NEG);
 	  else if (c == '~')
 	    write_exp_elt_opcode (&p->pstate, UNOP_COMPLEMENT);
+	  else if (c == '!')
+	    write_exp_elt_opcode (&p->pstate, UNOP_LOGICAL_NOT);
 	}
     }
   else if (isdigit (*p->arg))
-- 
2.26.2

Re: [PATCH] Change handling of '!' operator in stap probes

[Sergio Durigan Junior]

On Thursday, December 31 2020, Tom Tromey wrote:

> While working on the expression rewrite, I noticed that the '!'
> operator is handled as if it were a binary operator.  However, this
> seems incorrect.  Instead, I think it should be handled alongside the
> other unary operators in stap_parse_single_operand.
>
> This patch makes this change.

Thanks for the patch, Tom.

This code was written based on the information contained here:

  https://sourceware.org/binutils/docs/as/Infix-Ops.html

You can see that it says:

  !
    Bitwise Or Not

The problem is that, when we examine gas' source code, we see that it
doesn't treat '!' as "bitwise or not" (fortunately!), but as the
"logical not":

  https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gas/symbols.c;h=f6e4b718ef719eef8a7e6e29dbadf3605058403d;hb=HEAD#l1128

OK, this means that our stap-probe.c is not completely wrong: the enum
should indeed be named UNOP_LOGICAL_NOT.

Now, when we check to see if the precedence assigned to UNOP_LOGICAL_NOT
is the right one, we see that it is:

  https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gas/expr.c;h=1b420d928cde50456af0137529b8215294ba521d;hb=HEAD#l1481


  /* Rank	Examples
     0	operand, (expression)
     1	||
     2	&&
     3	== <> < <= >= >
     4	+ -
     5	used for * / % in MRI mode
     6	& ^ ! |
  ...

Huh, interesting.  Anyway, this whole investigation (plus the fact that,
as you mentioned, we don't have tests to exercise this part of the code)
made me uncover a few latent bugs on the stap parser code, so I'm
sending a patch that incorporates part of your changes and also fixes
the issues I found.  As a bonus, I'm proposing a new testcase
(amd64-only) to make sure that we expand the coverage of this code.

WDYT?

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

From da02db7cacef62ce085d5715b1339b24d8f2260c Mon Sep 17 00:00:00 2001
From: Sergio Durigan Junior <sergiodj@sergiodj.net>
Date: Sun, 3 Jan 2021 02:42:52 -0500
Subject: [PATCH] Fix a few stap parser issues and add a new test for probe
 expressions

The creation of this patch was motivated by Tom's "Change handling of
'!' operator in stap probes" patch.

While reviewing his patch, I stumbled upon a few issues with the stap
expression parser.  They are:

- As it turns out, even with Tom's patch applied the parser doesn't
  properly handle the '!' operator.  The underlying issue was the fact
  that stap_parse_argument_conditionally also needed to be patched in
  order to recognize '!' as an operator that is part of a single
  operand, and parse it accordingly.

- While writing the testcase I'm proposing on this patch, I found that
  parenthesized sub-expressions were not being parsed correctly when
  there was another term after them.  For example:

    1 - (2 + 3) + 4

  In this case, the parser was considering "1" to be the left-side of
  the expression, and "(2 + 3) + 4" to be the right-side.  The patch
  fixes the parser by making it identify whether a parenthesized
  sub-expression has just been parsed, and act accordingly.

I've tested this on my Debian testing amd64, and everything seems OK.

gdb/ChangeLog:
2021-01-03  Sergio Durigan Junior  <sergiodj@sergiodj.net>
	    Tom Tromey <tom@tromey.com>

	* stap-probe.c (stap_parse_single_operand): Handle '!'
	operator.
	(stap_parse_argument_conditionally): Likewise.
	Skip spaces after processing open-parenthesis sub-expression.
	(stap_parse_argument_1): Skip spaces after call to
	stap_parse_argument_conditionally.
	Handle case when right-side expression is a parenthesized
	sub-expression.
	Skip spaces after call to stap_parse_argument_1.

gdb/testsuite/ChangeLog:
2021-01-03  Sergio Durigan Junior  <sergiodj@sergiodj.net>

	* gdb.arch/amd64-stap-expressions.S: New file.
	* gdb.arch/amd64-stap-expressions.exp: New file.
---
 gdb/stap-probe.c                              | 31 +++++++--
 .../gdb.arch/amd64-stap-expressions.S         | 43 ++++++++++++
 .../gdb.arch/amd64-stap-expressions.exp       | 68 +++++++++++++++++++
 3 files changed, 137 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/amd64-stap-expressions.S
 create mode 100644 gdb/testsuite/gdb.arch/amd64-stap-expressions.exp

diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index c2ddd047f6..224dd5714f 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -870,7 +870,7 @@ stap_parse_single_operand (struct stap_parse_info *p)
       return;
     }
 
-  if (*p->arg == '-' || *p->arg == '~' || *p->arg == '+')
+  if (*p->arg == '-' || *p->arg == '~' || *p->arg == '+' || *p->arg == '!')
     {
       char c = *p->arg;
       /* We use this variable to do a lookahead.  */
@@ -924,6 +924,8 @@ stap_parse_single_operand (struct stap_parse_info *p)
 	    write_exp_elt_opcode (&p->pstate, UNOP_NEG);
 	  else if (c == '~')
 	    write_exp_elt_opcode (&p->pstate, UNOP_COMPLEMENT);
+	  else if (c == '!')
+	    write_exp_elt_opcode (&p->pstate, UNOP_LOGICAL_NOT);
 	}
     }
   else if (isdigit (*p->arg))
@@ -1012,7 +1014,7 @@ stap_parse_argument_conditionally (struct stap_parse_info *p)
 {
   gdb_assert (gdbarch_stap_is_single_operand_p (p->gdbarch));
 
-  if (*p->arg == '-' || *p->arg == '~' || *p->arg == '+' /* Unary.  */
+  if (*p->arg == '-' || *p->arg == '~' || *p->arg == '+' || *p->arg == '!'
       || isdigit (*p->arg)
       || gdbarch_stap_is_single_operand (p->gdbarch, p->arg))
     stap_parse_single_operand (p);
@@ -1027,11 +1029,12 @@ stap_parse_argument_conditionally (struct stap_parse_info *p)
 
       stap_parse_argument_1 (p, 0, STAP_OPERAND_PREC_NONE);
 
-      --p->inside_paren_p;
+      p->arg = skip_spaces (p->arg);
       if (*p->arg != ')')
-	error (_("Missign close-paren on expression `%s'."),
+	error (_("Missign close-parenthesis on expression `%s'."),
 	       p->saved_arg);
 
+      --p->inside_paren_p;
       ++p->arg;
       if (p->inside_paren_p)
 	p->arg = skip_spaces (p->arg);
@@ -1067,6 +1070,9 @@ stap_parse_argument_1 (struct stap_parse_info *p, bool has_lhs,
       stap_parse_argument_conditionally (p);
     }
 
+  if (p->inside_paren_p)
+    p->arg = skip_spaces (p->arg);
+
   /* Start to parse the right-side, and to "join" left and right sides
      depending on the operation specified.
 
@@ -1104,8 +1110,21 @@ stap_parse_argument_1 (struct stap_parse_info *p, bool has_lhs,
       if (p->inside_paren_p)
 	p->arg = skip_spaces (p->arg);
 
-      /* Parse the right-side of the expression.  */
+      /* Parse the right-side of the expression.
+
+	 We save whether the right-side is a parenthesized
+	 subexpression because, if it is, we will have to finish
+	 processing this part of the expression before continuing.  */
+      bool paren_subexp = *p->arg == '(';
+
       stap_parse_argument_conditionally (p);
+      if (p->inside_paren_p)
+	p->arg = skip_spaces (p->arg);
+      if (paren_subexp)
+	{
+	  write_exp_elt_opcode (&p->pstate, opcode);
+	  continue;
+	}
 
       /* While we still have operators, try to parse another
 	 right-side, but using the current right-side as a left-side.  */
@@ -1130,6 +1149,8 @@ stap_parse_argument_1 (struct stap_parse_info *p, bool has_lhs,
 	  /* Parse the right-side of the expression, but since we already
 	     have a left-side at this point, set `has_lhs' to 1.  */
 	  stap_parse_argument_1 (p, 1, lookahead_prec);
+	  if (p->inside_paren_p)
+	    p->arg = skip_spaces (p->arg);
 	}
 
       write_exp_elt_opcode (&p->pstate, opcode);
diff --git a/gdb/testsuite/gdb.arch/amd64-stap-expressions.S b/gdb/testsuite/gdb.arch/amd64-stap-expressions.S
new file mode 100644
index 0000000000..76a47aa9b5
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-stap-expressions.S
@@ -0,0 +1,43 @@
+/* Copyright (C) 2021 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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 <sys/sdt.h>
+
+	.file	"amd64-stap-expressions.S"
+	.text
+	.globl	main
+main:
+	/* We use a nop here because we don't want the first probe to
+	   be placed at the same location as the main label.  */
+	nop
+
+	/* Single operands.  */
+	STAP_PROBE1(probe, log_neg, 8@!($0+$1))
+	STAP_PROBE1(probe, minus, -8@-($3+$4))
+	STAP_PROBE1(probe, bit_neg, -8@~$22)
+
+	/* Arithmetic expressions.  */
+	STAP_PROBE1(probe, plus1, 8@$3+($10-$8)-$1)
+	STAP_PROBE1(probe, plus2, 8@$100-(  ($8+$10) -$50)+$3)
+	STAP_PROBE1(probe, plus3, 8@$100-(($8+$10)-$50)+((($8 - $9) + $40) - $4)+$4)
+
+	/* Bitwise expressions.  */
+	STAP_PROBE1(probe, and, 8@$128&$128)
+	STAP_PROBE1(probe, or, 8@$8|$4)
+
+	xor	%rax,%rax
+	ret
diff --git a/gdb/testsuite/gdb.arch/amd64-stap-expressions.exp b/gdb/testsuite/gdb.arch/amd64-stap-expressions.exp
new file mode 100644
index 0000000000..5e3cb60a9e
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-stap-expressions.exp
@@ -0,0 +1,68 @@
+# Copyright 2021 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/>.
+
+standard_testfile ".S"
+
+if { ![istarget "x86_64-*-*"] || ![is_lp64_target] } {
+    verbose "Skipping $testfile.exp"
+    return
+}
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
+    return -1
+}
+
+# Helper procedure to go to probe NAME
+
+proc goto_probe { name } {
+    global decimal hex
+
+    gdb_test "break -pstap $name" "Breakpoint $decimal at $hex"
+    gdb_test "continue" "Breakpoint $decimal, main \\(\\) at .*\r\n.*STAP_PROBE1.*${name},.*\\)"
+}
+
+# Helper procedure to test the probe's argument
+
+proc test_probe_value { value } {
+    gdb_test "print \$_probe_argc" "= 1"
+    gdb_test "print \$_probe_arg0" "= $value"
+}
+
+if { ![runto_main] } {
+    return -1
+}
+
+# Name and expected value for each probe.
+set probe_names_and_values {
+    { "log_neg" "0" }
+    { "minus" "-7" }
+    { "bit_neg" "-23" }
+
+    { "plus1" "4" }
+    { "plus2" "135" }
+    { "plus3" "171" }
+
+    { "and" "128" }
+    { "or" "12" }
+}
+
+foreach probe_info $probe_names_and_values {
+    set name [lindex $probe_info 0]
+    set value [lindex $probe_info 1]
+    with_test_prefix $name {
+	goto_probe $name
+	test_probe_value $value
+    }
+}
-- 
2.29.2

Re: [PATCH] Change handling of '!' operator in stap probes

[Sergio Durigan Junior]

On Sunday, January 03 2020, Sergio Durigan Junior via Gdb-patches wrote:

> On Thursday, December 31 2020, Tom Tromey wrote:
>
>> While working on the expression rewrite, I noticed that the '!'
>> operator is handled as if it were a binary operator.  However, this
>> seems incorrect.  Instead, I think it should be handled alongside the
>> other unary operators in stap_parse_single_operand.
>>
>> This patch makes this change.

Ping.

>
> Thanks for the patch, Tom.
>
> This code was written based on the information contained here:
>
>   https://sourceware.org/binutils/docs/as/Infix-Ops.html
>
> You can see that it says:
>
>   !
>     Bitwise Or Not
>
> The problem is that, when we examine gas' source code, we see that it
> doesn't treat '!' as "bitwise or not" (fortunately!), but as the
> "logical not":
>
>   https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gas/symbols.c;h=f6e4b718ef719eef8a7e6e29dbadf3605058403d;hb=HEAD#l1128
>
> OK, this means that our stap-probe.c is not completely wrong: the enum
> should indeed be named UNOP_LOGICAL_NOT.
>
> Now, when we check to see if the precedence assigned to UNOP_LOGICAL_NOT
> is the right one, we see that it is:
>
>   https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gas/expr.c;h=1b420d928cde50456af0137529b8215294ba521d;hb=HEAD#l1481
>
>
>   /* Rank	Examples
>      0	operand, (expression)
>      1	||
>      2	&&
>      3	== <> < <= >= >
>      4	+ -
>      5	used for * / % in MRI mode
>      6	& ^ ! |
>   ...
>
> Huh, interesting.  Anyway, this whole investigation (plus the fact that,
> as you mentioned, we don't have tests to exercise this part of the code)
> made me uncover a few latent bugs on the stap parser code, so I'm
> sending a patch that incorporates part of your changes and also fixes
> the issues I found.  As a bonus, I'm proposing a new testcase
> (amd64-only) to make sure that we expand the coverage of this code.
>
> WDYT?
>
> -- 
> Sergio
> GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
> Please send encrypted e-mail if possible
> https://sergiodj.net/
>
> From da02db7cacef62ce085d5715b1339b24d8f2260c Mon Sep 17 00:00:00 2001
> From: Sergio Durigan Junior <sergiodj@sergiodj.net>
> Date: Sun, 3 Jan 2021 02:42:52 -0500
> Subject: [PATCH] Fix a few stap parser issues and add a new test for probe
>  expressions
>
> The creation of this patch was motivated by Tom's "Change handling of
> '!' operator in stap probes" patch.
>
> While reviewing his patch, I stumbled upon a few issues with the stap
> expression parser.  They are:
>
> - As it turns out, even with Tom's patch applied the parser doesn't
>   properly handle the '!' operator.  The underlying issue was the fact
>   that stap_parse_argument_conditionally also needed to be patched in
>   order to recognize '!' as an operator that is part of a single
>   operand, and parse it accordingly.
>
> - While writing the testcase I'm proposing on this patch, I found that
>   parenthesized sub-expressions were not being parsed correctly when
>   there was another term after them.  For example:
>
>     1 - (2 + 3) + 4
>
>   In this case, the parser was considering "1" to be the left-side of
>   the expression, and "(2 + 3) + 4" to be the right-side.  The patch
>   fixes the parser by making it identify whether a parenthesized
>   sub-expression has just been parsed, and act accordingly.
>
> I've tested this on my Debian testing amd64, and everything seems OK.
>
> gdb/ChangeLog:
> 2021-01-03  Sergio Durigan Junior  <sergiodj@sergiodj.net>
> 	    Tom Tromey <tom@tromey.com>
>
> 	* stap-probe.c (stap_parse_single_operand): Handle '!'
> 	operator.
> 	(stap_parse_argument_conditionally): Likewise.
> 	Skip spaces after processing open-parenthesis sub-expression.
> 	(stap_parse_argument_1): Skip spaces after call to
> 	stap_parse_argument_conditionally.
> 	Handle case when right-side expression is a parenthesized
> 	sub-expression.
> 	Skip spaces after call to stap_parse_argument_1.
>
> gdb/testsuite/ChangeLog:
> 2021-01-03  Sergio Durigan Junior  <sergiodj@sergiodj.net>
>
> 	* gdb.arch/amd64-stap-expressions.S: New file.
> 	* gdb.arch/amd64-stap-expressions.exp: New file.
> ---
>  gdb/stap-probe.c                              | 31 +++++++--
>  .../gdb.arch/amd64-stap-expressions.S         | 43 ++++++++++++
>  .../gdb.arch/amd64-stap-expressions.exp       | 68 +++++++++++++++++++
>  3 files changed, 137 insertions(+), 5 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-stap-expressions.S
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-stap-expressions.exp
>
> diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
> index c2ddd047f6..224dd5714f 100644
> --- a/gdb/stap-probe.c
> +++ b/gdb/stap-probe.c
> @@ -870,7 +870,7 @@ stap_parse_single_operand (struct stap_parse_info *p)
>        return;
>      }
>  
> -  if (*p->arg == '-' || *p->arg == '~' || *p->arg == '+')
> +  if (*p->arg == '-' || *p->arg == '~' || *p->arg == '+' || *p->arg == '!')
>      {
>        char c = *p->arg;
>        /* We use this variable to do a lookahead.  */
> @@ -924,6 +924,8 @@ stap_parse_single_operand (struct stap_parse_info *p)
>  	    write_exp_elt_opcode (&p->pstate, UNOP_NEG);
>  	  else if (c == '~')
>  	    write_exp_elt_opcode (&p->pstate, UNOP_COMPLEMENT);
> +	  else if (c == '!')
> +	    write_exp_elt_opcode (&p->pstate, UNOP_LOGICAL_NOT);
>  	}
>      }
>    else if (isdigit (*p->arg))
> @@ -1012,7 +1014,7 @@ stap_parse_argument_conditionally (struct stap_parse_info *p)
>  {
>    gdb_assert (gdbarch_stap_is_single_operand_p (p->gdbarch));
>  
> -  if (*p->arg == '-' || *p->arg == '~' || *p->arg == '+' /* Unary.  */
> +  if (*p->arg == '-' || *p->arg == '~' || *p->arg == '+' || *p->arg == '!'
>        || isdigit (*p->arg)
>        || gdbarch_stap_is_single_operand (p->gdbarch, p->arg))
>      stap_parse_single_operand (p);
> @@ -1027,11 +1029,12 @@ stap_parse_argument_conditionally (struct stap_parse_info *p)
>  
>        stap_parse_argument_1 (p, 0, STAP_OPERAND_PREC_NONE);
>  
> -      --p->inside_paren_p;
> +      p->arg = skip_spaces (p->arg);
>        if (*p->arg != ')')
> -	error (_("Missign close-paren on expression `%s'."),
> +	error (_("Missign close-parenthesis on expression `%s'."),
>  	       p->saved_arg);
>  
> +      --p->inside_paren_p;
>        ++p->arg;
>        if (p->inside_paren_p)
>  	p->arg = skip_spaces (p->arg);
> @@ -1067,6 +1070,9 @@ stap_parse_argument_1 (struct stap_parse_info *p, bool has_lhs,
>        stap_parse_argument_conditionally (p);
>      }
>  
> +  if (p->inside_paren_p)
> +    p->arg = skip_spaces (p->arg);
> +
>    /* Start to parse the right-side, and to "join" left and right sides
>       depending on the operation specified.
>  
> @@ -1104,8 +1110,21 @@ stap_parse_argument_1 (struct stap_parse_info *p, bool has_lhs,
>        if (p->inside_paren_p)
>  	p->arg = skip_spaces (p->arg);
>  
> -      /* Parse the right-side of the expression.  */
> +      /* Parse the right-side of the expression.
> +
> +	 We save whether the right-side is a parenthesized
> +	 subexpression because, if it is, we will have to finish
> +	 processing this part of the expression before continuing.  */
> +      bool paren_subexp = *p->arg == '(';
> +
>        stap_parse_argument_conditionally (p);
> +      if (p->inside_paren_p)
> +	p->arg = skip_spaces (p->arg);
> +      if (paren_subexp)
> +	{
> +	  write_exp_elt_opcode (&p->pstate, opcode);
> +	  continue;
> +	}
>  
>        /* While we still have operators, try to parse another
>  	 right-side, but using the current right-side as a left-side.  */
> @@ -1130,6 +1149,8 @@ stap_parse_argument_1 (struct stap_parse_info *p, bool has_lhs,
>  	  /* Parse the right-side of the expression, but since we already
>  	     have a left-side at this point, set `has_lhs' to 1.  */
>  	  stap_parse_argument_1 (p, 1, lookahead_prec);
> +	  if (p->inside_paren_p)
> +	    p->arg = skip_spaces (p->arg);
>  	}
>  
>        write_exp_elt_opcode (&p->pstate, opcode);
> diff --git a/gdb/testsuite/gdb.arch/amd64-stap-expressions.S b/gdb/testsuite/gdb.arch/amd64-stap-expressions.S
> new file mode 100644
> index 0000000000..76a47aa9b5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-stap-expressions.S
> @@ -0,0 +1,43 @@
> +/* Copyright (C) 2021 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   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 <sys/sdt.h>
> +
> +	.file	"amd64-stap-expressions.S"
> +	.text
> +	.globl	main
> +main:
> +	/* We use a nop here because we don't want the first probe to
> +	   be placed at the same location as the main label.  */
> +	nop
> +
> +	/* Single operands.  */
> +	STAP_PROBE1(probe, log_neg, 8@!($0+$1))
> +	STAP_PROBE1(probe, minus, -8@-($3+$4))
> +	STAP_PROBE1(probe, bit_neg, -8@~$22)
> +
> +	/* Arithmetic expressions.  */
> +	STAP_PROBE1(probe, plus1, 8@$3+($10-$8)-$1)
> +	STAP_PROBE1(probe, plus2, 8@$100-(  ($8+$10) -$50)+$3)
> +	STAP_PROBE1(probe, plus3, 8@$100-(($8+$10)-$50)+((($8 - $9) + $40) - $4)+$4)
> +
> +	/* Bitwise expressions.  */
> +	STAP_PROBE1(probe, and, 8@$128&$128)
> +	STAP_PROBE1(probe, or, 8@$8|$4)
> +
> +	xor	%rax,%rax
> +	ret
> diff --git a/gdb/testsuite/gdb.arch/amd64-stap-expressions.exp b/gdb/testsuite/gdb.arch/amd64-stap-expressions.exp
> new file mode 100644
> index 0000000000..5e3cb60a9e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-stap-expressions.exp
> @@ -0,0 +1,68 @@
> +# Copyright 2021 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/>.
> +
> +standard_testfile ".S"
> +
> +if { ![istarget "x86_64-*-*"] || ![is_lp64_target] } {
> +    verbose "Skipping $testfile.exp"
> +    return
> +}
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
> +    return -1
> +}
> +
> +# Helper procedure to go to probe NAME
> +
> +proc goto_probe { name } {
> +    global decimal hex
> +
> +    gdb_test "break -pstap $name" "Breakpoint $decimal at $hex"
> +    gdb_test "continue" "Breakpoint $decimal, main \\(\\) at .*\r\n.*STAP_PROBE1.*${name},.*\\)"
> +}
> +
> +# Helper procedure to test the probe's argument
> +
> +proc test_probe_value { value } {
> +    gdb_test "print \$_probe_argc" "= 1"
> +    gdb_test "print \$_probe_arg0" "= $value"
> +}
> +
> +if { ![runto_main] } {
> +    return -1
> +}
> +
> +# Name and expected value for each probe.
> +set probe_names_and_values {
> +    { "log_neg" "0" }
> +    { "minus" "-7" }
> +    { "bit_neg" "-23" }
> +
> +    { "plus1" "4" }
> +    { "plus2" "135" }
> +    { "plus3" "171" }
> +
> +    { "and" "128" }
> +    { "or" "12" }
> +}
> +
> +foreach probe_info $probe_names_and_values {
> +    set name [lindex $probe_info 0]
> +    set value [lindex $probe_info 1]
> +    with_test_prefix $name {
> +	goto_probe $name
> +	test_probe_value $value
> +    }
> +}
> -- 
>
> 2.29.2
>
>

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

Re: [PATCH] Change handling of '!' operator in stap probes

[Sergio Durigan Junior]

On Tuesday, January 12 2021, Sergio Durigan Junior via Gdb-patches wrote:

> On Sunday, January 03 2020, Sergio Durigan Junior via Gdb-patches wrote:
>
>> On Thursday, December 31 2020, Tom Tromey wrote:
>>
>>> While working on the expression rewrite, I noticed that the '!'
>>> operator is handled as if it were a binary operator.  However, this
>>> seems incorrect.  Instead, I think it should be handled alongside the
>>> other unary operators in stap_parse_single_operand.
>>>
>>> This patch makes this change.
>
> Ping.

Ping^2.

>>
>> Thanks for the patch, Tom.
>>
>> This code was written based on the information contained here:
>>
>>   https://sourceware.org/binutils/docs/as/Infix-Ops.html
>>
>> You can see that it says:
>>
>>   !
>>     Bitwise Or Not
>>
>> The problem is that, when we examine gas' source code, we see that it
>> doesn't treat '!' as "bitwise or not" (fortunately!), but as the
>> "logical not":
>>
>>   https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gas/symbols.c;h=f6e4b718ef719eef8a7e6e29dbadf3605058403d;hb=HEAD#l1128
>>
>> OK, this means that our stap-probe.c is not completely wrong: the enum
>> should indeed be named UNOP_LOGICAL_NOT.
>>
>> Now, when we check to see if the precedence assigned to UNOP_LOGICAL_NOT
>> is the right one, we see that it is:
>>
>>   https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gas/expr.c;h=1b420d928cde50456af0137529b8215294ba521d;hb=HEAD#l1481
>>
>>
>>   /* Rank	Examples
>>      0	operand, (expression)
>>      1	||
>>      2	&&
>>      3	== <> < <= >= >
>>      4	+ -
>>      5	used for * / % in MRI mode
>>      6	& ^ ! |
>>   ...
>>
>> Huh, interesting.  Anyway, this whole investigation (plus the fact that,
>> as you mentioned, we don't have tests to exercise this part of the code)
>> made me uncover a few latent bugs on the stap parser code, so I'm
>> sending a patch that incorporates part of your changes and also fixes
>> the issues I found.  As a bonus, I'm proposing a new testcase
>> (amd64-only) to make sure that we expand the coverage of this code.
>>
>> WDYT?
>>
>> -- 
>> Sergio
>> GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
>> Please send encrypted e-mail if possible
>> https://sergiodj.net/
>>
>> From da02db7cacef62ce085d5715b1339b24d8f2260c Mon Sep 17 00:00:00 2001
>> From: Sergio Durigan Junior <sergiodj@sergiodj.net>
>> Date: Sun, 3 Jan 2021 02:42:52 -0500
>> Subject: [PATCH] Fix a few stap parser issues and add a new test for probe
>>  expressions
>>
>> The creation of this patch was motivated by Tom's "Change handling of
>> '!' operator in stap probes" patch.
>>
>> While reviewing his patch, I stumbled upon a few issues with the stap
>> expression parser.  They are:
>>
>> - As it turns out, even with Tom's patch applied the parser doesn't
>>   properly handle the '!' operator.  The underlying issue was the fact
>>   that stap_parse_argument_conditionally also needed to be patched in
>>   order to recognize '!' as an operator that is part of a single
>>   operand, and parse it accordingly.
>>
>> - While writing the testcase I'm proposing on this patch, I found that
>>   parenthesized sub-expressions were not being parsed correctly when
>>   there was another term after them.  For example:
>>
>>     1 - (2 + 3) + 4
>>
>>   In this case, the parser was considering "1" to be the left-side of
>>   the expression, and "(2 + 3) + 4" to be the right-side.  The patch
>>   fixes the parser by making it identify whether a parenthesized
>>   sub-expression has just been parsed, and act accordingly.
>>
>> I've tested this on my Debian testing amd64, and everything seems OK.
>>
>> gdb/ChangeLog:
>> 2021-01-03  Sergio Durigan Junior  <sergiodj@sergiodj.net>
>> 	    Tom Tromey <tom@tromey.com>
>>
>> 	* stap-probe.c (stap_parse_single_operand): Handle '!'
>> 	operator.
>> 	(stap_parse_argument_conditionally): Likewise.
>> 	Skip spaces after processing open-parenthesis sub-expression.
>> 	(stap_parse_argument_1): Skip spaces after call to
>> 	stap_parse_argument_conditionally.
>> 	Handle case when right-side expression is a parenthesized
>> 	sub-expression.
>> 	Skip spaces after call to stap_parse_argument_1.
>>
>> gdb/testsuite/ChangeLog:
>> 2021-01-03  Sergio Durigan Junior  <sergiodj@sergiodj.net>
>>
>> 	* gdb.arch/amd64-stap-expressions.S: New file.
>> 	* gdb.arch/amd64-stap-expressions.exp: New file.
>> ---
>>  gdb/stap-probe.c                              | 31 +++++++--
>>  .../gdb.arch/amd64-stap-expressions.S         | 43 ++++++++++++
>>  .../gdb.arch/amd64-stap-expressions.exp       | 68 +++++++++++++++++++
>>  3 files changed, 137 insertions(+), 5 deletions(-)
>>  create mode 100644 gdb/testsuite/gdb.arch/amd64-stap-expressions.S
>>  create mode 100644 gdb/testsuite/gdb.arch/amd64-stap-expressions.exp
>>
>> diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
>> index c2ddd047f6..224dd5714f 100644
>> --- a/gdb/stap-probe.c
>> +++ b/gdb/stap-probe.c
>> @@ -870,7 +870,7 @@ stap_parse_single_operand (struct stap_parse_info *p)
>>        return;
>>      }
>>  
>> -  if (*p->arg == '-' || *p->arg == '~' || *p->arg == '+')
>> +  if (*p->arg == '-' || *p->arg == '~' || *p->arg == '+' || *p->arg == '!')
>>      {
>>        char c = *p->arg;
>>        /* We use this variable to do a lookahead.  */
>> @@ -924,6 +924,8 @@ stap_parse_single_operand (struct stap_parse_info *p)
>>  	    write_exp_elt_opcode (&p->pstate, UNOP_NEG);
>>  	  else if (c == '~')
>>  	    write_exp_elt_opcode (&p->pstate, UNOP_COMPLEMENT);
>> +	  else if (c == '!')
>> +	    write_exp_elt_opcode (&p->pstate, UNOP_LOGICAL_NOT);
>>  	}
>>      }
>>    else if (isdigit (*p->arg))
>> @@ -1012,7 +1014,7 @@ stap_parse_argument_conditionally (struct stap_parse_info *p)
>>  {
>>    gdb_assert (gdbarch_stap_is_single_operand_p (p->gdbarch));
>>  
>> -  if (*p->arg == '-' || *p->arg == '~' || *p->arg == '+' /* Unary.  */
>> +  if (*p->arg == '-' || *p->arg == '~' || *p->arg == '+' || *p->arg == '!'
>>        || isdigit (*p->arg)
>>        || gdbarch_stap_is_single_operand (p->gdbarch, p->arg))
>>      stap_parse_single_operand (p);
>> @@ -1027,11 +1029,12 @@ stap_parse_argument_conditionally (struct stap_parse_info *p)
>>  
>>        stap_parse_argument_1 (p, 0, STAP_OPERAND_PREC_NONE);
>>  
>> -      --p->inside_paren_p;
>> +      p->arg = skip_spaces (p->arg);
>>        if (*p->arg != ')')
>> -	error (_("Missign close-paren on expression `%s'."),
>> +	error (_("Missign close-parenthesis on expression `%s'."),
>>  	       p->saved_arg);
>>  
>> +      --p->inside_paren_p;
>>        ++p->arg;
>>        if (p->inside_paren_p)
>>  	p->arg = skip_spaces (p->arg);
>> @@ -1067,6 +1070,9 @@ stap_parse_argument_1 (struct stap_parse_info *p, bool has_lhs,
>>        stap_parse_argument_conditionally (p);
>>      }
>>  
>> +  if (p->inside_paren_p)
>> +    p->arg = skip_spaces (p->arg);
>> +
>>    /* Start to parse the right-side, and to "join" left and right sides
>>       depending on the operation specified.
>>  
>> @@ -1104,8 +1110,21 @@ stap_parse_argument_1 (struct stap_parse_info *p, bool has_lhs,
>>        if (p->inside_paren_p)
>>  	p->arg = skip_spaces (p->arg);
>>  
>> -      /* Parse the right-side of the expression.  */
>> +      /* Parse the right-side of the expression.
>> +
>> +	 We save whether the right-side is a parenthesized
>> +	 subexpression because, if it is, we will have to finish
>> +	 processing this part of the expression before continuing.  */
>> +      bool paren_subexp = *p->arg == '(';
>> +
>>        stap_parse_argument_conditionally (p);
>> +      if (p->inside_paren_p)
>> +	p->arg = skip_spaces (p->arg);
>> +      if (paren_subexp)
>> +	{
>> +	  write_exp_elt_opcode (&p->pstate, opcode);
>> +	  continue;
>> +	}
>>  
>>        /* While we still have operators, try to parse another
>>  	 right-side, but using the current right-side as a left-side.  */
>> @@ -1130,6 +1149,8 @@ stap_parse_argument_1 (struct stap_parse_info *p, bool has_lhs,
>>  	  /* Parse the right-side of the expression, but since we already
>>  	     have a left-side at this point, set `has_lhs' to 1.  */
>>  	  stap_parse_argument_1 (p, 1, lookahead_prec);
>> +	  if (p->inside_paren_p)
>> +	    p->arg = skip_spaces (p->arg);
>>  	}
>>  
>>        write_exp_elt_opcode (&p->pstate, opcode);
>> diff --git a/gdb/testsuite/gdb.arch/amd64-stap-expressions.S b/gdb/testsuite/gdb.arch/amd64-stap-expressions.S
>> new file mode 100644
>> index 0000000000..76a47aa9b5
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/amd64-stap-expressions.S
>> @@ -0,0 +1,43 @@
>> +/* Copyright (C) 2021 Free Software Foundation, Inc.
>> +
>> +   This file is part of GDB.
>> +
>> +   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 <sys/sdt.h>
>> +
>> +	.file	"amd64-stap-expressions.S"
>> +	.text
>> +	.globl	main
>> +main:
>> +	/* We use a nop here because we don't want the first probe to
>> +	   be placed at the same location as the main label.  */
>> +	nop
>> +
>> +	/* Single operands.  */
>> +	STAP_PROBE1(probe, log_neg, 8@!($0+$1))
>> +	STAP_PROBE1(probe, minus, -8@-($3+$4))
>> +	STAP_PROBE1(probe, bit_neg, -8@~$22)
>> +
>> +	/* Arithmetic expressions.  */
>> +	STAP_PROBE1(probe, plus1, 8@$3+($10-$8)-$1)
>> +	STAP_PROBE1(probe, plus2, 8@$100-(  ($8+$10) -$50)+$3)
>> +	STAP_PROBE1(probe, plus3, 8@$100-(($8+$10)-$50)+((($8 - $9) + $40) - $4)+$4)
>> +
>> +	/* Bitwise expressions.  */
>> +	STAP_PROBE1(probe, and, 8@$128&$128)
>> +	STAP_PROBE1(probe, or, 8@$8|$4)
>> +
>> +	xor	%rax,%rax
>> +	ret
>> diff --git a/gdb/testsuite/gdb.arch/amd64-stap-expressions.exp b/gdb/testsuite/gdb.arch/amd64-stap-expressions.exp
>> new file mode 100644
>> index 0000000000..5e3cb60a9e
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/amd64-stap-expressions.exp
>> @@ -0,0 +1,68 @@
>> +# Copyright 2021 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/>.
>> +
>> +standard_testfile ".S"
>> +
>> +if { ![istarget "x86_64-*-*"] || ![is_lp64_target] } {
>> +    verbose "Skipping $testfile.exp"
>> +    return
>> +}
>> +
>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
>> +    return -1
>> +}
>> +
>> +# Helper procedure to go to probe NAME
>> +
>> +proc goto_probe { name } {
>> +    global decimal hex
>> +
>> +    gdb_test "break -pstap $name" "Breakpoint $decimal at $hex"
>> +    gdb_test "continue" "Breakpoint $decimal, main \\(\\) at .*\r\n.*STAP_PROBE1.*${name},.*\\)"
>> +}
>> +
>> +# Helper procedure to test the probe's argument
>> +
>> +proc test_probe_value { value } {
>> +    gdb_test "print \$_probe_argc" "= 1"
>> +    gdb_test "print \$_probe_arg0" "= $value"
>> +}
>> +
>> +if { ![runto_main] } {
>> +    return -1
>> +}
>> +
>> +# Name and expected value for each probe.
>> +set probe_names_and_values {
>> +    { "log_neg" "0" }
>> +    { "minus" "-7" }
>> +    { "bit_neg" "-23" }
>> +
>> +    { "plus1" "4" }
>> +    { "plus2" "135" }
>> +    { "plus3" "171" }
>> +
>> +    { "and" "128" }
>> +    { "or" "12" }
>> +}
>> +
>> +foreach probe_info $probe_names_and_values {
>> +    set name [lindex $probe_info 0]
>> +    set value [lindex $probe_info 1]
>> +    with_test_prefix $name {
>> +	goto_probe $name
>> +	test_probe_value $value
>> +    }
>> +}
>> -- 
>>
>> 2.29.2
>>
>>
>
> -- 
> Sergio
> GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
> Please send encrypted e-mail if possible
> https://sergiodj.net/

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

Re: [PATCH] Change handling of '!' operator in stap probes

[Tom Tromey]

Sergio> gdb/ChangeLog:
Sergio> 2021-01-03  Sergio Durigan Junior  <sergiodj@sergiodj.net>
Sergio> 	    Tom Tromey <tom@tromey.com>

Sergio> 	* stap-probe.c (stap_parse_single_operand): Handle '!'
Sergio> 	operator.
Sergio> 	(stap_parse_argument_conditionally): Likewise.
Sergio> 	Skip spaces after processing open-parenthesis sub-expression.
Sergio> 	(stap_parse_argument_1): Skip spaces after call to
Sergio> 	stap_parse_argument_conditionally.
Sergio> 	Handle case when right-side expression is a parenthesized
Sergio> 	sub-expression.
Sergio> 	Skip spaces after call to stap_parse_argument_1.

Sergio> gdb/testsuite/ChangeLog:
Sergio> 2021-01-03  Sergio Durigan Junior  <sergiodj@sergiodj.net>

Sergio> 	* gdb.arch/amd64-stap-expressions.S: New file.
Sergio> 	* gdb.arch/amd64-stap-expressions.exp: New file.

Looks good.  Thank you for doing this.

Tom

Re: [PATCH] Change handling of '!' operator in stap probes

[Sergio Durigan Junior]

On Wednesday, January 20 2021, Tom Tromey wrote:

> Sergio> gdb/ChangeLog:
> Sergio> 2021-01-03  Sergio Durigan Junior  <sergiodj@sergiodj.net>
> Sergio> 	    Tom Tromey <tom@tromey.com>
>
> Sergio> 	* stap-probe.c (stap_parse_single_operand): Handle '!'
> Sergio> 	operator.
> Sergio> 	(stap_parse_argument_conditionally): Likewise.
> Sergio> 	Skip spaces after processing open-parenthesis sub-expression.
> Sergio> 	(stap_parse_argument_1): Skip spaces after call to
> Sergio> 	stap_parse_argument_conditionally.
> Sergio> 	Handle case when right-side expression is a parenthesized
> Sergio> 	sub-expression.
> Sergio> 	Skip spaces after call to stap_parse_argument_1.
>
> Sergio> gdb/testsuite/ChangeLog:
> Sergio> 2021-01-03  Sergio Durigan Junior  <sergiodj@sergiodj.net>
>
> Sergio> 	* gdb.arch/amd64-stap-expressions.S: New file.
> Sergio> 	* gdb.arch/amd64-stap-expressions.exp: New file.
>
> Looks good.  Thank you for doing this.

Thanks for the review, Tom.

Pushed: 6f52fdf40465ee8f5421c66ceb21f37856bf6e5e

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