Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmp doesn't works for integers #88

Closed
p6rt opened this issue May 19, 2008 · 14 comments
Closed

cmp doesn't works for integers #88

p6rt opened this issue May 19, 2008 · 14 comments
Labels
Bug

Comments

@p6rt
Copy link

p6rt commented May 19, 2008

Migrated from rt.perl.org#54474 (status was 'resolved')

Searchable as RT54474$
@p6rt
Copy link
Author

p6rt commented May 19, 2008

From @bacek

Parrot's op cmp doesn't work for integers.

$ cat v.pir
.sub main :main
  $P99 = subclass 'Integer', 'Int'

  .local pmc a, b
  a = new 'Int'
  a = 1

  b = new 'Int'
  b = 2

  $I0 = cmp a, b
  say $I0

  $I0 = cmp b, a
  say $I0
.end
$ ./parrot v.pir
1
1
$

Expected results
-1
1

(Thanks to pmichaud for clean version of test case)

@p6rt
Copy link
Author

p6rt commented May 20, 2008

From @bacek

On Mon May 19 14​:10​:05 2008, bacek wrote​:

Parrot's op cmp doesn't work for integers.

I did some investigations. Everything will work, if I'll replace
PMC_int_val(SELF) with VTABLE_get_integer(INTERP, SELF) in
src/pmc/integer.pmc. Just because SELF is not 'Integer' but 'Object'.

(gdb) r v.pir
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program​: /Users/bacek/src/parrot-git/parrot/parrot v.pir
Reading symbols for shared libraries .. done

Breakpoint 3, Parrot_cmp_i_p_p (cur_opcode=0xb23690, interp=0xb09d30) at
cmp.ops​:622
622 $1 = mmd_dispatch_i_pp(interp, $2, $3, MMD_CMP);
(gdb) c
Continuing.

Breakpoint 6, Parrot_Integer_cmp (interp=0xb09d30, pmc=0xd2bbe0,
value=0xd2bb8c) at integer.pmc​:1176
1176 /* int or undef */
(gdb) up
#​1 0x005283f6 in mmd_dispatch_i_pp (interp=0xb09d30, left=0xd2bbe0,
right=0xd2bb8c, func_nr=37) at src/mmd.c​:744
744 return (*real_function)(interp, left, right);
(gdb) p real_function
$20 = (mmd_f_i_pp) 0x60f850 <Parrot_Integer_cmp>
(gdb) p left
$21 = (PMC *) 0xd2bbe0
(gdb) p Parrot_Object_get_integer(interp, left)
$22 = 5
(gdb) p Parrot_Integer_get_integer(interp, left)
$23 = 13810628
(gdb)

@p6rt
Copy link
Author

p6rt commented May 20, 2008

@bacek - Status changed from 'new' to 'open'

@p6rt
Copy link
Author

p6rt commented May 20, 2008

From @bacek

There is a patch for src/pmc/integer.pm to use SELF.get_integer()
instead of PMC_int_val.

@p6rt
Copy link
Author

p6rt commented May 20, 2008

From @bacek

integer.diff 
Index: src/pmc/integer.pmc
===================================================================
--- src/pmc/integer.pmc	(revision 27652)
+++ src/pmc/integer.pmc	(working copy)
@@ -200,7 +200,7 @@
 
 */
     VTABLE FLOATVAL get_number() {
-        return (FLOATVAL)PMC_int_val(SELF);
+        return (FLOATVAL)SELF.get_integer();
     }
 
 /*
@@ -229,11 +229,11 @@
 
 */
     VTABLE STRING *get_string() {
-        return string_from_int(INTERP, PMC_int_val(SELF));
+        return string_from_int(INTERP, SELF.get_integer());
     }
 
     VTABLE STRING *get_repr() {
-        return string_from_int(INTERP, PMC_int_val(SELF));
+        return string_from_int(INTERP, SELF.get_integer());
     }
 
 /*
@@ -413,7 +413,7 @@
         }
 MMD_DEFAULT: {
         VTABLE_set_number_native(INTERP, SELF,
-            PMC_int_val(SELF) + VTABLE_get_number(INTERP, value));
+            SELF.get_integer() + VTABLE_get_number(INTERP, value));
         }
     }
 
@@ -660,7 +660,7 @@
         }
 MMD_DEFAULT: {
         VTABLE_set_number_native(INTERP, SELF,
-            PMC_int_val(SELF) * VTABLE_get_number(INTERP, value));
+            SELF.get_integer() * VTABLE_get_number(INTERP, value));
         }
     }
 
@@ -1136,7 +1136,7 @@
     VTABLE INTVAL is_equal(PMC *value) {
 MMD_BigInt: {
         PMC * const temp = pmc_new(INTERP, enum_class_BigInt);
-        VTABLE_set_integer_native(INTERP, temp, PMC_int_val(SELF));
+        VTABLE_set_integer_native(INTERP, temp, SELF.get_integer());
         return Parrot_BigInt_is_equal_BigInt(INTERP, temp, value);
     }
 MMD_DEFAULT: {
@@ -1157,10 +1157,10 @@
     VTABLE INTVAL cmp(PMC *value) {
 MMD_String: {
             FLOATVAL fdiff =
-                PMC_int_val(SELF) - VTABLE_get_number(INTERP, value);
+                SELF.get_integer() - VTABLE_get_number(INTERP, value);
             if (FLOAT_IS_ZERO(fdiff)) {
                 const INTVAL idiff =
-                    PMC_int_val(SELF) - VTABLE_get_integer(INTERP, value);
+                    SELF.get_integer() - VTABLE_get_integer(INTERP, value);
                 return idiff > 0 ? 1 : idiff < 0 ? -1 : 0;
             }
             else {
@@ -1169,12 +1169,12 @@
         }
 MMD_Float: {
             const FLOATVAL diff =
-                (FLOATVAL)PMC_int_val(SELF) - VTABLE_get_number(INTERP, value);
+                (FLOATVAL)SELF.get_integer() - VTABLE_get_number(INTERP, value);
             return diff > 0 ? 1 : diff < 0 ? -1 : 0;
         }
 MMD_DEFAULT: {
             /* int or undef */
-            const INTVAL diff = PMC_int_val(SELF)
+            const INTVAL diff = SELF.get_integer()
                 - VTABLE_get_integer(INTERP, value);
             return diff > 0 ? 1 : diff < 0 ? -1 : 0;
         }
@@ -1192,11 +1192,11 @@
     VTABLE INTVAL cmp_num(PMC *value) {
 MMD_String: {
             FLOATVAL fdiff =
-                PMC_int_val(SELF) - VTABLE_get_number(INTERP, value);
+                SELF.get_integer() - VTABLE_get_number(INTERP, value);
 
             if (FLOAT_IS_ZERO(fdiff)) {
                 const INTVAL idiff =
-                    PMC_int_val(SELF) - VTABLE_get_integer(INTERP, value);
+                    SELF.get_integer() - VTABLE_get_integer(INTERP, value);
                 return idiff > 0 ? 1 : idiff < 0 ? -1 : 0;
             }
             else {
@@ -1205,13 +1205,13 @@
         }
 MMD_Float: {
             const FLOATVAL diff =
-                (FLOATVAL)PMC_int_val(SELF) - VTABLE_get_number(INTERP, value);
+                (FLOATVAL)SELF.get_integer() - VTABLE_get_number(INTERP, value);
             return diff > 0 ? 1 : diff < 0 ? -1 : 0;
         }
 MMD_DEFAULT: {
             /* int or undef */
             const INTVAL diff =
-                PMC_int_val(SELF) - VTABLE_get_integer(INTERP, value);
+                SELF.get_integer() - VTABLE_get_integer(INTERP, value);
             return diff > 0 ? 1 : diff < 0 ? -1 : 0;
         }
     }
@@ -1308,7 +1308,7 @@
     VTABLE void freeze(visit_info *info) {
         IMAGE_IO * const io = info->image_io;
         SUPER(info);
-        VTABLE_push_integer(INTERP, io, PMC_int_val(SELF));
+        VTABLE_push_integer(INTERP, io, SELF.get_integer());
     }
 
 /*

@p6rt
Copy link
Author

p6rt commented May 20, 2008

From @bacek

Cleaned version of patch.

@p6rt
Copy link
Author

p6rt commented May 20, 2008

From @bacek

integer2.diff 
--- src/pmc/integer.pmc	2008-05-21 00:31:57.000000000 +1000
+++ src/pmc/integer2.pmc	2008-05-21 00:28:32.000000000 +1000
@@ -187,7 +187,7 @@
 
 */
     VTABLE INTVAL get_bool() {
-        return PMC_int_val(SELF) ? 1 : 0;
+        return SELF.get_integer() ? 1 : 0;
     }
 
 /*
@@ -200,7 +200,7 @@
 
 */
     VTABLE FLOATVAL get_number() {
-        return (FLOATVAL)PMC_int_val(SELF);
+        return SELF.get_integer();
     }
 
 /*
@@ -229,11 +229,11 @@
 
 */
     VTABLE STRING *get_string() {
-        return string_from_int(INTERP, PMC_int_val(SELF));
+        return string_from_int(INTERP, SELF.get_integer());
     }
 
     VTABLE STRING *get_repr() {
-        return string_from_int(INTERP, PMC_int_val(SELF));
+        return string_from_int(INTERP, SELF.get_integer());
     }
 
 /*
@@ -413,7 +413,7 @@
         }
 MMD_DEFAULT: {
         VTABLE_set_number_native(INTERP, SELF,
-            PMC_int_val(SELF) + VTABLE_get_number(INTERP, value));
+            SELF.get_integer() + VTABLE_get_number(INTERP, value));
         }
     }
 
@@ -660,7 +660,7 @@
         }
 MMD_DEFAULT: {
         VTABLE_set_number_native(INTERP, SELF,
-            PMC_int_val(SELF) * VTABLE_get_number(INTERP, value));
+            SELF.get_integer() * VTABLE_get_number(INTERP, value));
         }
     }
 
@@ -1136,7 +1136,7 @@
     VTABLE INTVAL is_equal(PMC *value) {
 MMD_BigInt: {
         PMC * const temp = pmc_new(INTERP, enum_class_BigInt);
-        VTABLE_set_integer_native(INTERP, temp, PMC_int_val(SELF));
+        VTABLE_set_integer_native(INTERP, temp, SELF.get_integer());
         return Parrot_BigInt_is_equal_BigInt(INTERP, temp, value);
     }
 MMD_DEFAULT: {
@@ -1157,10 +1157,10 @@
     VTABLE INTVAL cmp(PMC *value) {
 MMD_String: {
             FLOATVAL fdiff =
-                PMC_int_val(SELF) - VTABLE_get_number(INTERP, value);
+                SELF.get_number() - VTABLE_get_number(INTERP, value);
             if (FLOAT_IS_ZERO(fdiff)) {
                 const INTVAL idiff =
-                    PMC_int_val(SELF) - VTABLE_get_integer(INTERP, value);
+                    SELF.get_integer() - VTABLE_get_integer(INTERP, value);
                 return idiff > 0 ? 1 : idiff < 0 ? -1 : 0;
             }
             else {
@@ -1169,12 +1169,12 @@
         }
 MMD_Float: {
             const FLOATVAL diff =
-                (FLOATVAL)PMC_int_val(SELF) - VTABLE_get_number(INTERP, value);
+                SELF.get_number() - VTABLE_get_number(INTERP, value);
             return diff > 0 ? 1 : diff < 0 ? -1 : 0;
         }
 MMD_DEFAULT: {
             /* int or undef */
-            const INTVAL diff = PMC_int_val(SELF)
+            const INTVAL diff = SELF.get_integer()
                 - VTABLE_get_integer(INTERP, value);
             return diff > 0 ? 1 : diff < 0 ? -1 : 0;
         }
@@ -1192,11 +1192,11 @@
     VTABLE INTVAL cmp_num(PMC *value) {
 MMD_String: {
             FLOATVAL fdiff =
-                PMC_int_val(SELF) - VTABLE_get_number(INTERP, value);
+                SELF.get_number() - VTABLE_get_number(INTERP, value);
 
             if (FLOAT_IS_ZERO(fdiff)) {
                 const INTVAL idiff =
-                    PMC_int_val(SELF) - VTABLE_get_integer(INTERP, value);
+                    SELF.get_integer() - VTABLE_get_integer(INTERP, value);
                 return idiff > 0 ? 1 : idiff < 0 ? -1 : 0;
             }
             else {
@@ -1205,13 +1205,13 @@
         }
 MMD_Float: {
             const FLOATVAL diff =
-                (FLOATVAL)PMC_int_val(SELF) - VTABLE_get_number(INTERP, value);
+                SELF.get_number() - VTABLE_get_number(INTERP, value);
             return diff > 0 ? 1 : diff < 0 ? -1 : 0;
         }
 MMD_DEFAULT: {
             /* int or undef */
             const INTVAL diff =
-                PMC_int_val(SELF) - VTABLE_get_integer(INTERP, value);
+                SELF.get_integer() - VTABLE_get_integer(INTERP, value);
             return diff > 0 ? 1 : diff < 0 ? -1 : 0;
         }
     }
@@ -1308,7 +1308,7 @@
     VTABLE void freeze(visit_info *info) {
         IMAGE_IO * const io = info->image_io;
         SUPER(info);
-        VTABLE_push_integer(INTERP, io, PMC_int_val(SELF));
+        VTABLE_push_integer(INTERP, io, SELF.get_integer());
     }
 
 /*

@p6rt
Copy link
Author

p6rt commented May 20, 2008

From @coke

The original problem description is a little off​: this works for core
Integer type, but not for the parrotObject subclass.

I was able to use gdb to break in the function called by the cmp op,
and then step through to find which actual function this was being
sent to via MMD. It was using Integer's 'cmp', in the DEFAULT branch,
which assumes things about the storage of SELF which are apparently
not true.

I suspect that if this is the right fix (see below), that we need to
replace this usage throughout the core PMCs, or subclassing is going
to break in all sorts of odd places.

$ svn diff src
Index​: src/pmc/integer.pmc

--- src/pmc/integer.pmc (revision 27433)
+++ src/pmc/integer.pmc (working copy)
@​@​ -1174,7 +1174,7 @​@​
  }
MMD_DEFAULT​: {
  /* int or undef */
- const INTVAL diff = PMC_int_val(SELF)
+ const INTVAL diff = VTABLE_get_integer(INTERP, SELF)
  - VTABLE_get_integer(INTERP, value);
  return diff > 0 ? 1 : diff < 0 ? -1 : 0;
  }

On Mon, May 19, 2008 at 5​:10 PM, via RT Vasily Chekalkin
<parrotbug-followup@​parrotcode.org> wrote​:

# New Ticket Created by Vasily Chekalkin
# Please include the string​: [perl #​54474]
# in the subject line of all future correspondence about this issue.
# <URL​: http://rt.perl.org/rt3/Ticket/Display.html?id=54474 >

Parrot's op cmp doesn't work for integers.

$ cat v.pir
.sub main :main
$P99 = subclass 'Integer', 'Int'

\.local pmc a, b
a = new 'Int'
a = 1

b = new 'Int'
b = 2

$I0 = cmp a, b
say $I0

$I0 = cmp b, a
say $I0

.end
$ ./parrot v.pir
1
1
$

Expected results
-1
1

(Thanks to pmichaud for clean version of test case)

--
Will "Coke" Coleda

@p6rt
Copy link
Author

p6rt commented May 20, 2008

From @chromatic

On Tuesday 20 May 2008 13​:05​:43 Will Coleda wrote​:

The original problem description is a little off​: this works for core
Integer type, but not for the parrotObject subclass.

I was able to use gdb to break in the function called by the cmp op,
and then step through to find which actual function this was being
sent to via MMD. It was using Integer's 'cmp', in the DEFAULT branch,
which assumes things about the storage of SELF which are apparently
not true.

I suspect that if this is the right fix (see below), that we need to
replace this usage throughout the core PMCs, or subclassing is going
to break in all sorts of odd places.

$ svn diff src
Index​: src/pmc/integer.pmc

--- src/pmc/integer.pmc (revision 27433)
+++ src/pmc/integer.pmc (working copy)
@​@​ -1174,7 +1174,7 @​@​
}
MMD_DEFAULT​: {
/* int or undef */
- const INTVAL diff = PMC_int_val(SELF)
+ const INTVAL diff = VTABLE_get_integer(INTERP, SELF)
- VTABLE_get_integer(INTERP, value);
return diff > 0 ? 1 : diff < 0 ? -1 : 0;
}

This is correct. PMC_*_val must die.

-- c

@p6rt
Copy link
Author

p6rt commented May 20, 2008

From @bacek

Hello.

There is patch for src/pmc/integer.pmc which changes usage of
PMC_int_val to SELF.get_integer() for fetching value.

--
Bacek.

@p6rt
Copy link
Author

p6rt commented May 20, 2008

From @bacek

integer.diff 
Index: src/pmc/integer.pmc
===================================================================
--- src/pmc/integer.pmc	(revision 27652)
+++ src/pmc/integer.pmc	(working copy)
@@ -200,7 +200,7 @@
 
 */
     VTABLE FLOATVAL get_number() {
-        return (FLOATVAL)PMC_int_val(SELF);
+        return (FLOATVAL)SELF.get_integer();
     }
 
 /*
@@ -229,11 +229,11 @@
 
 */
     VTABLE STRING *get_string() {
-        return string_from_int(INTERP, PMC_int_val(SELF));
+        return string_from_int(INTERP, SELF.get_integer());
     }
 
     VTABLE STRING *get_repr() {
-        return string_from_int(INTERP, PMC_int_val(SELF));
+        return string_from_int(INTERP, SELF.get_integer());
     }
 
 /*
@@ -413,7 +413,7 @@
         }
 MMD_DEFAULT: {
         VTABLE_set_number_native(INTERP, SELF,
-            PMC_int_val(SELF) + VTABLE_get_number(INTERP, value));
+            SELF.get_integer() + VTABLE_get_number(INTERP, value));
         }
     }
 
@@ -660,7 +660,7 @@
         }
 MMD_DEFAULT: {
         VTABLE_set_number_native(INTERP, SELF,
-            PMC_int_val(SELF) * VTABLE_get_number(INTERP, value));
+            SELF.get_integer() * VTABLE_get_number(INTERP, value));
         }
     }
 
@@ -1136,7 +1136,7 @@
     VTABLE INTVAL is_equal(PMC *value) {
 MMD_BigInt: {
         PMC * const temp = pmc_new(INTERP, enum_class_BigInt);
-        VTABLE_set_integer_native(INTERP, temp, PMC_int_val(SELF));
+        VTABLE_set_integer_native(INTERP, temp, SELF.get_integer());
         return Parrot_BigInt_is_equal_BigInt(INTERP, temp, value);
     }
 MMD_DEFAULT: {
@@ -1157,10 +1157,10 @@
     VTABLE INTVAL cmp(PMC *value) {
 MMD_String: {
             FLOATVAL fdiff =
-                PMC_int_val(SELF) - VTABLE_get_number(INTERP, value);
+                SELF.get_integer() - VTABLE_get_number(INTERP, value);
             if (FLOAT_IS_ZERO(fdiff)) {
                 const INTVAL idiff =
-                    PMC_int_val(SELF) - VTABLE_get_integer(INTERP, value);
+                    SELF.get_integer() - VTABLE_get_integer(INTERP, value);
                 return idiff > 0 ? 1 : idiff < 0 ? -1 : 0;
             }
             else {
@@ -1169,12 +1169,12 @@
         }
 MMD_Float: {
             const FLOATVAL diff =
-                (FLOATVAL)PMC_int_val(SELF) - VTABLE_get_number(INTERP, value);
+                (FLOATVAL)SELF.get_integer() - VTABLE_get_number(INTERP, value);
             return diff > 0 ? 1 : diff < 0 ? -1 : 0;
         }
 MMD_DEFAULT: {
             /* int or undef */
-            const INTVAL diff = PMC_int_val(SELF)
+            const INTVAL diff = SELF.get_integer()
                 - VTABLE_get_integer(INTERP, value);
             return diff > 0 ? 1 : diff < 0 ? -1 : 0;
         }
@@ -1192,11 +1192,11 @@
     VTABLE INTVAL cmp_num(PMC *value) {
 MMD_String: {
             FLOATVAL fdiff =
-                PMC_int_val(SELF) - VTABLE_get_number(INTERP, value);
+                SELF.get_integer() - VTABLE_get_number(INTERP, value);
 
             if (FLOAT_IS_ZERO(fdiff)) {
                 const INTVAL idiff =
-                    PMC_int_val(SELF) - VTABLE_get_integer(INTERP, value);
+                    SELF.get_integer() - VTABLE_get_integer(INTERP, value);
                 return idiff > 0 ? 1 : idiff < 0 ? -1 : 0;
             }
             else {
@@ -1205,13 +1205,13 @@
         }
 MMD_Float: {
             const FLOATVAL diff =
-                (FLOATVAL)PMC_int_val(SELF) - VTABLE_get_number(INTERP, value);
+                (FLOATVAL)SELF.get_integer() - VTABLE_get_number(INTERP, value);
             return diff > 0 ? 1 : diff < 0 ? -1 : 0;
         }
 MMD_DEFAULT: {
             /* int or undef */
             const INTVAL diff =
-                PMC_int_val(SELF) - VTABLE_get_integer(INTERP, value);
+                SELF.get_integer() - VTABLE_get_integer(INTERP, value);
             return diff > 0 ? 1 : diff < 0 ? -1 : 0;
         }
     }
@@ -1308,7 +1308,7 @@
     VTABLE void freeze(visit_info *info) {
         IMAGE_IO * const io = info->image_io;
         SUPER(info);
-        VTABLE_push_integer(INTERP, io, PMC_int_val(SELF));
+        VTABLE_push_integer(INTERP, io, SELF.get_integer());
     }
 
 /*

@p6rt
Copy link
Author

p6rt commented May 20, 2008

From @bacek

On Tue May 20 13​:18​:36 2008, chromatic@​wgz.org wrote​:

This is correct. PMC_*_val must die.

I've created patch with replacing PMC_int_val with SELF.get_integer().
It's attached to ticket as integer2.diff

--
Bacek.

@p6rt
Copy link
Author

p6rt commented May 21, 2008

From @jnthn

On Tue May 20 15​:08​:28 2008, bacek wrote​:

On Tue May 20 13​:18​:36 2008, chromatic@​wgz.org wrote​:

This is correct. PMC_*_val must die.

I've created patch with replacing PMC_int_val with SELF.get_integer().
It's attached to ticket as integer2.diff

Applied as r27700, thanks.

Jonathan

@p6rt
Copy link
Author

p6rt commented May 21, 2008

@jnthn - Status changed from 'open' to 'resolved'

@p6rt p6rt closed this as completed May 21, 2008
@p6rt p6rt added the Bug label Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@p6rt