565072 - PoolObject::getLegalDefaultValue looks in wrong list for CONSTANT_UInt

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[Steven Johnson]

It looks in cpool_int rather than cpool_uint. (This only affects code that declares a function with a uint-typed arg with a default value.)

injection in argo, code was restructured since 10.0

Comment 1

[Steven Johnson]

Attachment: Patch

Routing review to Werner since Edwin is out

Comment 2

[Steven Johnson]

http://asteam.macromedia.com/hg/tamarin-redux-argo/rev/ab1d421d08b5

(leaving open for Edwin to sr after the fact)

Comment 3

[Brent Baker]

Steven does this require a testcase and is it possible to test this via AS code? I was playing around with code that was like this, but this did not appear to be executing hitting PoolObject.getLegalDefaultValue():case CONSTANT_UInt:

public static function foobar(bar:uint=12){
    trace(bar);
}

Also tried with:
public static const value:uint = 22;
public static function foobar(bar:uint=MyClass.value){
    trace(bar);
}

Comment 4

[Steven Johnson]

It is possible but challenging: you have the right idea, but you'd need to have an ABC with lots and lots of uints (and the one you want to use near the end of the pool) and very few ints. abcasm is probably the simplest way to ensure this.

Comment 5

[Edwin Smith]

A test case with abcasm shouldn't be very hard.  If abcasm can't express OP_pushuint with an illegal cpool index operand, then we should enhance abcasm.

Comment 6

[Brent Baker]

(In reply to comment #5)
> A test case with abcasm shouldn't be very hard.  If abcasm can't express
> OP_pushuint with an illegal cpool index operand, then we should enhance abcasm.

I don't really see how to use pushuint in abcasm with specified cpool index, I can only see how to "pushuint <value>" (or how one would setup a cpool entry). TomH do you have any ideas on how I could test this?

Comment 7

[Tom Harwood]

(In reply to comment #5)
>  If abcasm can't express
> OP_pushuint with an illegal cpool index operand, then we should enhance abcasm.

That is the current situation, yes (can't expression OP_pushuint with invalid cpool index).
Have you tried using the hex-opcode option?
 0x2e 9999

Comment 8

[Brent Baker]

Not sure exactly if this testing is correct for this patch but this is what I have tried:

function main() {
    getlocal0
    pushscope

    pushint 11111
    0x2e 0

    returnvoid
}

This simply returns a verifier error with and without this patch:
    VerifyError: Error #1032: Cpool index 1 is out of range 1.

Since this runs the same with and without the change I do not believe that this is testing what was expected.

Comment 9

[Tom Harwood]

This might be an ASC error.  Compiled:

function foobar(bar:uint=12){
    trace(bar);
}
foobar();

abcdump -pools shows me:

$abcdump -pools 565072.abc
// magic 2e0010
ints:2
// int[0] = 0
// int[1] = 12

So the compiler put the default value in the int pool.

I think what Steven was suggesting was a TC that did lots of valid pushuint instructions (with a lot of different values) and had a function parameter with a uint value that matched one of the last uints pushed.  You would probably have to do the pushing in function A and then set the default in function B.  That would work if abcasm supported default values in its trait entries, which it currently doesn't.  You could log an enhancement request for that.

Comment 10

[Brent Baker]

testcase generation is currently blocked by abcasm bug# 567809 (ability to
specify default value for method traits)