[RFA] Handle BINOP_INTDIV in valarith.c

[RFA] Handle BINOP_INTDIV in valarith.c

[Pierre Muller]

While trying to write a pascal expression parser test
I came upon the problem that
BINOP_INTDIV is not handled in value_binop function.

(gdb) set lang pascal
(gdb) print 25 div 2
GDB does not (yet) know how to evaluate that kind of expression

This patch fixes this.

OK to commit?

Pierre Muller

I also noticed that the unsigned case is not guarded against v2 being zero,
but I will send another patch for this once that one is in.


ChangeLog entry:

2008-01-16  Pierre Muller  <muller@ics.u-strasbg.fr>

	* valarith.c (value_binop): Handle BINOP_INTDIV
	for unsigned and signed integers.



Index: gdb/valarith.c
===================================================================
RCS file: /cvs/src/src/gdb/valarith.c,v
retrieving revision 1.52
diff -u -p -r1.52 valarith.c
--- gdb/valarith.c	7 Jan 2008 22:33:57 -0000	1.52
+++ gdb/valarith.c	16 Jan 2008 14:09:44 -0000
@@ -1033,6 +1033,7 @@ value_binop (struct value *arg1, struct 
 	      break;
 
 	    case BINOP_DIV:
+	    case BINOP_INTDIV:
 	      v = v1 / v2;
 	      break;
 
@@ -1152,6 +1153,7 @@ value_binop (struct value *arg1, struct 
 	      break;
 
 	    case BINOP_DIV:
+	    case BINOP_INTDIV:
 	      if (v2 != 0)
 		v = v1 / v2;
 	      else

Re: [RFA] Handle BINOP_INTDIV in valarith.c

[Eli Zaretskii]

> From: "Pierre Muller" <muller@ics.u-strasbg.fr>
> Date: Wed, 16 Jan 2008 15:13:14 +0100
> 
> While trying to write a pascal expression parser test
> I came upon the problem that
> BINOP_INTDIV is not handled in value_binop function.
> 
> (gdb) set lang pascal
> (gdb) print 25 div 2
> GDB does not (yet) know how to evaluate that kind of expression
> 
> This patch fixes this.
> 
> OK to commit?

If this patch is approved, please document this operator in the
"Pascal" node of the user manual.

TIA

RE: [RFA] Handle BINOP_INTDIV in valarith.c

[Pierre Muller]

> If this patch is approved, please document this operator in the
> "Pascal" node of the user manual.

Hi Eli,

  as I got the approval of Joel, I 
committed this patch last week.
  But I had an email connection problem
last week that prevented me from replying to
your email.

  Concerning the pascal 'div' operator
subject of my patch, I must admit that 
there is no reason to add it to 'pascal' node,
because there is basically nothing in that node yet,
and the little that is present is obsolete anyway.

  I would need to rewrite completely, I will try to work on this.
The BINOP_INTDIV seems to be used also
in fortran language (f-lang.c:237)
and modula-2 language (m2-exp.y:430 and m2-lang.c:290)
But there seems to no test this feature, as I saw no
testsuite change.
  I will add a pascal testsuite that
does test this feature.

  I have another problem with the '/' operator:
for pascal and contrary to C language
'/' is always returning a float value, even if both
left and right nodes are integers.

  I will send a separate patch for
a new binop called BINOP_FLOATDIV
that will force the result of 'a / b' to be a
float in all cases.

  This is the right thing to do for pascal,
but I don't know about the other languages:
do fortran, Ada, Modula-2 or java
allow 'a / b' for a or b of integer types?

Pierre Muller

Pascal language maintainer.

Re: [RFA] Handle BINOP_INTDIV in valarith.c

[Joel Brobecker]

>   I will send a separate patch for
> a new binop called BINOP_FLOATDIV
> that will force the result of 'a / b' to be a
> float in all cases.

That seems unnecessary to me unless BINOP_DIV and your BINOP_FLOATDIV
have different meanings in Pascal? Otherwise, I think the problem is
that the pascal language needs its own expression evaluator so that
it can handle the '/' operator specifically. The rest can be delegated
to the standard expression evaluator.

>   This is the right thing to do for pascal, but I don't know about the
>   other languages: do fortran, Ada, Modula-2 or java allow 'a / b' for
>   a or b of integer types?

Ada does, but it's an overloaded operator, so the type of the result
depends on the type of the arguments. Don't know about the other
languages.

-- 
Joel

RE: [RFA] Handle BINOP_INTDIV in valarith.c

[Pierre Muller]

> -----Original Message-----
> From: Joel Brobecker [mailto:brobecker@adacore.com]
> Sent: Wednesday, January 23, 2008 7:25 PM
> To: Pierre Muller
> Cc: 'Eli Zaretskii'; gdb-patches@sourceware.org
> Subject: Re: [RFA] Handle BINOP_INTDIV in valarith.c
> 
> >   I will send a separate patch for
> > a new binop called BINOP_FLOATDIV
> > that will force the result of 'a / b' to be a
> > float in all cases.
> 
> That seems unnecessary to me unless BINOP_DIV and your BINOP_FLOATDIV
> have different meanings in Pascal? Otherwise, I think the problem is
> that the pascal language needs its own expression evaluator so that
> it can handle the '/' operator specifically. The rest can be delegated
> to the standard expression evaluator.

  I wrote the floatdiv version,
which allows me to easily force the conversion to double
formats for left and right operand:
  This simple patch portion from gdb/valarith.c does the main trick:
@@ -865,7 +868,8 @@ value_binop (struct value *arg1, struct 
     }
   else if (TYPE_CODE (type1) == TYPE_CODE_FLT
 	   ||
-	   TYPE_CODE (type2) == TYPE_CODE_FLT)
+	   TYPE_CODE (type2) == TYPE_CODE_FLT
+	   || op == BINOP_FLOATDIV)
     {
       /* FIXME-if-picky-about-floating-accuracy: Should be doing this
          in target format.  real.c in GCC probably has the necessary

Adding BINOP_FLOATDIV is indeed unnecessary if
you can tell me how to check for the correct
languages here.

   maybe 
   || (op == BINOP_DIV && current_language = language_pascal))
would work, but I don't know if current_language is the right variable to
test and it would make the use of this feature by other languages
more complicated than just switching from
BINOP_DIV vto BINOP_FLOATDIV in their respective expression parser?


Pierre Muller
GDB pascal language support maintainer

Re: [RFA] Handle BINOP_INTDIV in valarith.c

[Joel Brobecker]

>   I wrote the floatdiv version, which allows me to easily force the
>   conversion to double formats for left and right operand:

Right, but you are doing the conversion inside what I call "core-gdb",
which is a language-independent file. As a result, you ended up needing
a new operator.  But the creation of this new operator would not be
necessary if you handled BINOP_DIV in a pascal-specific way.

>    maybe 
>    || (op == BINOP_DIV && current_language = language_pascal))

That's not the proper way of doing this. What you need to do is to
setup your language to have a pascal-specific expression evaluator.
Right now, it's set to use the "standard" expression evaluator which
(IIRC) is the C evaluator.  Your pascal expression evaluator will
defer back to the standard expression evaluator most of the time,
except for the case when the binary operator is FLOAT_DIV.  In this
case, you'll do the division yourself.

You can have a look at ada-lang.c:ada_evaluate_subexp(). It's a large
routine because of a lot of stuff is special to Ada, but you'll also
see:

  op = exp->elts[pc].opcode;
  [...]
  switch (op)
    {
    default:
      *pos -= 1;
      arg1 = evaluate_subexp_standard (expect_type, exp, pos, noside);
    [...]
    case BINOP_DIV:
      arg1 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
      arg2 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
      if (noside == EVAL_SKIP)
        goto nosideret;
      else if (noside == EVAL_AVOID_SIDE_EFFECTS
               && (op == BINOP_DIV || op == BINOP_REM || op == BINOP_MOD))
        return value_zero (value_type (arg1), not_lval);
      else
        {
          if (ada_is_fixed_point_type (value_type (arg1)))
            arg1 = cast_from_fixed_to_double (arg1);
          if (ada_is_fixed_point_type (value_type (arg2)))
            arg2 = cast_from_fixed_to_double (arg2);
          return ada_value_binop (arg1, arg2, op);
        }

If you look at the "default:" case, you'll see that we let
the standard evaluator do the work for us. But we treat the
BINOP_DIV case specially: First we evaluate each side of our
division, and then perform the division and return the result.

This ada_evaluate_subexp routine is setup in the language through
the exp_descriptor structure.

-- 
Joel

RE: [RFA] Handle BINOP_INTDIV in valarith.c

[Pierre Muller]

OK, I get the idea,
but the problem is that this leads to
lots of code copy...
And errors are duplicated at several places:
  See for instance my bug report:
http://sourceware.org/ml/gdb/2008-01/msg00209.html

'ptyp 3 / 2.0'
which returns 'int' type
while 'print 3 / 2.0' returns '1.5' which is not an int!

I suspect that the code in Ada shows the same bugs:

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Joel Brobecker
> Sent: Thursday, January 24, 2008 12:09 AM
> To: Pierre Muller
> Cc: 'Eli Zaretskii'; gdb-patches@sourceware.org
> Subject: Re: [RFA] Handle BINOP_INTDIV in valarith.c
> 
> >   I wrote the floatdiv version, which allows me to easily force the
> >   conversion to double formats for left and right operand:
> 
> Right, but you are doing the conversion inside what I call "core-gdb",
> which is a language-independent file. As a result, you ended up needing
> a new operator.  But the creation of this new operator would not be
> necessary if you handled BINOP_DIV in a pascal-specific way.
> 
> >    maybe
> >    || (op == BINOP_DIV && current_language = language_pascal))
> 
> That's not the proper way of doing this. What you need to do is to
> setup your language to have a pascal-specific expression evaluator.
> Right now, it's set to use the "standard" expression evaluator which
> (IIRC) is the C evaluator.  Your pascal expression evaluator will
> defer back to the standard expression evaluator most of the time,
> except for the case when the binary operator is FLOAT_DIV.  In this
> case, you'll do the division yourself.
> 
> You can have a look at ada-lang.c:ada_evaluate_subexp(). It's a large
> routine because of a lot of stuff is special to Ada, but you'll also
> see:
> 
>   op = exp->elts[pc].opcode;
>   [...]
>   switch (op)
>     {
>     default:
>       *pos -= 1;
>       arg1 = evaluate_subexp_standard (expect_type, exp, pos, noside);
>     [...]
>     case BINOP_DIV:
>       arg1 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
>       arg2 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
>       if (noside == EVAL_SKIP)
>         goto nosideret;
>       else if (noside == EVAL_AVOID_SIDE_EFFECTS
>                && (op == BINOP_DIV || op == BINOP_REM || op ==
> BINOP_MOD))
>         return value_zero (value_type (arg1), not_lval);
which means that 'ptyp' which sets noside to EVAL_AVOID_SIDE_EFFECTS
will not see the fixed to double conversions below
and will also return 'fixed', while the result of print will be
a 'double'.
  Furthermore, you can clearly see that this code
was copied from the C evaluator without removing the 
unnecessary parts:
the check
>                && (op == BINOP_DIV || op == BINOP_REM || op ==
> BINOP_MOD))
is clearly redundant here as we are inside case BINOP_DIV.

The same unnecessary test is also done in the
BINOP_MOD-BINOP_REM case.

>       else
>         {
>           if (ada_is_fixed_point_type (value_type (arg1)))
>             arg1 = cast_from_fixed_to_double (arg1);
>           if (ada_is_fixed_point_type (value_type (arg2)))
>             arg2 = cast_from_fixed_to_double (arg2);
>           return ada_value_binop (arg1, arg2, op);
>         }
> 
> If you look at the "default:" case, you'll see that we let
> the standard evaluator do the work for us. But we treat the
> BINOP_DIV case specially: First we evaluate each side of our
> division, and then perform the division and return the result.
> 
> This ada_evaluate_subexp routine is setup in the language through
> the exp_descriptor structure.

  I agree with you that writing a pascal specific
evaluate_subexp() function is better that
explicitly checking the language of the currently parsed expression,
but my idea was really to have on the contrary
something usable for other languages, and thus not inside 
pascal specific code...
  
  On the other hand, no one said that 
another language has the same view on the '/' operator as pascal...
Whihch validates your view that a new BINOP is not necessary...
So I will probably try to write a pascal_evaluate_subexp function
as you suggested.

  Thanks for the feedback.

Pierre Muller

Re: [RFA] Handle BINOP_INTDIV in valarith.c

[Joel Brobecker]

Pierre,

You raised some valid points in your message. I got a code review
for free :) and I sincerly thank you for that. I'll look into
them as soon as I can.

> OK, I get the idea, but the problem is that this leads to lots of code
> copy...

For this part, we can extract out the code you would otherwise
duplicate and have the pascal_evaluate_subexp call it.

> I agree with you that writing a pascal specific evaluate_subexp()
> function is better that explicitly checking the language of the
> currently parsed expression, but my idea was really to have on the
> contrary something usable for other languages, and thus not inside
> pascal specific code...

I don't think that we'll have to give up on your suggestion to
share the code. As explained above, any code that needs to be shared
can be shared through a single function.  Would it work?

-- 
Joel

Re: [RFA] Handle BINOP_INTDIV in valarith.c

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <muller@ics.u-strasbg.fr> writes:

Pierre>   This is the right thing to do for pascal,
Pierre> but I don't know about the other languages:
Pierre> do fortran, Ada, Modula-2 or java
Pierre> allow 'a / b' for a or b of integer types?

For Java the normal binary promotion rules apply to '/'.

That is: if either a or b is double, the other is promoted to double.
Then likewise for float.
Then likewise for long.
And finally, if none of those apply, both are promoted to int.

So IOW, yes :)

There are also special rules about certain integer divisions.
Division by zero throws an exception, and MIN_INT/-1 is defined to be
MIN_INT.

In Java 5 there is also unboxing, but we never updated gdb to know
about that.

Tom

RE: [RFA] Handle BINOP_INTDIV in valarith.c

[Pierre Muller]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Tom Tromey
> Sent: Wednesday, January 23, 2008 7:30 PM
> To: Pierre Muller
> Cc: 'Eli Zaretskii'; gdb-patches@sourceware.org
> Subject: Re: [RFA] Handle BINOP_INTDIV in valarith.c
> 
> >>>>> "Pierre" == Pierre Muller <muller@ics.u-strasbg.fr> writes:
> 
> Pierre>   This is the right thing to do for pascal,
> Pierre> but I don't know about the other languages:
> Pierre> do fortran, Ada, Modula-2 or java
> Pierre> allow 'a / b' for a or b of integer types?
> 
> For Java the normal binary promotion rules apply to '/'.
> 
> That is: if either a or b is double, the other is promoted to double.
> Then likewise for float.
Currently gdb does promotion to doubles
if one of the two is TYPE_CODE_FLT (i.e. any type of floating point,
except maybe the decimal floating that was recently introduced and
which code I did not look into yet).

> Then likewise for long.
> And finally, if none of those apply, both are promoted to int.
> 
> So IOW, yes :)

 But this means that like C and unlike pascal
'35 / 2' returns a integer of value 17, while for pascal
it does return a real of value 17.5.

 
> There are also special rules about certain integer divisions.
> Division by zero throws an exception, and MIN_INT/-1 is defined to be
> MIN_INT.
You probably ment MAX_INT here, no? 
> In Java 5 there is also unboxing, but we never updated gdb to know
> about that.
I almost never used Java, so I have no idea what unboxing means...

Pierre Muller

Re: [RFA] Handle BINOP_INTDIV in valarith.c

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <muller@ics.u-strasbg.fr> writes:

>> So IOW, yes :)

Pierre>  But this means that like C and unlike pascal
Pierre> '35 / 2' returns a integer of value 17, while for pascal
Pierre> it does return a real of value 17.5.

I must have misunderstood your question.  I thought you were asking if
Java has integer division, which it does.  Anyway, it doesn't matter,
since I think we understand each other.

>> There are also special rules about certain integer divisions.
>> Division by zero throws an exception, and MIN_INT/-1 is defined to be
>> MIN_INT.
Pierre> You probably ment MAX_INT here, no? 

Nope.  Maybe the code is clearer; on some platforms we have to
implement integer divide via this function:

jint
_Jv_divI (jint dividend, jint divisor)
{
  if (__builtin_expect (divisor == 0, false))
    {
      java::lang::ArithmeticException *arithexception 
	= new java::lang::ArithmeticException (JvNewStringLatin1 ("/ by zero"));      
      throw arithexception;
    }
  
  if (dividend == (jint) 0x80000000L && divisor == -1)
    return dividend;

  return dividend / divisor;
}

There is a similar function for long.

>> In Java 5 there is also unboxing, but we never updated gdb to know
>> about that.

Pierre> I almost never used Java, so I have no idea what unboxing means...

A primitive type like 'int' has a corresponding object wrapper type,
e.g., Integer.  Unboxing means the compiler will automatically fetch
the value from an object wrapper.  I.e., this is valid:

    public int add5(Integer x) { return x + 5; }


More than you ever wanted to know about Java, I'm sure :)

Tom

Re: [RFA] Handle BINOP_INTDIV in valarith.c

[Joel Brobecker]

For the record, I asked myself about the reason for using both BINOP_DIV
and BINOP_INTDIV if both have the same semantics. I looked at some
Pascal web sites, and the only difference between the '/' operator
and the 'div' operator is the fact that '/' returns a real whereas
"div" only operates on integers an returns an integer. So the distinction
is indeed necessary.

> 2008-01-16  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> 	* valarith.c (value_binop): Handle BINOP_INTDIV
> 	for unsigned and signed integers.

This patch is approved.

-- 
Joel

Re: [RFA] Handle BINOP_INTDIV in valarith.c

[Joel Brobecker]

> > 2008-01-16  Pierre Muller  <muller@ics.u-strasbg.fr>
> > 
> > 	* valarith.c (value_binop): Handle BINOP_INTDIV
> > 	for unsigned and signed integers.
> 
> This patch is approved.

BTW: Please remember to update the documentation as asked by Eli!
I think this is also worth a NEWS entry. Eli?

-- 
Joel

Re: [RFA] Handle BINOP_INTDIV in valarith.c

[Eli Zaretskii]

> Date: Thu, 17 Jan 2008 04:03:27 -0800
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> I think this is also worth a NEWS entry. Eli?

Yes, I agree.

RE: [RFA] Handle BINOP_INTDIV in valarith.c

[Pierre Muller]

 I would like to have some feedback from the other languages that
seem to also use BINOP_INTDIV.
 
  Furthermore, my patch is incomplete,
I forgot the eval.c part that is required:

I send here a minimal patch to
allow to get a 'print 13 div 3' to return '4'

  Does this patch work for fortran, ada, modula-2?
I suspect that the type checking is not appropriate for
most of these languages:
Boolean types are not considered as ordinals
in pascal, and probably also not in the languages
cited above.
  For pascal, 'print 13 div true'
as well as print 12 + true'
should not be allowed, but it is now...

  Is this OK?
  
ChangeLog entry:

2008-01-21  Pierre Muller  <muller@ics.u-strasbg.fr>

	* eval.c (evaluate_subexp_standard): Support
	BINOP_INTDIV opcode.


Index: gdb/eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.77
diff -u -p -r1.77 eval.c
--- gdb/eval.c  18 Jan 2008 17:07:39 -0000      1.77
+++ gdb/eval.c  21 Jan 2008 14:55:45 -0000
@@ -1496,6 +1496,7 @@ evaluate_subexp_standard (struct type *e
     case BINOP_EXP:
     case BINOP_MUL:
     case BINOP_DIV:
+    case BINOP_INTDIV:
     case BINOP_REM:
     case BINOP_MOD:
     case BINOP_LSH:
@@ -1510,7 +1511,8 @@ evaluate_subexp_standard (struct type *e
       if (binop_user_defined_p (op, arg1, arg2))
        return value_x_binop (arg1, arg2, op, OP_NULL, noside);
       else if (noside == EVAL_AVOID_SIDE_EFFECTS
-              && (op == BINOP_DIV || op == BINOP_REM || op == BINOP_MOD))
+              && (op == BINOP_DIV || op == BINOP_REM || op == BINOP_MOD
+                  || op == BINOP_INTDIV))
        return value_zero (value_type (arg1), not_lval);
       else
        return value_binop (arg1, arg2, op);

[RFA] Handle BINOP_INTDIV in eval.c

[Pierre Muller]

 My first patch handling BINOP_INTDIV was incomplete,
I forgot the eval.c part that is required:

I send here a minimal patch to
allow to get a 'print 13 div 3' to return '4'

  Is this OK?
  
ChangeLog entry:

2008-01-25  Pierre Muller  <muller@ics.u-strasbg.fr>

	* eval.c (evaluate_subexp_standard): Support
	BINOP_INTDIV opcode.


Index: gdb/eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.77
diff -u -p -r1.77 eval.c
--- gdb/eval.c  18 Jan 2008 17:07:39 -0000      1.77
+++ gdb/eval.c  21 Jan 2008 14:55:45 -0000
@@ -1496,6 +1496,7 @@ evaluate_subexp_standard (struct type *e
     case BINOP_EXP:
     case BINOP_MUL:
     case BINOP_DIV:
+    case BINOP_INTDIV:
     case BINOP_REM:
     case BINOP_MOD:
     case BINOP_LSH:
@@ -1510,7 +1511,8 @@ evaluate_subexp_standard (struct type *e
       if (binop_user_defined_p (op, arg1, arg2))
        return value_x_binop (arg1, arg2, op, OP_NULL, noside);
       else if (noside == EVAL_AVOID_SIDE_EFFECTS
-              && (op == BINOP_DIV || op == BINOP_REM || op == BINOP_MOD))
+              && (op == BINOP_DIV || op == BINOP_REM || op == BINOP_MOD
+                  || op == BINOP_INTDIV))
        return value_zero (value_type (arg1), not_lval);
       else
        return value_binop (arg1, arg2, op);

Re: [RFA] Handle BINOP_INTDIV in eval.c

[Daniel Jacobowitz]

On Fri, Jan 25, 2008 at 11:52:28AM +0100, Pierre Muller wrote:
>  My first patch handling BINOP_INTDIV was incomplete,
> I forgot the eval.c part that is required:
> 
> I send here a minimal patch to
> allow to get a 'print 13 div 3' to return '4'
> 
>   Is this OK?
>   
> ChangeLog entry:
> 
> 2008-01-25  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> 	* eval.c (evaluate_subexp_standard): Support
> 	BINOP_INTDIV opcode.

OK.

-- 
Daniel Jacobowitz
CodeSourcery

RE: [RFA] Handle BINOP_INTDIV in eval.c

[Pierre Muller]

Thanks, committed right now.

  I will try to also commit my pascal testcase
later.

Pierre Muller

> -----Original Message-----
> From: Daniel Jacobowitz [mailto:drow@false.org]
> Sent: Wednesday, January 30, 2008 1:59 AM
> To: Pierre Muller
> Cc: 'Joel Brobecker'; gdb-patches@sourceware.org
> Subject: Re: [RFA] Handle BINOP_INTDIV in eval.c
> 
> On Fri, Jan 25, 2008 at 11:52:28AM +0100, Pierre Muller wrote:
> >  My first patch handling BINOP_INTDIV was incomplete,
> > I forgot the eval.c part that is required:
> >
> > I send here a minimal patch to
> > allow to get a 'print 13 div 3' to return '4'
> >
> >   Is this OK?
> >
> > ChangeLog entry:
> >
> > 2008-01-25  Pierre Muller  <muller@ics.u-strasbg.fr>
> >
> > 	* eval.c (evaluate_subexp_standard): Support
> > 	BINOP_INTDIV opcode.
> 
> OK.
> 
> --
> Daniel Jacobowitz
> CodeSourcery