[RFC] Fix build failure in stap-probe.c.

[RFC] Fix build failure in stap-probe.c.

[Doug Evans]

Hi.

I'm getting build failures, gcc is complaining that "opcode" and
"lookahead_opcode" "may be used uninitialized".

cc1: warnings being treated as errors
../../src/gdb/stap-probe.c: In function 'stap_parse_argument_1':
../../src/gdb/stap-probe.c:1558: error: 'lookahead_opcode' may be used uninitialized in this function
../../src/gdb/stap-probe.c:813: note: 'lookahead_opcode' was declared here
../../src/gdb/stap-probe.c:1558: error: 'opcode' may be used uninitialized in this function
../../src/gdb/stap-probe.c:778: note: 'opcode' was declared here
make: *** [stap-probe.o] Error 1

This patch is just RFC.
IIUC the code already watches for valid operators before
calling stap_get_opcode, so stap_get_opcode should "never" return zero.
So I'm wondering if maybe step_get_opcode should be changed
to always succeed and always set the opcode.

Can one of you look at this?


2012-05-02  Doug Evans  <dje@google.com>

	* stap-probe.c (stap_parse_argument_1): Fix "may be used uninitialized"
	build failures.

Index: stap-probe.c
===================================================================
RCS file: /cvs/src/src/gdb/stap-probe.c,v
retrieving revision 1.1
diff -u -p -r1.1 stap-probe.c
--- stap-probe.c	27 Apr 2012 20:47:56 -0000	1.1
+++ stap-probe.c	2 May 2012 08:02:24 -0000
@@ -775,7 +775,8 @@ stap_parse_argument_1 (struct stap_parse
   while (p->arg && *p->arg && *p->arg != ')' && !isspace (*p->arg))
     {
       const char *tmp_exp_buf;
-      enum exp_opcode opcode;
+      /* Initialize to pacify gcc.  */
+      enum exp_opcode opcode = OP_LONG;
       enum stap_operand_prec cur_prec;
 
       if (!stap_is_operator (*p->arg))
@@ -810,7 +811,8 @@ stap_parse_argument_1 (struct stap_parse
 	 right-side, but using the current right-side as a left-side.  */
       while (*p->arg && stap_is_operator (*p->arg))
 	{
-	  enum exp_opcode lookahead_opcode;
+	  /* Initialize to pacify gcc.  */
+	  enum exp_opcode lookahead_opcode = OP_LONG;
 	  enum stap_operand_prec lookahead_prec;
 
 	  /* Saving the current expression buffer position.  The explanation

Re: [RFC] Fix build failure in stap-probe.c.

[Sergio Durigan Junior]

On Wednesday, May 02 2012, Doug Evans wrote:

> Hi.
>
> I'm getting build failures, gcc is complaining that "opcode" and
> "lookahead_opcode" "may be used uninitialized".

Hi Doug,

I'm not seeing this error when I compile (even with -O2), but I believe
you :-).

> cc1: warnings being treated as errors
> ../../src/gdb/stap-probe.c: In function 'stap_parse_argument_1':
> ../../src/gdb/stap-probe.c:1558: error: 'lookahead_opcode' may be used uninitialized in this function
> ../../src/gdb/stap-probe.c:813: note: 'lookahead_opcode' was declared here
> ../../src/gdb/stap-probe.c:1558: error: 'opcode' may be used uninitialized in this function
> ../../src/gdb/stap-probe.c:778: note: 'opcode' was declared here
> make: *** [stap-probe.o] Error 1
>
> This patch is just RFC.
> IIUC the code already watches for valid operators before
> calling stap_get_opcode, so stap_get_opcode should "never" return zero.
> So I'm wondering if maybe step_get_opcode should be changed
> to always succeed and always set the opcode.

You're right.  Does this patch work for you?

2012-05-02  Sergio Durigan Junior  <sergiodj@redhat.com>

	* stap-probe.c (stap_is_operator): Change declaration.
	(stap_get_opcode): Change return value.
	(stap_parse_argument_1): Update calls to `stap_get_opcode' and
	`stap_parse_argument_1'.

---
 gdb/stap-probe.c |  101 ++++++++++++++++++++++++++++++++---------------------
 1 files changed, 61 insertions(+), 40 deletions(-)

diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index d1a38da..273ae07 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -154,7 +154,7 @@ static void stap_parse_argument_conditionally (struct stap_parse_info *p);
 
 /* Returns 1 if *S is an operator, zero otherwise.  */
 
-static int stap_is_operator (char op);
+static int stap_is_operator (const char *op);
 
 static void
 show_stapexpressiondebug (struct ui_file *file, int from_tty,
@@ -209,111 +209,107 @@ stap_get_operator_prec (enum exp_opcode op)
 /* Given S, read the operator in it and fills the OP pointer with its code.
    Return 1 on success, zero if the operator was not recognized.  */
 
-static int
-stap_get_opcode (const char **s, enum exp_opcode *op)
+static enum exp_opcode
+stap_get_opcode (const char **s)
 {
   const char c = **s;
-  int ret = 1;
+  enum exp_opcode op;
 
   *s += 1;
 
   switch (c)
     {
     case '*':
-      *op = BINOP_MUL;
+      op = BINOP_MUL;
       break;
 
     case '/':
-      *op = BINOP_DIV;
+      op = BINOP_DIV;
       break;
 
     case '%':
-      *op = BINOP_REM;
+      op = BINOP_REM;
     break;
 
     case '<':
-      *op = BINOP_LESS;
+      op = BINOP_LESS;
       if (**s == '<')
 	{
 	  *s += 1;
-	  *op = BINOP_LSH;
+	  op = BINOP_LSH;
 	}
       else if (**s == '=')
 	{
 	  *s += 1;
-	  *op = BINOP_LEQ;
+	  op = BINOP_LEQ;
 	}
       else if (**s == '>')
 	{
 	  *s += 1;
-	  *op = BINOP_NOTEQUAL;
+	  op = BINOP_NOTEQUAL;
 	}
     break;
 
     case '>':
-      *op = BINOP_GTR;
+      op = BINOP_GTR;
       if (**s == '>')
 	{
 	  *s += 1;
-	  *op = BINOP_RSH;
+	  op = BINOP_RSH;
 	}
       else if (**s == '=')
 	{
 	  *s += 1;
-	  *op = BINOP_GEQ;
+	  op = BINOP_GEQ;
 	}
     break;
 
     case '|':
-      *op = BINOP_BITWISE_IOR;
+      op = BINOP_BITWISE_IOR;
       if (**s == '|')
 	{
 	  *s += 1;
-	  *op = BINOP_LOGICAL_OR;
+	  op = BINOP_LOGICAL_OR;
 	}
     break;
 
     case '&':
-      *op = BINOP_BITWISE_AND;
+      op = BINOP_BITWISE_AND;
       if (**s == '&')
 	{
 	  *s += 1;
-	  *op = BINOP_LOGICAL_AND;
+	  op = BINOP_LOGICAL_AND;
 	}
     break;
 
     case '^':
-      *op = BINOP_BITWISE_XOR;
+      op = BINOP_BITWISE_XOR;
       break;
 
     case '!':
-      *op = UNOP_LOGICAL_NOT;
+      op = UNOP_LOGICAL_NOT;
       break;
 
     case '+':
-      *op = BINOP_ADD;
+      op = BINOP_ADD;
       break;
 
     case '-':
-      *op = BINOP_SUB;
+      op = BINOP_SUB;
       break;
 
     case '=':
-      if (**s != '=')
-	{
-	  ret = 0;
-	  break;
-	}
-      *op = BINOP_EQUAL;
+      gdb_assert (**s == '=');
+      op = BINOP_EQUAL;
       break;
 
     default:
-      /* We didn't find any operator.  */
-      *s -= 1;
-      return 0;
+      internal_error (__FILE__, __LINE__,
+		      _("Invalid opcode in expression `%s' for SystemTap"
+			"probe"), *s);
     }
 
-  return ret;
+  return op;
 }
 
 /* Given the bitness of the argument, represented by B, return the
@@ -773,7 +769,7 @@ stap_parse_argument_1 (struct stap_parse_info *p, int has_lhs,
       enum exp_opcode opcode;
       enum stap_operand_prec cur_prec;
 
-      if (!stap_is_operator (*p->arg))
+      if (!stap_is_operator (p->arg))
 	error (_("Invalid operator `%c' on expression `%s'."), *p->arg,
 	       p->saved_arg);
 
@@ -782,7 +778,7 @@ stap_parse_argument_1 (struct stap_parse_info *p, int has_lhs,
 	 operator.  If this operator's precedence is lower than PREC, we
 	 should return and not advance the expression buffer pointer.  */
       tmp_exp_buf = p->arg;
-      stap_get_opcode (&tmp_exp_buf, &opcode);
+      opcode = stap_get_opcode (&tmp_exp_buf);
 
       cur_prec = stap_get_operator_prec (opcode);
       if (cur_prec < prec)
@@ -803,7 +799,7 @@ stap_parse_argument_1 (struct stap_parse_info *p, int has_lhs,
 
       /* While we still have operators, try to parse another
 	 right-side, but using the current right-side as a left-side.  */
-      while (*p->arg && stap_is_operator (*p->arg))
+      while (*p->arg && stap_is_operator (p->arg))
 	{
 	  enum exp_opcode lookahead_opcode;
 	  enum stap_operand_prec lookahead_prec;
@@ -811,7 +807,7 @@ stap_parse_argument_1 (struct stap_parse_info *p, int has_lhs,
 	  /* Saving the current expression buffer position.  The explanation
 	     is the same as above.  */
 	  tmp_exp_buf = p->arg;
-	  stap_get_opcode (&tmp_exp_buf, &lookahead_opcode);
+	  lookahead_opcode = stap_get_opcode (&tmp_exp_buf);
 	  lookahead_prec = stap_get_operator_prec (lookahead_opcode);
 
 	  if (lookahead_prec <= prec)
@@ -1013,11 +1009,36 @@ stap_get_probe_argument_count (struct probe *probe_generic,
    otherwise.  */
 
 static int
-stap_is_operator (char op)
+stap_is_operator (const char *op)
 {
-  return (op == '+' || op == '-' || op == '*' || op == '/'
-	  || op == '>' || op == '<' || op == '!' || op == '^'
-	  || op == '|' || op == '&' || op == '%' || op == '=');
+  int ret = 1;
+
+  switch (*op)
+    {
+    case '*':
+    case '/':
+    case '%':
+    case '^':
+    case '!':
+    case '+':
+    case '-':
+    case '<':
+    case '>':
+    case '|':
+    case '&':
+      break;
+
+    case '=':
+      if (op[1] != '=')
+	ret = 0;
+      break;
+
+    default:
+      /* We didn't find any operator.  */
+      ret = 0;
+    }
+
+  return ret;
 }
 
 static struct stap_probe_arg *
-- 
1.7.7.6

-- 
Sergio

Re: [RFC] Fix build failure in stap-probe.c.

[Doug Evans]

On Wed, May 2, 2012 at 11:56 AM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> On Wednesday, May 02 2012, Doug Evans wrote:
>
>> Hi.
>>
>> I'm getting build failures, gcc is complaining that "opcode" and
>> "lookahead_opcode" "may be used uninitialized".
>
> Hi Doug,
>
> I'm not seeing this error when I compile (even with -O2), but I believe
> you :-).
>
>> cc1: warnings being treated as errors
>> ../../src/gdb/stap-probe.c: In function 'stap_parse_argument_1':
>> ../../src/gdb/stap-probe.c:1558: error: 'lookahead_opcode' may be used uninitialized in this function
>> ../../src/gdb/stap-probe.c:813: note: 'lookahead_opcode' was declared here
>> ../../src/gdb/stap-probe.c:1558: error: 'opcode' may be used uninitialized in this function
>> ../../src/gdb/stap-probe.c:778: note: 'opcode' was declared here
>> make: *** [stap-probe.o] Error 1
>>
>> This patch is just RFC.
>> IIUC the code already watches for valid operators before
>> calling stap_get_opcode, so stap_get_opcode should "never" return zero.
>> So I'm wondering if maybe step_get_opcode should be changed
>> to always succeed and always set the opcode.
>
> You're right.  Does this patch work for you?
>
> 2012-05-02  Sergio Durigan Junior  <sergiodj@redhat.com>
>
>        * stap-probe.c (stap_is_operator): Change declaration.
>        (stap_get_opcode): Change return value.
>        (stap_parse_argument_1): Update calls to `stap_get_opcode' and
>        `stap_parse_argument_1'.

Works great, thanks!

Re: [RFC] Fix build failure in stap-probe.c.

[Sergio Durigan Junior]

On Thursday, May 03 2012, Doug Evans wrote:

> On Wed, May 2, 2012 at 11:56 AM, Sergio Durigan Junior
> <sergiodj@redhat.com> wrote:

>> You're right.  Does this patch work for you?
>>
>> 2012-05-02  Sergio Durigan Junior  <sergiodj@redhat.com>
>>
>>        * stap-probe.c (stap_is_operator): Change declaration.
>>        (stap_get_opcode): Change return value.
>>        (stap_parse_argument_1): Update calls to `stap_get_opcode' and
>>        `stap_parse_argument_1'.
>
> Works great, thanks!

Thanks, checked in:

    http://sourceware.org/ml/gdb-cvs/2012-05/msg00024.html

-- 
Sergio

Re: [RFC] Fix build failure in stap-probe.c.

[Doug Evans]

On Thu, May 3, 2012 at 1:04 PM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> On Thursday, May 03 2012, Doug Evans wrote:
>
>> On Wed, May 2, 2012 at 11:56 AM, Sergio Durigan Junior
>> <sergiodj@redhat.com> wrote:
>
>>> You're right.  Does this patch work for you?
>>>
>>> 2012-05-02  Sergio Durigan Junior  <sergiodj@redhat.com>
>>>
>>>        * stap-probe.c (stap_is_operator): Change declaration.
>>>        (stap_get_opcode): Change return value.
>>>        (stap_parse_argument_1): Update calls to `stap_get_opcode' and
>>>        `stap_parse_argument_1'.
>>
>> Works great, thanks!
>
> Thanks, checked in:
>
>    http://sourceware.org/ml/gdb-cvs/2012-05/msg00024.html

Awesome.  Thanks.