455062 - 64 bit windows tamarin vm crashes when sampling api processes samples

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Dan Schaffer]

in acceptance tests with a release-debugger or debug-debugger on 64 bit windows  run:
./runtests.py as3/sampling

the ProcessingSampling.as and SimpleSampling.as tests crash

both tests output
avmplus crash: exception 0xC0000005 occurred, writing minidump crash to log avmplusCrash.dmp

Comment 1

[Dan Schaffer]

Attachment: patch to exclude failing tests

Comment 2

[Dan Schaffer]

added patch to exclude the failing tests on 64-bit windows.

Comment 3

[Dan Schaffer]

now excluding failing test on 64 bit windows.
probably a low priority, but sampling on 64 bit windows crashes reproducibly.  works ok on other platforms win,linux,mac,...

Comment 4

[Lars T Hansen]

Erik says this does not block the 11/26 merge from TR -> TC as it is already a problem in TC.

Comment 5

[Erik Tierney]

Attachment: Fix for sampling api on 64 bit.

Just needed a cast to uintptr.

Comment 6

[Steven Johnson]

Patch is fine, but the fact that a cast is needed makes me scared that this is a fragile API... wouldn't it be better to have varieties of read() and write() with explicitly-named sizing, rather than relying on overloading to (hopefully) choose the right one?

Comment 7

[Lars T Hansen]

(In reply to comment #6)
> Patch is fine, but the fact that a cast is needed makes me scared that this is
> a fragile API... wouldn't it be better to have varieties of read() and write()
> with explicitly-named sizing, rather than relying on overloading to (hopefully)
> choose the right one?

One the one hand I agree.  The implementation method chosen - templated type inference - lends itself to errors.  The obvious zero-overhead fix would be to make all the "write" method inline functions with explicit signatures, which in turn (for simplicity's sake) could call the current templated write methods, which would be private and renamed in such a way as to make them hard to call accidentally.

On the other hand that fix would not have made any difference in the current case if there were both write(*, uintptr_t) and write(*, uint32_t) methods - the wrong datum would still have been written.  The bug really stems from writing out an external datum in an untyped manner (using native endianness and word sizes as far as I can tell - the samples are not remotely portable?) instead of just pickling a typed data structure.

On the third hand, the protocol used here could do with some documenting.

Comment 8

[Erik Tierney]

The sample stream is not remotely portable, and was never designed to be so.  It is only meant to temporarily store samples until they can be processed by actionscript code.

I added some documentation about the sample format to the Sample struct in Sampler.h

Pushed to TR:  http://hg.mozilla.org/tamarin-redux/rev/2d090531da54

Comment 9

[Steven Johnson]

Hey, with this patch I now get 32-bit Mac Debug builds failing the Callback test with an assertion:

 as3/sampling/Callback.abc : Assertion failed: "((*(int*)obj != 0))" ("/Users/stejohns/tamdev/tr/platform/mac/MMgc/../../../MMgc/GCAlloc.cpp":455)

Comment 10

[Erik Tierney]

That sounds like bug 464957

Comment 11

[Steven Johnson]

Hmm, could be, it would probably crash in release mode

Comment 12

[Brent Baker]

Testcase passing, removed the exclude:

changeset:   1151:e3f9c488f99d
user:        Brent Baker <[e-mail]>
date:        Tue Nov 25 13:15:01 2008 -0500
summary:     Removed the 'skip' for testcases related to the following bugs, I have run the testcases that were skipped on the target platform and everything was passing:https://bugzilla.mozilla.org/show_bug.cgi?id=455062