nqp icon indicating copy to clipboard operation
nqp copied to clipboard

[JVM] nqp::attrinited does not work for natives

Open usev6 opened this issue 6 years ago • 2 comments

The nqp::attrinited op always returns 1 for native attributes on the JVM backend.

<bartolin> r: use nqp; class Foo { has int $.foo }; my $a := Foo.new( ); say nqp::attrinited($a, Foo,"\$!foo")
<camelia> rakudo-moar 4cbb0fa40: OUTPUT: «0␤»
<camelia> ..rakudo-jvm 4cbb0fa40: OUTPUT: «1␤»

This also leads to trait 'is required' not failing if a native attribute is not initialized. (That problem leads to a failing spectest, btw: https://github.com/perl6/roast/blob/d86f648fb87fcf9df457c7aaffd804cdfb64249c/S11-compunit/compunit-dependencyspecification.t#L6).

<bartolin> r: use nqp; class Foo { has str $.foo is required }; my $a := Foo.new( ); say "alive"
<camelia> rakudo-moar 4cbb0fa40: OUTPUT: «The attribute '$!foo' is required, but you did not provide a value for it.␤  in submethod BUILDALL at <tmp> line 1␤  in block <unit> at <tmp> line 1␤␤»
<camelia> ..rakudo-jvm 4cbb0fa40: OUTPUT: «alive␤»

usev6 avatar May 21 '18 19:05 usev6

For the record: I tried to make 'is required' work with native attributes, but didn't really find a solution. Maybe my findings are usefull for someone else (or for a later myelf), so I'm writing down my results.

For native strings, the following patch seems to help (didn't run full spectest):

diff --git a/src/vm/jvm/runtime/org/perl6/nqp/sixmodel/reprs/P6Opaque.java b/src/vm/jvm/runtime/org/perl6/nqp/sixmodel/reprs/P6Opaque.java
index 0ee13ed6a..efb3fc63c 100644
--- a/src/vm/jvm/runtime/org/perl6/nqp/sixmodel/reprs/P6Opaque.java
+++ b/src/vm/jvm/runtime/org/perl6/nqp/sixmodel/reprs/P6Opaque.java
@@ -500,6 +500,12 @@ public class P6Opaque extends REPR {
                 
                 /* Add is init code. */
                 isInitVisitor.visitLabel(isInitLabels[i]);
+                /* Is native string initialized? */
+                if ((ss.can_box & StorageSpec.CAN_BOX_STR) != 0) {
+                    isInitVisitor.visitVarInsn(Opcodes.ALOAD, 0);
+                    isInitVisitor.visitFieldInsn(Opcodes.GETFIELD, className, prefix, "Ljava/lang/String;");
+                    isInitVisitor.visitJumpInsn(Opcodes.IFNULL, isInitNull);
+                }
                 isInitVisitor.visitInsn(Opcodes.ICONST_1);
                 isInitVisitor.visitInsn(Opcodes.I2L);
                 isInitVisitor.visitInsn(Opcodes.LRETURN);

AFAIU the problem with native ints and nums is, that instances of long or double in Java get a default value of 0. It's not possible to use an IFNULL check as above and testing for a value of zero would give wrong results if the attribute is really initialized with a value of zero.

diff --git a/src/vm/jvm/runtime/org/perl6/nqp/sixmodel/reprs/P6Opaque.java b/src/vm/jvm/runtime/org/perl6/nqp/sixmodel/reprs/P6Opaque.java
index 0ee13ed6a..f17f2534d 100644
--- a/src/vm/jvm/runtime/org/perl6/nqp/sixmodel/reprs/P6Opaque.java
+++ b/src/vm/jvm/runtime/org/perl6/nqp/sixmodel/reprs/P6Opaque.java
@@ -500,6 +500,24 @@ public class P6Opaque extends REPR {
 
                 /* Add is init code. */
                 isInitVisitor.visitLabel(isInitLabels[i]);
+// doesn't work since instances of int and double get a default value of 0
+//              if ((ss.can_box & StorageSpec.CAN_BOX_INT) != 0) {
+//                  isInitVisitor.visitVarInsn(Opcodes.ALOAD, 0);
+//                  isInitVisitor.visitFieldInsn(Opcodes.GETFIELD, className, prefix, "J");
+//                  isInitVisitor.visitInsn(Opcodes.L2I);
+//                  isInitVisitor.visitJumpInsn(Opcodes.IFEQ, isInitNull);
+//              }
+//              else if ((ss.can_box & StorageSpec.CAN_BOX_NUM) != 0) {
+//                  isInitVisitor.visitVarInsn(Opcodes.ALOAD, 0);
+//                  isInitVisitor.visitFieldInsn(Opcodes.GETFIELD, className, prefix, "D");
+//                  isInitVisitor.visitInsn(Opcodes.D2I);
+//                  isInitVisitor.visitJumpInsn(Opcodes.IFEQ, isInitNull);
+//              }
+                if ((ss.can_box & StorageSpec.CAN_BOX_STR) != 0) {
+                    isInitVisitor.visitVarInsn(Opcodes.ALOAD, 0);
+                    isInitVisitor.visitFieldInsn(Opcodes.GETFIELD, className, prefix, "Ljava/lang/String;");
+                    isInitVisitor.visitJumpInsn(Opcodes.IFNULL, isInitNull);
+                }
                 isInitVisitor.visitInsn(Opcodes.ICONST_1);
                 isInitVisitor.visitInsn(Opcodes.I2L);
                 isInitVisitor.visitInsn(Opcodes.LRETURN);

usev6 avatar May 21 '18 19:05 usev6

As a status update: The test in S11-compunit/compunit-dependencyspecification.t is passing now, but the code examples from the original report are still working differently on the JVM backend.

usev6 avatar Jan 22 '22 15:01 usev6