t/pmc/packfileconstanttable.t failure

[coke]

Both via smolders:

 http://smolder.plusthree.com/app/public_projects/report_details/18409

and on my OS X/x86 box:

[10:08:20] t/pmc/packfileconstanttable....
1..3
ok 1 - sanity
ok 2 - elements
not ok 3 - get_type, get_*_keyed_int

#   Failed test 'get_type, get_*_keyed_int'
#   at t/pmc/packfileconstanttable.t line 92.
# Exited with error code: 1
# Received:
# Requested constant of the wrong type.
# current instr.: 'test' pc 79 (/Users/coke/research/parrot-pure/t/pmc/packfileconstanttable_3.pir:26)
#
# Expected:
# done.
#
# Looks like you failed 1 test of 3.
 Dubious, test returned 1 (wstat 256, 0x100)
 Failed 1/3 subtests
[10:08:21]

Test Summary Report
-------------------
t/pmc/packfileconstanttable (Wstat: 256 Tests: 3 Failed: 1)
  Failed test:  3
  Non-zero exit status: 1
Files=1, Tests=3,  1 wallclock secs ( 0.03 usr  0.01 sys +  0.14 cusr  0.08 csys =  0.26 CPU)
Result: FAIL

[jkeenan]

Confirmed on Linux/i386 at r37055. See  this Smolder report.

All tests were passing as of r37039.

[jkeenan]

Replying to jkeenan:

Confirmed on Linux/i386 at r37055. See  this Smolder report. All tests were passing as of r37039.

And the test in question was passing as of r37041 on the same box.

[jkeenan]

Replying to jkeenan:

And the test in question was passing as of r37041 on the same box.

Getting a failure at r37044:

t/pmc/packfileconstanttable....
1..3
ok 1 - sanity
ok 2 - elements
not ok 3 - get_type, get_*_keyed_int

#   Failed test 'get_type, get_*_keyed_int'
#   at t/pmc/packfileconstanttable.t line 92.
# Exited with error code: 1
# Received:
# Requested constant of the wrong type.
# current instr.: 'test' pc 79 (/home/jimk/work/37044/t/pmc/packfileconstanttable_3.pir:26)
# 
# Expected:
# done.
# 
# Looks like you failed 1 test of 3.
 Dubious, test returned 1 (wstat 256, 0x100)
 Failed 1/3 subtests 

Test Summary Report
-------------------
t/pmc/packfileconstanttable (Wstat: 256 Tests: 3 Failed: 1)
  Failed test:  3
  Non-zero exit status: 1
Files=1, Tests=3,  1 wallclock secs ( 0.01 usr  0.00 sys +  0.12 cusr  0.01 csys =  0.14 CPU)
Result: FAIL

[jkeenan]

Replying to jkeenan:

I got that test to pass at r37042. Given that r37043 dealt with Parrotbug, that makes me quite certain that the bug crept in in r37044.

However, I wonder whether the problem may be that the test was not updated properly when François began to yank the Random PMC (rather than the yanking itself). Here's what was done in r37044:

$ svn diff -r 37043:37044 .
Index: runtime/parrot/library/uuid.pir
===================================================================
--- runtime/parrot/library/uuid.pir     (revision 37043)
+++ runtime/parrot/library/uuid.pir     (revision 37044)
@@ -1,4 +1,4 @@
-# Copyright (C) 2008, Parrot Foundation.
+# Copyright (C) 2008-2009, Parrot Foundation.
 # $Id$
 
 =head1 NAME
@@ -22,6 +22,7 @@
 .namespace ['uuid']
 
 .sub '__onload' :anon :load :init
+    load_bytecode 'Math/Rand.pbc'
     $P0 = subclass 'FixedIntegerArray', 'uuid'
 .end
 
@@ -96,16 +97,23 @@
     .local pmc res
     new res, 'uuid'
     set res, N
-    new $P0, 'Random'
+    .local pmc rand
+    rand = get_hll_global [ 'Math'; 'Rand' ], 'rand'
+    .local pmc srand
+    srand = get_hll_global [ 'Math'; 'Rand' ], 'srand'
+    .local int RAND_MAX
+    $P0 = get_hll_global [ 'Math'; 'Rand' ], 'RAND_MAX'
+    RAND_MAX = $P0()
+    inc RAND_MAX
     time $I0    # less than enough entropy
-    set $P0, $I0
+    srand($I0)
     .local int i
     i = 0
   L1:
     unless i < N goto L2
-    $N0 = $P0
-    $N0 *= 256
-    $I0 = floor $N0
+    $I0 = rand()
+    $I0 *= 256
+    $I0 /= RAND_MAX
     res[i] = $I0
     inc i
     goto L1
@@ -133,16 +141,23 @@
     .local pmc res
     new res, 'uuid'
     set res, N
-    new $P0, 'Random'
-    time $I0    # less than enough entropy
-    set $P0, $I0
+    .local pmc rand
+    rand = get_hll_global [ 'Math'; 'Rand' ], 'rand'
+    .local pmc srand
+    srand = get_hll_global [ 'Math'; 'Rand' ], 'srand'
+     .local int RAND_MAX
+    $P0 = get_hll_global [ 'Math'; 'Rand' ], 'RAND_MAX'
+    RAND_MAX = $P0()
+    inc RAND_MAX
+   time $I0    # less than enough entropy
+    srand($I0)
     .local int i
     i = 10
   L1:
     unless i < N goto L2
-    $N0 = $P0
-    $N0 *= 256
-    $I0 = floor $N0
+    $I0 = rand()
+    $I0 *= 256
+    $I0 /= RAND_MAX
     res[i] = $I0
     inc i
     goto L1

And here's the failing test:

pir_output_is( <<'CODE' . $get_uuid_pbc, <<'OUT', 'get_type, get_*_keyed_int' );
.sub 'test' :main
    .local pmc pf, pfdir, pftable
    .local int size, this, type
    pf      = _pbc()
    pfdir   = pf.'get_directory'()
    pftable = pfdir[2]
    size    = elements pftable
    this    = 0
    LOOP:
    type = pftable.'get_type'(this)
    eq type, 0x00, NEXT
    eq type, 0x6E, CONST_NUM
    eq type, 0x73, CONST_STR
    eq type, 0x70, CONST_PMC
    eq type, 0x6B, CONST_KEY
    goto BAD
    CONST_NUM:
    $N0 = pftable[this]
    goto NEXT
    CONST_STR:
    $S0 = pftable[this]
    goto NEXT
    CONST_PMC:
    $P0 = pftable[this]
    goto NEXT
    CONST_KEY:
    $P0 = pftable[this]
    $S0 = typeof $P0
    eq $S0, 'Key', NEXT
    print 'constant Key with wrong type: '
    say $S0
    goto BAD
    NEXT:
    this = this + 1
    ge this, size, DONE
    goto LOOP
    gt size, 0, DONE
    BAD:
    say 'unknown constant type found!'
    DONE:
    say 'done.'
.end
CODE
done.
OUT

[rg]

Replying to jkeenan:

Given that r37043 dealt with Parrotbug, that makes me quite certain that the bug crept in in r37044. However, I wonder whether the problem may be that the test was not updated properly when François began to yank the Random PMC (rather than the yanking itself).

Actually I think the test was never fully exercised. After the change in r37044 the uuid.pmc now seems to contain a type Key pftable element, which can't be retrieved. Instead of autoboxing, the packfileconstanttable.pmc just throws an exception. I have no idea how to fix that (other than not checking type Key elements at all).

[coke]

Test TODO'd in r37079.

[Infinoid]

Replying to rg:

Actually I think the test was never fully exercised. After the change in r37044 the uuid.pmc now seems to contain a type Key pftable element, which can't be retrieved. Instead of autoboxing, the packfileconstanttable.pmc just throws an exception. I have no idea how to fix that (other than not checking type Key elements at all).

Yep. PDD13 says it will throw an exception if the constant at the given offset isn't of the requested type, so autoconverting would be wrong.

=item * C<get_number_keyed_int> (v-table)

Gets the value of the number constant at the specified index in the constants
table. If the constant at that position in the table is not a number, an
exception will be thrown.

(And similar text, for all of the other type accessors.) So I think the PackfileConstantTable class is doing the right thing here.

The real issue is the test. When I wrote these tests, I chose a pbc file at random which didn't seem likely to change (uuid.pbc). So of course, when it got modified and the items in its constant table were no longer the same, the test blew up. Oops.

To fix this properly, the test should use a pbc file whose contents are known (and guaranteed to never change). It should probably generate this pbc file itself.

(Note that some of the other t/pmc/packfile*.t tests also use uuid.pbc for testing. Having each test generate its own pbc test file is probably better than using a common one, as a common one will cause race conditions with parallel testing.)

[rg]

Replying to Infinoid:

Yep. PDD13 says it will throw an exception if the constant at the given offset isn't of the requested type, so autoconverting would be wrong.

Not quite. Here's the description for the failing case:

=item * C<get_pmc_keyed_int> (v-table)

Gets the value of the PMC or key constant at the specified index in the
constants table. If the constant at that position in the table is not a PMC
or key, an exception will be thrown.

So I think the PackfileConstantTable class is doing the right thing here.

I think it's missing the "or key constant" part. The failing case is the one accessing the key constant, but the lvalue, being a PMC, is not accepted as the right type. Since there is no such thing as a "key" register, autoconverting (if that's even necessary) is the only possibility.

To fix this properly, the test should use a pbc file whose contents are known (and guaranteed to never change). It should probably generate this pbc file itself.

I think this is exactly what the test was supposed to check. And while I agree that a pbc file whose contents are known to fully exercise the test and api would be preferable, it should at least work with any pbc file.

[rg]

Replying to rg:

Since there is no such thing as a "key" register, autoconverting (if that's even necessary) is the only possibility.

This actually made me curious. Since I've only encountered a key PMC, I thought, maybe it really doesn't need conversion. And it seems to work. This simple change makes the test pass again:

--- src/pmc/packfileconstanttable.pmc	(revision 37330)
+++ src/pmc/packfileconstanttable.pmc	(working copy)
@@ -34,7 +34,8 @@
         Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_OUT_OF_BOUNDS,
                 "Requested data out of range.");
     rv = table->constants[index];
-    if (rv->type != type)
+    if (rv->type != type &&
+        !(rv->type == PFC_KEY && type == PFC_PMC))
         Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_INVALID_OPERATION,
                 "Requested constant of the wrong type.");
     return rv;

(I hope you agree that putting this patch inline is actually easier to use)

Could somebody a lot more knowledgeable with Parrot and Keys please check if that's really the whole problem? FWIW it's not breaking make test and fixing the given problem for me.

[Infinoid]

Replying to rg:

(I hope you agree that putting this patch inline is actually easier to use)

(A normal attachment is easier for me to import into my patch stack, but I can make it work either way.)

Thanks, you're right that keys aren't handled properly and that they should be. However, it still isn't the full solution, as the tests should generate their own pbc files to test against. (And those test files should include some keys, of course.) Currently relying on random library pbcs (which can be validly changed for other reasons) makes the tests brittle, and it also means we can't test specific attributes like the number of opcodes and the pbc version number and things like this. These tests were a good starting point for verifying my code works, but their coverage is poor and I need to improve them quite a bit.

Could somebody a lot more knowledgeable with Parrot and Keys please check if that's really the whole problem? FWIW it's not breaking make test and fixing the given problem for me.

Yes, that's the whole problem, your patch brings the method in line with what pdd13 specifies. But to me all of this stuff just points to the larger issue of needing better (more targeted) testing, which is why I want to leave this ticket open until I've dealt with that issue.

Mark

[Infinoid]

Since the last commentry on this ticket, the packfile PMCs have evolved quite a bit, thanks to bacek++'s hard work. The test no longer fails, the packfile tests in general have been rewritten to adapt to changing pbc formats and be less rigid in general, and the code rg++ suggested patching has been completely replaced (and I believe no longer has the issue in question). So I believe it's safe to resolve this ticket.

Thanks, everyone!