Add some concreteness/definedness checks #3130

usev6 · 2019-08-20T21:10:18Z

This makes most of S02-names/pseudo-6e.t pass on the JVM backend.

vrurg · 2019-08-21T01:57:01Z

Overall, I don't see a point for this PR. Usually it's good to have safety guards, but in this case they're redundant. Unless I overlook something.

usev6 · 2019-08-21T05:58:52Z

Well, there are failures when running S02-names/pseudo-6e.t on the JVM backend (on HEAD). I'll add details below (should probably have done that before, sorry).

The patches help with the mentioned problems, but I'm totally not sure if the underlying problem is somewhere else -- and my changes are just glossing over that underlying problem. In that case, I should probably open an issue (and close this PR).

The first check for concreteness helps with this code:

HEAD$ ./perl6-j --ll-exception -e 'use v6.e.PREVIEW; my $x = 10; my $z; { $x = [1,2,3]; $LEXICAL::z := $x }; say "alive"'
Cannot do aggregate operation on a type object
  in lookup-ctx (gen/jvm/CORE.e.setting:370)
  in BIND-KEY (gen/jvm/CORE.e.setting:390)
  in postcircumfix:<{ }> (gen/jvm/CORE.setting:14612)
  in postcircumfix:<{ }> (gen/jvm/CORE.setting:14602)
  in <anon> (-e:1)
  in <unit> (-e:1)
  in <unit-outer> (-e:1)
  in eval (gen/jvm/stage2/NQPHLL.nqp:1208)
  in <anon> (gen/jvm/stage2/NQPHLL.nqp:1319)
  in command_eval (gen/jvm/stage2/NQPHLL.nqp:1316)
  in command_eval (src/Perl6/Compiler.nqp:62)
  in command_line (gen/jvm/stage2/NQPHLL.nqp:1300)
  in MAIN (gen/jvm/main.nqp:88)
  in <mainline> (gen/jvm/main.nqp:76)
  in <anon> (gen/jvm/main.nqp)
HEAD$ git checkout pseudo-6e_jvm
pseudo-6e_jvm$ ./perl6-j --ll-exception -e 'use v6.e.PREVIEW; my $x = 10; my $z; { $x = [1,2,3]; $LEXICAL::z := $x }; say "alive"'
alive

The second added check and third added check make this code work. (The third check (for nqp::defined) avoids the NPE, the second check (for nqp::isconcrete) resolves the resulting error 'ctxlexpad requires an operand with REPR ContextRef):

HEAD$ ./perl6-j --ll-exception -e 'use v6.e.PREVIEW; say MY::.values'
java.lang.NullPointerException
  in prefix:<!> (gen/jvm/CORE.setting:18102)
  in next-one (gen/jvm/CORE.e.setting:486)
  in pull-one (gen/jvm/CORE.e.setting:530)
  in push-exactly (gen/jvm/CORE.setting:3020)
  in push-at-least (gen/jvm/CORE.setting:3038)
  in reify-at-least (gen/jvm/CORE.setting:25617)
  in pull-one (gen/jvm/CORE.setting:26187)
  in pull-one (gen/jvm/CORE.setting:11891)
  in join (gen/jvm/CORE.setting:24026)
  in <anon> (gen/jvm/CORE.setting:26415)
  in <anon> (gen/jvm/CORE.setting:1564)
  in <anon> (gen/jvm/CORE.setting:1558)
  in gistseen (gen/jvm/CORE.setting:1554)
  in <anon> (gen/jvm/CORE.setting:1574)
  in gistseen (gen/jvm/CORE.setting:1554)
  in gist (gen/jvm/CORE.setting:26414)
  in gist (gen/jvm/CORE.setting:1578)
  in gist (gen/jvm/CORE.setting:23914)
  in gist (gen/jvm/CORE.setting:1578)
  in say (gen/jvm/CORE.setting:46612)
  in say (gen/jvm/CORE.setting:46610)
  in <unit> (-e:1)
  in <unit-outer> (-e:1)
  in eval (gen/jvm/stage2/NQPHLL.nqp:1208)
  in <anon> (gen/jvm/stage2/NQPHLL.nqp:1319)
  in command_eval (gen/jvm/stage2/NQPHLL.nqp:1316)
  in command_eval (src/Perl6/Compiler.nqp:62)
  in command_line (gen/jvm/stage2/NQPHLL.nqp:1300)
  in MAIN (gen/jvm/main.nqp:88)
  in <mainline> (gen/jvm/main.nqp:76)
  in <anon> (gen/jvm/main.nqp)
HEAD$ git checkout pseudo-6e_jvm
psuedo-6e_jvm$ ./perl6-j --ll-exception -e 'use v6.e.PREVIEW; say MY::.values'
((GLOBAL) (GLOBAL) (Mu) [] (EXPORT) (GLOBAL) (!UNIT_MARKER) Nil (Any) Nil Nil)

vrurg · 2019-08-21T13:26:47Z

Don't close the PR yet. But the problem requires deeper investigation. I don't use JVM backend. It could be peculiarity in it which I don't know about. Though it would be really weird as context cannot be just a type object. Anyway, in this case your patch would perhaps be necessary.

More likely it's another bug in JVM popping up. Then we better don't cover it with a workaround.

Another thing I would like to ask you about is to provide a test for this problem.

vrurg

Current status: needs more investigation on JVM backend. Likely a bug in JVM implementation.

vrurg · 2019-08-21T01:34:41Z

src/core.e/PseudoStash.pm6

@@ -359,7 +359,7 @@ my class PseudoStash is Map {
                nqp::stmts(
                    (my $ctx := nqp::decont(@ctx-info[0])),
                    nqp::if(
-                        nqp::existskey($ctx,nqp::unbox_s($key)),
+                        (nqp::isconcrete($ctx) && nqp::existskey($ctx,nqp::unbox_s($key))),


What's the purpose of this check? $ctx comes from CtxWalker.next-one which would not return null ctx. Instead it returns empty list which is cut off by the enclosing nqp::while condition. I consider this isconcrete check redundant.

I poked some more and I think the underlying problem is this: #1613

The nqp::isnull checks for $!ctx in method next-ctx() don't really work on the JVM backend. I played around with using Mu and changing the checks to nqp::eqaddr($!ctx,Mu) and that seemed to help -- this isconcrete check wasn't necessary anymore.

Since there are a lot of nqp::isnull checks, it doesn't seem feasible to special case for the JVM backend (with #?if jvm). sigh

vrurg · 2019-08-21T01:52:02Z

src/core.e/PseudoStash.pm6

+                            nqp::if(
+                                nqp::isconcrete($!ctx),
+                                ($!iter := nqp::iterator(nqp::ctxlexpad($!ctx)))
+                            )


Similar to the previous: the enclosing nqp::if checks if CtxWalker object has at least one contexts to iterate over. For this reason $!ctx will always be something here.

vrurg · 2019-08-21T01:54:12Z

src/core.e/PseudoStash.pm6

@@ -480,7 +483,7 @@ my class PseudoStash is Map {
                            ($sym := nqp::iterkey_s($!iter)),
                            # The symbol has to be dynamic if pseudo-package is marked as requiring dynamics or if
                            # we'recurrently iterating over the dynamic chain.
-                            ($got-one := !nqp::atkey($!seen,$sym) && (
+                            ($got-one := !nqp::defined(nqp::atkey($!seen,$sym)) && (


What do we win with this? Values in $!seen would either exists and be 1 or not exists. ! would effectively boolify (in NQP terms) both 1 and null.

Well, AFAIK nqp::atkey returns null for the 'not exists' case on the JVM backend. And trying to negate that null results in a NullPointerException:

$ ./perl6-j -e 'use nqp; my $foo := nqp::hash(); my $bar := !nqp::atkey($foo,"foo")' java.lang.NullPointerException in block <unit> at -e line 1

Just adding the nqp::defined looks inprecise, though. Reading your comment ("Values in $!seen would either exists and be 1 or not exists) and the code of next-one() again, wouldn't using nqp::existskey instead of nqp::atkey suffice:

($got-one := !nqp::existskey($!seen,$sym) && ( ...

I was thinking about nqp::existskey it too. Don't think it would ever be necessary to store 0s in $!seen. I'd be ok with it.

But !nqp::null() failing on JVM sounds like a bug to me. Could be a source of many other problems around the core.

But !nqp::null() failing on JVM sounds like a bug to me. Could be a source of many other problems around the core.

That's very true. For the record, it seems to work with NQP:

$ (cd nqp; ./nqp-j -e 'say(!nqp::null)') 1 $ ./perl6-j -e 'use nqp; nqp::say(!nqp::null)' java.lang.NullPointerException in block <unit> at -e line 1

This should be fixed with Raku/nqp@670400e625

No need for s/atkey/existskey/.

usev6 · 2019-08-22T19:27:09Z

I think, it became clear that my proposed changes didn't make much sense. We've identified the underlying problems:

Boolification of nqp::null didn't work on the JVM -- should work after the next NQP bump.
The checks for nqp::isnull (e.g. in next-ctx) don't work on the JVM, cmp. [JVM] a nqp::null bound to an attribute gets hllized #1613 If we want to work around that bug, those checks could be adjusted. For now, I'd go with just opening an issue to document the problem.

If no one objects, I'll close this PR. @vrurg Thanks for looking at this!

vrurg · 2019-08-22T19:53:59Z

Ok, I'm closing. Thank you too!

This avoids explosions in S02-names/pseudo-6e.t (on the JVM backend) that seem to be related to the code in 'method ctx()' in src/core.e/PseudoStash.pm6. (Compare comments in the closed PR rakudo/rakudo#3130.)

Add some concreteness/definedness checks

28e3248

This makes most of S02-names/pseudo-6e.t pass on the JVM backend.

usev6 requested a review from vrurg August 20, 2019 21:10

vrurg reviewed Aug 21, 2019

View reviewed changes

vrurg closed this Aug 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some concreteness/definedness checks #3130

Add some concreteness/definedness checks #3130

usev6 commented Aug 20, 2019

vrurg commented Aug 21, 2019

usev6 commented Aug 21, 2019

vrurg commented Aug 21, 2019

vrurg left a comment

vrurg Aug 21, 2019

usev6 Aug 21, 2019

vrurg Aug 21, 2019

vrurg Aug 21, 2019

usev6 Aug 21, 2019

vrurg Aug 21, 2019

usev6 Aug 21, 2019

usev6 Aug 22, 2019

usev6 commented Aug 22, 2019

vrurg commented Aug 22, 2019

Add some concreteness/definedness checks #3130

Add some concreteness/definedness checks #3130

Conversation

usev6 commented Aug 20, 2019

vrurg commented Aug 21, 2019

usev6 commented Aug 21, 2019

vrurg commented Aug 21, 2019

vrurg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

usev6 commented Aug 22, 2019

vrurg commented Aug 22, 2019