update MMGC_GET_STACK_EXTENTS for gcc/thumb

VERIFIED FIXED in flash10.1

Status

Tamarin
Garbage Collection (mmGC)
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: Edwin Smith, Assigned: Tommy Reilly)

Tracking

unspecified
flash10.1
ARM
All
Bug Flags:
flashplayer-qrb +
flashplayer-triage +

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
the only gcc/arm case that exists uses stmia which isn't thumb compatible.
(Reporter)

Comment 1

9 years ago
from an email thread:

This is an easy change.  Can I talk you into submitting it to the open source repository?  Assuming my file is very similar to the current open source file, then around line 213 of GC.h you will find the gcc definition of of MMGC_GET_STACK_EXTENTS.  The gcc symbol you need to look for is __thumb__ and the register set you want to save is r4-r6.  So my file now has:

#ifdef __thumb__
// Store nonvolatile registers r4-r6
// Find stack pointer
#define MMGC_GET_STACK_EXTENTS(_gc, _stack, _size) \
int regs[3];\
asm("stmia %0,{r4-r6}" : : "r" (regs));\
asm("mov %0,sp" : "=r" (_stack));\
_size = (uintptr)_gc->GetStackTop() - (uintptr)_stack;
#else
// Store nonvolatile registers r4-r10
// Find stack pointer
#define MMGC_GET_STACK_EXTENTS(_gc, _stack, _size) \
int regs[7];\
asm("stmia %0,{r4-r10}" : : "r" (regs));\
asm("mov %0,sp" : "=r" (_stack));\
_size = (uintptr)_gc->GetStackTop() - (uintptr)_stack;
#endif // not _ARMCC__ (so gcc) and not thumb (so arm)

Updated

9 years ago
Flags: flashplayer-triage+
Flags: flashplayer-qrb?
(Reporter)

Comment 2

9 years ago
In gcc’s thumb build the implementation of setjmp is an arm (not thumb) routine and copies all of the registers into the jmp_buf.  So it seems like a perfectly good and portable definition of GET_STACK_EXTENTS would be something like:

#define MMGC_GET_STACK_EXTENTS( _gc, _stack, _size ) \

        jmp_buf jb; \

        setjmp( jb ); \

        _size = (uintpr)_gc->GetStackTop() – (uintptr)&jb;

And then you could get rid of the “_stack” parameter and probably have just a single implementation of it for all platforms.

Paul

Updated

9 years ago
Assignee: nobody → treilly
Status: NEW → ASSIGNED
Flags: flashplayer-qrb? → flashplayer-qrb+
Target Milestone: --- → flash10.x
(Reporter)

Comment 3

9 years ago
Marking this required for the release, for __thumb__ but not for existing cpu's (more vetting required)
Blocks: 469386
(Reporter)

Updated

9 years ago
Blocks: 478870
No longer blocks: 469386

Updated

9 years ago
Blocks: 481413

Updated

9 years ago
No longer blocks: 481413
(Assignee)

Comment 4

9 years ago
I'm actually using the setjmp approach in my FlashRuntime branch and its working great on win/mac/linux so far.  Question is how do we test it on thumb?
(Assignee)

Comment 5

9 years ago
Created attachment 369425 [details] [diff] [review]
new portable impl
Attachment #369425 - Flags: review?(edwsmith)
(Assignee)

Comment 6

9 years ago
Created attachment 369428 [details] [diff] [review]
new portable impl of MMGC_GET_STACK_EXTENTS

Same as last patch but includes winbuild.h which disables an assert needed for MSVC
Attachment #369425 - Attachment is obsolete: true
Attachment #369428 - Flags: review?(edwsmith)
Attachment #369425 - Flags: review?(edwsmith)
(Reporter)

Comment 7

9 years ago
Comment on attachment 369428 [details] [diff] [review]
new portable impl of MMGC_GET_STACK_EXTENTS

I don't understand the motivation for GCAutoExit, we need comments for that.  but i cant find any bugs, so R+ and lets unblock the release.
Attachment #369428 - Flags: review?(edwsmith) → review+
(Assignee)

Comment 8

9 years ago
MMGC_GCENTER macros uses it (see avmshell.cpp), it defines the stack entry point.  This is why all the platform specific code to find the start of the stack went away.
(Assignee)

Comment 9

9 years ago
pushed as 1631
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

9 years ago
Created attachment 369739 [details] [diff] [review]
hotfix for win32

win32 was storing callee saved regs into an area of the stack above the jmp_buf (ie lower address) in ZCT::Reap.  So the MMGC_GET_STACK_EXTENTS macro window missed this value which caused live objects to be collected.
Attachment #369739 - Flags: review?(edwsmith)
(Assignee)

Updated

9 years ago
Attachment #369739 - Flags: review?(edwsmith) → review?(stejohns)

Updated

9 years ago
Attachment #369739 - Flags: review?(stejohns) → review+

Comment 11

9 years ago
Resolved fixed engineering / work item that has been pushed.  Setting status to verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.