797222 - Comment desperately needed for fromMarkedLocation in IonCaches.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

The following two lines are very suspicious:

ion/IonCaches.h:        return HandlePropertyName::fromMarkedLocation(&u.bindname.name);
ion/IonCaches.h:        return HandlePropertyName::fromMarkedLocation(&u.name.name);

They may be ok, but only if something either marks or invalidates all of these caches on GC. Either way, this sort of usage of fromMarkedLocation is very dangerous and needs a comment exlaining why it's ok.

Comment 1

[Steve Fink [:sfink] [:s:]]

I can't figure out to dig up the blame for this line. It dates back to a merge, but neither parent of the merge contains the fromMarkedLocation. Unless it was a hand-edit during the merge? dvander?

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Yes, it was probably just some merge bogosity on my part. It is not safe. ICs get reset on preserving GC, and upon invalidation, but that doesn't mean the IC isn't still on the stack, so the fields could still be read.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Also those fields are not traced.

Comment 4

[Steve Fink [:sfink] [:s:]]

Going through ancient bugs, I see 2 fromMarkedLocation calls still there. Maybe Jan can enlighten as to their current validity?

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Steve Fink [:sfink] from comment #4)
> Going through ancient bugs, I see 2 fromMarkedLocation calls still there.
> Maybe Jan can enlighten as to their current validity?

IonCaches.h has this comment:

// Since all the scripts stored in CacheLocations are guaranteed to have been
// Ion compiled, and are kept alive by function objects in jitcode, and since
// the CacheLocations only have the lifespan of the jitcode, there is no need
// to trace or mark any of the scripts. Since JSScripts are always allocated
// tenured, and never moved, we can keep raw pointers, and there is no need
// for HeapPtrScripts here.

This still holds and the same is true for the PropertyName pointers. But it's a bit lame and if compacting GC ever starts moving strings it will break... I wonder if we should just make them HeapPtr* things and trace them.

Comment 6

[Jon Coppeard (:jonco)]

All this code has gone now, so I'm closing the bug.