Unqueueing in mark stack overflow handling must check that the item is a GC item

VERIFIED FIXED

Status

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

People

(Reporter: Lars T Hansen, Assigned: Lars T Hansen)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

3.08 KB, patch
Tommy Reilly
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
Otherwise we crash... would have been caught by slightly better placed asserts elsewhere, will fix that too.
(Assignee)

Comment 1

9 years ago
Created attachment 391350 [details] [diff] [review]
Fix
Attachment #391350 - Flags: review?(treilly)

Comment 2

9 years ago
Comment on attachment 391350 [details] [diff] [review]
Fix

MMGC_MEMORY_INFO should always be on if DEBUG is defined, is the bit in MMgc.h that does that not working.  I notice you're using _DEBUG and MMgc.h uses DEBUG
Attachment #391350 - Flags: review?(treilly) → review+
(Assignee)

Comment 3

9 years ago
(In reply to comment #2)
> (From update of attachment 391350 [details] [diff] [review])
> MMGC_MEMORY_INFO should always be on if DEBUG is defined, is the bit in MMgc.h
> that does that not working.  I notice you're using _DEBUG and MMgc.h uses DEBUG

You're right, it's curious that that assert does not trigger, as MMGC_MEMORY_INFO should have been defined.  I now realize what it was: it's not the placement that is wrong, it is the assert: it should assert that item is a pointer to a GC /object/, not a GC /page/.

BTW, since GetRealPointer is a no-op if MMGC_MEMORY_INFO is not defined, is that #ifdef really necessary?  It seems to add mainly clutter.

Re the change in placement of the asserts, I just factored it out because the GetGC call is only needed for the GCAssert, which is only useful if _DEBUG is on.  I like to keep my concerns separated, call me pedantic :-)

Btw MMgc mainly uses _DEBUG, not DEBUG:

$ grep _DEBUG *.cpp *.h | wc -l
      82
$ grep '[^_]DEBUG' *.cpp *.h | wc -l
       6

Anyhow there's code in VMPI.h that makes sure one is defined if the other is defined, so they're equivalent.  Once upon a time I had it in my mind that I could force us to switch to DEBUG but I've decided that my time is better spent elsewhere.  Opinions?
(Assignee)

Comment 4

9 years ago
Created attachment 391584 [details] [diff] [review]
Take 2

Introduces GC::IsPointerToGCObject; gets rid of #ifdefs more generally.  I think this wants a re-review.
Attachment #391350 - Attachment is obsolete: true
Attachment #391584 - Flags: review?(treilly)

Comment 5

9 years ago
Comment on attachment 391584 [details] [diff] [review]
Take 2

Much better, I hate ifdefs!
Attachment #391584 - Flags: review?(treilly) → review+
(Assignee)

Comment 6

9 years ago
redux changeset:   2278:21f88e5cd783
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.