[PATCH] ada-lang, possible_user_operator_p, null pointer

[PATCH] ada-lang, possible_user_operator_p, null pointer

[msnyder]

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

If type1 is null (which is checked for earlier), this line will
dereference it.

Also, I'm not sure what the CONCAT operator is, but is it
possible that the OR at the beginning of the same line
should be an AND?  Just checking...


[-- Attachment #2: 56.txt --]
[-- Type: text/plain, Size: 1254 bytes --]

2007-08-11  Michael Snyder  <msnyder@access-company.com>

	* ada-lang.c (possible_user_operator_p): Guard against NULL.

Index: ada-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/ada-lang.c,v
retrieving revision 1.100
diff -p -r1.100 ada-lang.c
*** ada-lang.c	6 Aug 2007 20:07:44 -0000	1.100
--- ada-lang.c	11 Aug 2007 20:39:20 -0000
*************** possible_user_operator_p (enum exp_opcod
*** 3536,3542 ****
          ((TYPE_CODE (type0) != TYPE_CODE_ARRAY
            && (TYPE_CODE (type0) != TYPE_CODE_PTR
                || TYPE_CODE (TYPE_TARGET_TYPE (type0)) != TYPE_CODE_ARRAY))
!          || (TYPE_CODE (type1) != TYPE_CODE_ARRAY
               && (TYPE_CODE (type1) != TYPE_CODE_PTR
                   || (TYPE_CODE (TYPE_TARGET_TYPE (type1)) 
  		     != TYPE_CODE_ARRAY))));
--- 3536,3542 ----
          ((TYPE_CODE (type0) != TYPE_CODE_ARRAY
            && (TYPE_CODE (type0) != TYPE_CODE_PTR
                || TYPE_CODE (TYPE_TARGET_TYPE (type0)) != TYPE_CODE_ARRAY))
!          || (type1 != NULL && TYPE_CODE (type1) != TYPE_CODE_ARRAY
               && (TYPE_CODE (type1) != TYPE_CODE_PTR
                   || (TYPE_CODE (TYPE_TARGET_TYPE (type1)) 
  		     != TYPE_CODE_ARRAY))));

Re: [PATCH] ada-lang, possible_user_operator_p, null pointer

[Joel Brobecker]

> 2007-08-11  Michael Snyder  <msnyder@access-company.com>
> 
> 	* ada-lang.c (possible_user_operator_p): Guard against NULL.

This is another case where I'd prefer to have the check for a NULL
type moved to the top of the function. But I'm having a tough time
determining how this might actually happen. So I'd like to sit on it
for a little longer before we proceed.

> Also, I'm not sure what the CONCAT operator is, but is it
> possible that the OR at the beginning of the same line
> should be an AND?  Just checking...

The BINOP_CONCAT operator corresponds to the "&" binop operator, which
allows us to concatenate two arrays into one. For instance, you may
have some code like this:

        Hello : constant String := "Hello";
        There : constant String := "there";
        Hello_There : constant String := Hello & " " & There;

In terms of the condition, it's kind of confusing, since the logic is
sort of backwards as far as my brain seems to work. But I think the
condition is actually correct: If you have an array on each side,
then use the pre-defined operator. So if you negate this condition,
if either lhs *or* rhs are not arrays, then...

> Index: ada-lang.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/ada-lang.c,v
> retrieving revision 1.100
> diff -p -r1.100 ada-lang.c
> *** ada-lang.c	6 Aug 2007 20:07:44 -0000	1.100
> --- ada-lang.c	11 Aug 2007 20:39:20 -0000
> *************** possible_user_operator_p (enum exp_opcod
> *** 3536,3542 ****
>           ((TYPE_CODE (type0) != TYPE_CODE_ARRAY
>             && (TYPE_CODE (type0) != TYPE_CODE_PTR
>                 || TYPE_CODE (TYPE_TARGET_TYPE (type0)) != TYPE_CODE_ARRAY))
> !          || (TYPE_CODE (type1) != TYPE_CODE_ARRAY
>                && (TYPE_CODE (type1) != TYPE_CODE_PTR
>                    || (TYPE_CODE (TYPE_TARGET_TYPE (type1)) 
>   		     != TYPE_CODE_ARRAY))));
> --- 3536,3542 ----
>           ((TYPE_CODE (type0) != TYPE_CODE_ARRAY
>             && (TYPE_CODE (type0) != TYPE_CODE_PTR
>                 || TYPE_CODE (TYPE_TARGET_TYPE (type0)) != TYPE_CODE_ARRAY))
> !          || (type1 != NULL && TYPE_CODE (type1) != TYPE_CODE_ARRAY
>                && (TYPE_CODE (type1) != TYPE_CODE_PTR
>                    || (TYPE_CODE (TYPE_TARGET_TYPE (type1)) 
>   		     != TYPE_CODE_ARRAY))));

-- 
Joel

Re: [PATCH] ada-lang, possible_user_operator_p, null pointer

[Joel Brobecker]

> > 2007-08-11  Michael Snyder  <msnyder@access-company.com>
> > 
> > 	* ada-lang.c (possible_user_operator_p): Guard against NULL.
> 
> This is another case where I'd prefer to have the check for a NULL
> type moved to the top of the function. But I'm having a tough time
> determining how this might actually happen. So I'd like to sit on it
> for a little longer before we proceed.

I'm glad I sat on it :). I overlooked the fact that part of this function
is not limited to binary operators. I also handles unary operators, so
it would make sense that type1 might actually be NULL.

However, the expression evaluation should always make sure that
we have two operands before trying to evaluate it, so this should
never happen, right?

Unfortunately, digging deeper, I found elsewhere something that looks
suspicious to me, and it actually allowed me to build an example that
trigger a dereference of a NULL type1!!!

I am currently discussing all this with Paul Hilfinger, who wrote
most of this code, and I've suggested some other ways of rewriting
the code that I think will make it more consistent: In short, define
a separate function that matches arrays or pointer to arrays, with
a guard against null pointers, the same as numeric_type_p for instance,
and simplify the condition by using that function. We'd end up with
something like this:

    case BINOP_CONCAT:
      return !(array_type_p (type0) && array_type_p (type1));

We also need to address the bug that this check guards against!

In the meantime, I think it's fine to check in your patch.
We'll probably end up rewriting this part of the code, but
that won't actually change the actual logic, so....

> > Index: ada-lang.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/ada-lang.c,v
> > retrieving revision 1.100
> > diff -p -r1.100 ada-lang.c
> > *** ada-lang.c	6 Aug 2007 20:07:44 -0000	1.100
> > --- ada-lang.c	11 Aug 2007 20:39:20 -0000
> > *************** possible_user_operator_p (enum exp_opcod
> > *** 3536,3542 ****
> >           ((TYPE_CODE (type0) != TYPE_CODE_ARRAY
> >             && (TYPE_CODE (type0) != TYPE_CODE_PTR
> >                 || TYPE_CODE (TYPE_TARGET_TYPE (type0)) != TYPE_CODE_ARRAY))
> > !          || (TYPE_CODE (type1) != TYPE_CODE_ARRAY
> >                && (TYPE_CODE (type1) != TYPE_CODE_PTR
> >                    || (TYPE_CODE (TYPE_TARGET_TYPE (type1)) 
> >   		     != TYPE_CODE_ARRAY))));
> > --- 3536,3542 ----
> >           ((TYPE_CODE (type0) != TYPE_CODE_ARRAY
> >             && (TYPE_CODE (type0) != TYPE_CODE_PTR
> >                 || TYPE_CODE (TYPE_TARGET_TYPE (type0)) != TYPE_CODE_ARRAY))
> > !          || (type1 != NULL && TYPE_CODE (type1) != TYPE_CODE_ARRAY
> >                && (TYPE_CODE (type1) != TYPE_CODE_PTR
> >                    || (TYPE_CODE (TYPE_TARGET_TYPE (type1)) 
> >   		     != TYPE_CODE_ARRAY))));

-- 
Joel

Re: [PATCH] ada-lang, possible_user_operator_p, null pointer

[msnyder]

> In the meantime, I think it's fine to check in your patch.
> We'll probably end up rewriting this part of the code, but
> that won't actually change the actual logic, so....

Glad you're on it, and I'll commit this patch.
Michael,

[commit] ada-lang, possible_user_operator_p, null pointer (take 2)

[Joel Brobecker]

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

Hello,

> > In the meantime, I think it's fine to check in your patch.
> > We'll probably end up rewriting this part of the code, but
> > that won't actually change the actual logic, so....
> 
> Glad you're on it, and I'll commit this patch.

Paul Hilfinger pointed out that we actually have the perfect
function for it, so we don't even have to create it. I just
committed the following patch, which considerably simplifies
the complexity of the condition :).

2007-08-15  Paul Hilfinger  <hilfinger@adacore.com>
            Joel Brobecker  <brobecker@adacore.com>

        * ada-lang.c (possible_user_operator_p): Alternative fix to last
        checkin guarding against NULL.

Tested on x86-linux.

-- 
Joel

[-- Attachment #2: user_op.diff --]
[-- Type: text/plain, Size: 994 bytes --]

Index: ada-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/ada-lang.c,v
retrieving revision 1.102
diff -u -p -r1.102 ada-lang.c
--- ada-lang.c	14 Aug 2007 20:16:16 -0000	1.102
+++ ada-lang.c	15 Aug 2007 18:31:06 -0000
@@ -3532,14 +3535,7 @@ possible_user_operator_p (enum exp_opcod
       return (!(scalar_type_p (type0) && scalar_type_p (type1)));
 
     case BINOP_CONCAT:
-      return
-        ((TYPE_CODE (type0) != TYPE_CODE_ARRAY
-          && (TYPE_CODE (type0) != TYPE_CODE_PTR
-              || TYPE_CODE (TYPE_TARGET_TYPE (type0)) != TYPE_CODE_ARRAY))
-         || (type1 != NULL && TYPE_CODE (type1) != TYPE_CODE_ARRAY
-             && (TYPE_CODE (type1) != TYPE_CODE_PTR
-                 || (TYPE_CODE (TYPE_TARGET_TYPE (type1)) 
-		     != TYPE_CODE_ARRAY))));
+      return !ada_is_array_type (type0) || !ada_is_array_type (type1);
 
     case BINOP_EXP:
       return (!(numeric_type_p (type0) && integer_type_p (type1)));