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

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

Open
usev6 opened this issue May 21, 2018 · 2 comments
Open

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

usev6 opened this issue May 21, 2018 · 2 comments
Labels

Comments

@usev6
Copy link
Contributor

usev6 commented May 21, 2018

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 usev6 added the JVM label May 21, 2018
@usev6
Copy link
Contributor Author

usev6 commented May 21, 2018

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
Copy link
Contributor Author

usev6 commented Jan 22, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant