Re: [BUG] BINOP_DIV and ptyp command

Re: [BUG] BINOP_DIV and ptyp command

[Doug Evans]

On Jan 21, 2008 1:47 AM, Pierre Muller <muller@ics.u-strasbg.fr> wrote:
> The ptyp command returns inconsistent types for the c '/'(BINOP_DIV)
> operator:
>
> (gdb) ptyp 3 / 2
> type = int
> (gdb) p 3 / 2
> $4 = 1
> (gdb) ptyp 3.0 / 2
> type = double
> (gdb) p 3.0 / 2
> $5 = 1.5
> (gdb) ptyp 3 / 2.0
> type = int
> (gdb) p 3 / 2.0
> $6 = 1.5
>
> I suspect that this bug is due to the fact that
> the type of this binary operator is inferred from the
> left node type, but this is wrong in the case '3 / 2.0'
>
>
> Pierre Muller

I may be missing something, but it seems like special casing
EVAL_AVOID_SIDE_EFFECTS for DIV/MOD/REM is no longer useful here (one
can imagine that it was added way back when to avoid division by zero,
but I'm just guessing).  I don't see any need to special case this
here, value_binop will catch integer division by zero and doesn't have
any special checks for floating point division by 0 (uses host fp
which can result in +/-inf, and isn't necessarily accurate for the
target at any rate, but that's an orthogonal issue).  If it were a
case of wanting to avoid tripping over the call to error("Division by
zero") then I'd expect to also see special casing of "1 >> 3.0" when
EVAL_AVOID_SIDE_EFFECTS.  Also note that for integer MOD, "division by
zero" isn't an issue.

So it seems like the following is the correct patch.

2008-01-28  Doug Evans  <dje@google.com>

        * eval.c (evaluate_subexp_standard): If DIV/REM/MOD and
        EVAL_AVOID_SIDE_EFFECTS, remove special case and process normally.

Index: eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.77
diff -u -p -u -p -r1.77 eval.c
--- eval.c      18 Jan 2008 17:07:39 -0000      1.77
+++ eval.c      29 Jan 2008 04:37:52 -0000
@@ -1509,9 +1509,6 @@ evaluate_subexp_standard (struct type *e
        goto nosideret;
       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))
-       return value_zero (value_type (arg1), not_lval);
       else
        return value_binop (arg1, arg2, op);

Re: [BUG] BINOP_DIV and ptyp command

[Doug Evans]

On Jan 28, 2008 8:52 PM, Doug Evans <dje@google.com> wrote:
> [...]
> So it seems like the following is the correct patch.

Well, not quite.  Ok to check the appended patch in?

Changing the expected result to long makes / and % no longer special -
x*y has an expected result of long.

One can argue it should be int, but any fix for that is orthogonal to
fixing / and % handling.  I think.

Derived from whatis-exp.exp:

(gdb) whatis x
type = int
(gdb) whatis y
type = int
(gdb) whatis x*y
type = long

2008-01-28  Doug Evans  <dje@sebabeach.org>

        * eval.c (evaluate_subexp_standard): If DIV/REM/MOD and
        EVAL_AVOID_SIDE_EFFECTS, remove special case and process normally.

        * gdb.base/whatis-exp.exp: Update expected result of x/y and x%y.

Index: eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.77
diff -u -p -r1.77 eval.c
--- eval.c      18 Jan 2008 17:07:39 -0000      1.77
+++ eval.c      29 Jan 2008 05:27:55 -0000
@@ -1509,9 +1509,6 @@ evaluate_subexp_standard (struct type *e
        goto nosideret;
       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))
-       return value_zero (value_type (arg1), not_lval);
       else
        return value_binop (arg1, arg2, op);

Index: testsuite/gdb.base/whatis-exp.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/whatis-exp.exp,v
retrieving revision 1.7
diff -u -p -r1.7 whatis-exp.exp
--- testsuite/gdb.base/whatis-exp.exp   1 Jan 2008 22:53:19 -0000       1.7
+++ testsuite/gdb.base/whatis-exp.exp   29 Jan 2008 05:27:55 -0000
@@ -139,7 +139,7 @@ gdb_expect {

 send_gdb "whatis x/y\n"
 gdb_expect {
-    -re ".*type = int.*$gdb_prompt $" {
+    -re ".*type = long.*$gdb_prompt $" {
         pass "whatis value of x/y"
       }
     -re ".*$gdb_prompt $" { fail "whatis value of x/y" }
@@ -148,7 +148,7 @@ gdb_expect {

 send_gdb "whatis x%y\n"
 gdb_expect {
-    -re ".*type = int.*$gdb_prompt $" {
+    -re ".*type = long.*$gdb_prompt $" {
         pass "whatis value of x%y"
       }
     -re ".*$gdb_prompt $" { fail "whatis value of x%y" }

Re: [BUG] BINOP_DIV and ptyp command

[Joel Brobecker]

> I may be missing something, but it seems like special casing
> EVAL_AVOID_SIDE_EFFECTS for DIV/MOD/REM is no longer useful here

I don't think this is right. EVAL_AVOID_SIDE_EFFECTS is used when
computing the actual value is not needed. For instance, when you do
"ptype", the expression is evaluated in that mode.  So when you do
"ptype 3 div 2", we don't do the division, we just know that we're only
interested in the type of result.  So the expression evaluator will
return a struct value of the correct type but with a bogus contents,
instead of doing the division, only to discard the result down the road.

So the code you suggested we remove is useful.

-- 
Joel

RE: [BUG] BINOP_DIV and ptyp command

[Pierre Muller]

  The code might be useful, but it returns wrong results...
Because if you go the the value_binop
code, you will see that there are some typecast
that do change the result type of BINOP_DIV.
  The problem with Doug's code is that
the error('Division by zero') will be thrown
even for a ptyp command, which is indeed wrong,
but how do we fix the bug then?

  By the way, Doug patch is also not correct because
even though the signed integer division is caught
in value_binop, the corresponding unsigned code
is still missing the same check.
  The patch below fixes
'p 1/0U' output, without the patch
my cygwin gdb just freezes!

OK to commit?

ChangeLog entry:

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

	* valarith.c (value_binop): Handle unsigned integer
	division by zero.

Index: gdb/valarith.c
===================================================================
RCS file: /cvs/src/src/gdb/valarith.c,v
retrieving revision 1.54
diff -u -p -r1.54 valarith.c
--- gdb/valarith.c      18 Jan 2008 17:07:40 -0000      1.54
+++ gdb/valarith.c      29 Jan 2008 07:23:43 -0000
@@ -1035,7 +1035,10 @@ value_binop (struct value *arg1, struct

            case BINOP_DIV:
            case BINOP_INTDIV:
-             v = v1 / v2;
+             if (v2 != 0)
+               v = v1 / v2;
+             else
+               error (_("Division by zero"));
              break;

            case BINOP_EXP: 

Re: [BUG] BINOP_DIV and ptyp command

[Daniel Jacobowitz]

On Tue, Jan 29, 2008 at 08:35:34AM +0100, Pierre Muller wrote:
> 2008-01-29  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> 	* valarith.c (value_binop): Handle unsigned integer
> 	division by zero.

OK.

(How embarrassing.  I've been meaning to commit this for three years!)

-- 
Daniel Jacobowitz
CodeSourcery

Re: [BUG] BINOP_DIV and ptyp command

[Daniel Jacobowitz]

On Mon, Jan 28, 2008 at 09:34:48PM -0800, Doug Evans wrote:
> On Jan 28, 2008 8:52 PM, Doug Evans <dje@google.com> wrote:
> > [...]
> > So it seems like the following is the correct patch.
> 
> Well, not quite.  Ok to check the appended patch in?
> 
> Changing the expected result to long makes / and % no longer special -
> x*y has an expected result of long.
> 
> One can argue it should be int, but any fix for that is orthogonal to
> fixing / and % handling.  I think.

If you had to change a testcase to a more wrong answer, then the bug
isn't orthogonal :-) We should be following C promotion rules, and
automatically promoting int math to long is incorrect.

-- 
Daniel Jacobowitz
CodeSourcery

RE: [BUG] BINOP_DIV and ptyp command

[Pierre Muller]

Committed, thanks.

May I also add a corresponding test?

testsuite/Changelog entry:

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

	* gdb.base/gdb1056.exp: Add unsigned integer test.


Index: gdb/testsuite/gdb.base/gdb1056.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/gdb1056.exp,v
retrieving revision 1.4
diff -r1.4 gdb1056.exp
47a48,49
> gdb_test "print 1U/0" ".*Division by zero.*" "Test unsigned division by
zero"
>

Re: [BUG] BINOP_DIV and ptyp command

[Doug Evans]

On Jan 28, 2008 10:41 PM, Joel Brobecker <brobecker@adacore.com> wrote:
> > I may be missing something, but it seems like special casing
> > EVAL_AVOID_SIDE_EFFECTS for DIV/MOD/REM is no longer useful here
>
> I don't think this is right. EVAL_AVOID_SIDE_EFFECTS is used when
> computing the actual value is not needed. For instance, when you do
> "ptype", the expression is evaluated in that mode.  So when you do
> "ptype 3 div 2", we don't do the division, we just know that we're only
> interested in the type of result.  So the expression evaluator will
> return a struct value of the correct type but with a bogus contents,
> instead of doing the division, only to discard the result down the road.
>
> So the code you suggested we remove is useful.

As I say, if the intent is to avoid the call to error(), I'd expect to
see tests for 1 >> 3.0, etc.  That will throw error ("Integer-only
operation on floating point number").  If that code is the code that's
missing, fine, except that I'd also change the comments describing
what EVAL_AVOID_SIDE_EFFECTS is for:

    EVAL_AVOID_SIDE_EFFECTS     /* Don't modify any variables or
                                   call any functions.  The value
                                   returned will have the correct
                                   type, and will have an
                                   approximately correct lvalue
                                   type (inaccuracy: anything that is
                                   listed as being in a register in
                                   the function in which it was
                                   declared will be lval_register).  */

One can believe the intent that side effects in this particular case
includes throwing error(), but the docs should be explicit on this.

Re: [BUG] BINOP_DIV and ptyp command

[Doug Evans]

On Jan 28, 2008 11:35 PM, Pierre Muller <muller@ics.u-strasbg.fr> wrote:
>
>   By the way, Doug patch is also not correct because
> even though the signed integer division is caught
> in value_binop, the corresponding unsigned code
> is still missing the same check.

Fix one bug, expose another ...
The latter bug's existence is not a proof one way or another that any
patch to the former bug is or isn't correct.

Re: [BUG] BINOP_DIV and ptyp command

[Doug Evans]

On Jan 29, 2008 5:46 AM, Daniel Jacobowitz <drow@false.org> wrote:
> On Mon, Jan 28, 2008 at 09:34:48PM -0800, Doug Evans wrote:
> > On Jan 28, 2008 8:52 PM, Doug Evans <dje@google.com> wrote:
> > > [...]
> > > So it seems like the following is the correct patch.
> >
> > Well, not quite.  Ok to check the appended patch in?
> >
> > Changing the expected result to long makes / and % no longer special -
> > x*y has an expected result of long.
> >
> > One can argue it should be int, but any fix for that is orthogonal to
> > fixing / and % handling.  I think.
>
> If you had to change a testcase to a more wrong answer, then the bug
> isn't orthogonal :-) We should be following C promotion rules, and
> automatically promoting int math to long is incorrect.

I can't tell if that's a fun comment for grin's sake or not.  I can
flip my pedantic bit and argue the same thing.  Obviously "ptype
int+int" is "int".

In this particular case having to hack whatis-exp.exp to "pass" until
the general bug is fixed is ok by me - is anyone relying on ptype 4/2
to be int whereas ptype 4*2 is long?

Re: [BUG] BINOP_DIV and ptyp command

[Doug Evans]

On Jan 29, 2008 8:03 AM, Doug Evans <dje@google.com> wrote:
> I can't tell if that's a fun comment for grin's sake or not.  I can
> flip my pedantic bit and argue the same thing.  Obviously "ptype
> int+int" is "int".
>
> In this particular case having to hack whatis-exp.exp to "pass" until
> the general bug is fixed is ok by me - is anyone relying on ptype 4/2
> to be int whereas ptype 4*2 is long?

For completeness' sake, while my pedantic bit is flipped,
One premise of this particular comment is that it assumes one
particular definition of EVAL_AVOID_SIDE_EFFECTS.  If E_A_S_E is
indeed intended to mean to avoid calling error() (which isn't
unreasonable), then that changes the premise.

To make progress here (and those who have worked on gdb longer
probably know the complete definition of E_A_S_E already - they take
it as a given - I can't) it would be nice to at least have a full and
correct definition of EVAL_AVOID_SIDE_EFFECTS at its definition site.

Re: [BUG] BINOP_DIV and ptyp command

['Daniel Jacobowitz']

On Tue, Jan 29, 2008 at 03:41:36PM +0100, Pierre Muller wrote:
> Committed, thanks.
> 
> May I also add a corresponding test?
> 
> testsuite/Changelog entry:
> 
> 2008-01-29  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> 	* gdb.base/gdb1056.exp: Add unsigned integer test.

OK.

-- 
Daniel Jacobowitz
CodeSourcery

RE: [BUG] BINOP_DIV and ptyp command

[Pierre Muller]

Doug, I tend to agree with you that
we should always call value_binop,
or never, not on all cases
except BINOP_DIV, BINOP_INTDIV, BINOP_REM and BINOP_MOD
this is not logical as you explained it.

 The current code makes
those operators listed above act 
differently from other operators for 'ptype' command
and lead to several cases where
(gdb) print EXPR
$? = EXPRESULT
(gdb) ptype $
returns another type than
(gdb) ptype EXPR

  The implicit conversion rules in
value_binop are rather complicated...

Try for instance

(gdb) ptyp 1.5 * 2.5l
type = long double
but 
(top-gdb) p 1.5 / 2.5l
$1 = 0.60000000000000000002168404344971009
(top-gdb) ptyp $
type = long double
(top-gdb) ptyp 1.5 / 2.5l
type = double

But anyhow,
(gdb) ptyp 1 * 2.5
type = double
but 
(top-gdb) p 1 / 2.5
$1 = 0.40000000000000000002
(top-gdb) ptyp $
type = double
(top-gdb) ptyp 1 / 2.5
type = int 
Is obviously a much bigger bug, no?

I don't care much about 'int' 'long int' or 'long long int'
but 'int' and 'double' are completely different story!

Pierre

Re: [BUG] BINOP_DIV and ptyp command

[Daniel Jacobowitz]

I would prefer that we not make type promotion more wrong, so I would
rather not apply your patch until the standard promotions do something
more sensible.

On Tue, Jan 29, 2008 at 08:20:07AM -0800, Doug Evans wrote:
> For completeness' sake, while my pedantic bit is flipped,
> One premise of this particular comment is that it assumes one
> particular definition of EVAL_AVOID_SIDE_EFFECTS.  If E_A_S_E is
> indeed intended to mean to avoid calling error() (which isn't
> unreasonable), then that changes the premise.
> 
> To make progress here (and those who have worked on gdb longer
> probably know the complete definition of E_A_S_E already - they take
> it as a given - I can't) it would be nice to at least have a full and
> correct definition of EVAL_AVOID_SIDE_EFFECTS at its definition site.

Either there's no complete definition of the type you want, or it was
lost before I began working on GDB.  We need to set one.

EVAL_AVOID_SIDE_EFFECTS is checked in many places, but only set in a
few: evaluate_type, to check for user defined operators in
short-circuit expressions, and sizeof.  evaluate_type is only used for
whatis (ptype), maint print type, and the error case for varobjs.

I think that avoiding value-dependent calls to error, when practical,
is more useful in all of those cases.  One way to do it would be to
pass noside further down; but that changes all call sites.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [BUG] BINOP_DIV and ptyp command

[Daniel Jacobowitz]

On Tue, Jan 29, 2008 at 07:48:00AM -0800, Doug Evans wrote:
> As I say, if the intent is to avoid the call to error(), I'd expect to
> see tests for 1 >> 3.0, etc.  That will throw error ("Integer-only
> operation on floating point number").

That's not a value-dependent error.  1 / 0 has a sensible type (it's
an int), but no sensible value; 1 >> 3.0 has no sensible type (it's a
syntax error).

I think of it this way:

drow@caradoc:~% cat a.c
int a = sizeof (1/0);
int b = sizeof (1 >> 3.0);
drow@caradoc:~% gcc -Wall -c a.c
a.c:2: error: invalid operands to binary >>

-- 
Daniel Jacobowitz
CodeSourcery