64 bit windows tamarin vm crashes when sampling api processes samples

VERIFIED FIXED in Future

Status

Tamarin
Virtual Machine
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Dan Schaffer, Assigned: Erik Tierney)

Tracking

unspecified
Future
x86
Windows XP
Bug Flags:
in-testsuite +
wanted-flashplayer10 +
flashplayer-qrb +
flashplayer-triage +

Details

Attachments

(2 attachments)

(Reporter)

Description

10 years ago
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
Flags: wanted-flashplayer10+
Flags: flashplayer-triage+
(Reporter)

Comment 1

10 years ago
Created attachment 338372 [details] [diff] [review]
patch to exclude failing tests
(Reporter)

Comment 2

10 years ago
added patch to exclude the failing tests on 64-bit windows.
(Reporter)

Comment 3

10 years ago
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,...
(Reporter)

Updated

10 years ago
Flags: in-testsuite+
Flags: flashplayer-qrb?

Updated

10 years ago
Assignee: nobody → tierney
Flags: flashplayer-qrb? → flashplayer-qrb+
Target Milestone: --- → Future

Comment 4

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

Comment 5

9 years ago
Created attachment 348249 [details] [diff] [review]
Fix for sampling api on 64 bit.

Just needed a cast to uintptr.

Comment 6

9 years ago
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

9 years ago
(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.
(Assignee)

Comment 8

9 years ago
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
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 9

9 years ago
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)
(Assignee)

Comment 10

9 years ago
That sounds like bug 464957

Comment 11

9 years ago
Hmm, could be, it would probably crash in release mode

Comment 12

9 years ago
Testcase passing, removed the exclude:

changeset:   1151:e3f9c488f99d
user:        Brent Baker <brbaker@adobe.com>
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.