Closed Bug 475583 Opened 15 years ago Closed 15 years ago

update MMGC_GET_STACK_EXTENTS for gcc/thumb

Categories

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

ARM
All
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
flash10.1

People

(Reporter: edwsmith, Assigned: treilly)

References

Details

Attachments

(2 files, 1 obsolete file)

the only gcc/arm case that exists uses stmia which isn't thumb compatible.
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)
Flags: flashplayer-triage+
Flags: flashplayer-qrb?
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
Assignee: nobody → treilly
Status: NEW → ASSIGNED
Flags: flashplayer-qrb? → flashplayer-qrb+
Target Milestone: --- → flash10.x
Marking this required for the release, for __thumb__ but not for existing cpu's (more vetting required)
Blocks: 469386
Blocks: 478870
No longer blocks: 469386
Blocks: 481413
No longer blocks: 481413
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?
Attached patch new portable impl (obsolete) — Splinter Review
Attachment #369425 - Flags: review?(edwsmith)
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)
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+
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.
pushed as 1631
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attached patch hotfix for win32Splinter Review
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)
Attachment #369739 - Flags: review?(edwsmith) → review?(stejohns)
Attachment #369739 - Flags: review?(stejohns) → review+
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.

Attachment

General

Creator:
Created:
Updated:
Size: