531350 - TMFLAGS=fragprofile leads to reading freed memory

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Julian Seward [:jseward]]

When landed a couple of months back, the fragment profiler ran
Memcheck-clean, but that is no longer the case.  It now sometimes
reads freed memory and usually asserts or segfaults as a result, which
undermines its usefulness.

This appears to be to do with collecting profile data from
GuardRecords at the end of a run, for traces for which the recorder
aborted, and have already been freed up.  eg:

TMFLAGS=minimal,fragprofile valgrind --smc-check=all \
   ./D32/js -j ../../../SunSpider/tests/access-binary-trees.js

 Invalid read of size 4
    at 0x811D67B: js_FragProfiling_FragFinalizer(nanojit::Fragment*,
                  JSTraceMonitor*) (jstracer.cpp:513)
    by 0x8122D26: js_PurgeScriptFragments(JSContext*, JSScript*) 
                   (jstracer.cpp:7728)
    by 0x80FC469: js_DestroyScript (jsscript.cpp:1773)
    by 0x8057955: JS_DestroyScript (jsapi.cpp:4771)
    by 0x8050874: Process(JSContext*, JSObject*, char*, int) (js.cpp:440)
    by 0x80513F9: ProcessArgs(JSContext*, JSObject*, char**, int) (js.cpp:848)
    by 0x805172B: main (js.cpp:4860)
  Address 0x7a21cdc is 572 bytes inside a block of size 2,004 free'd
    at 0x4C9EB16: free (vg_replace_malloc.c:325)
    by 0x811C390: nanojit::Allocator::freeChunk(void*) (jstracer.cpp:309)
    by 0x814944E: VMAllocator::rewind(VMAllocator::Mark const&) (jstracer.h:585)
    by 0x81494B8: VMAllocator::Mark::~Mark() (jstracer.h:576)
    by 0x813D26E: TraceRecorder::~TraceRecorder() (jstracer.cpp:2488)
    by 0x813D462: TraceRecorder::finishAbort(char const*) (jstracer.cpp:2570)
    by 0x813D4FC: js_AbortRecordingImpl(JSContext*, char const*)
                  (jstracer.cpp:7193)
    by 0x814487E: TraceRecorder::attemptTreeCall(TreeFragment*,
                  unsigned int&) (jstracer.cpp:6034)
    by 0x8145103: TraceRecorder::record_EnterFrame(unsigned int&)
                  (jstracer.cpp:9818)
    by 0x81882F3: js_Interpret (jsops.cpp:2233)
    by 0x80A4000: js_Execute (jsinterp.cpp:1619)
    by 0x80571C1: JS_ExecuteScript (jsapi.cpp:4948)

One "fix" is (or appears to be) to change 
TraceRecorder::createGuardRecord(VMSideExit* exit)
to allocate the new GuardRecords in dataAlloc rather than traceAlloc.

This stops Memcheck complaining, and gets rid of assertion failures
and segfaults inside js_FragProfiling_FragFinalizer, so the profiler
is usable.  But I doubt it is a proper fix.

Comment 1

[Graydon Hoare :graydon]

Yeah, doing this unconditionally is bad since we generate a ton of guards (more than almost any other structure) and a lot of them are thrown out when we rewind. Enough that this change will seriously decrease the proportion of live data in the trace cache. Two options would work though:

One is to finalize the fragments earlier (when the traceAlloc is rewound) by keeping a log if the fragments allocated in it. You rewind this list by finalizing everything on it, and commit the list by forgetting it. Would be contained within the VMAllocator::Mark object or similar.

Another (simpler) option is to ifdef-DEBUG the choice of whether to store guards in the traceAlloc or the dataAlloc; IOW only apply your proposed fix when there's a fragment profiler around to care. A helpful comment explaining the ifdef would be required, but I think this would be ... possibly acceptable. It'll change memory behavior when built DEBUG, but I'm pretty sure we already change substantially due to all the debugging-output support structures we put in those allocators.

Comment 2

[Julian Seward [:jseward]]

Attachment: patch that does the simple fix as per above comment

Comment 3

[Nicholas Nethercote [inactive]]

Another option would be to only allocate longer-lived guards if LC_FragProfile is true.  Julian -- what do you think?

Comment 4

[Julian Seward [:jseward]]

Fine by me.  The only reason I didn't gate it on LC_FragProfile is
that is gives 3 configurations to verify (as memcheck-clean): release,
debug w/o LC_FragProfile, and debug w/ LC_FragProfile
rather than just 2.

Comment 5

[Julian Seward [:jseward]]

Graydon, review ping.

Comment 6

[Graydon Hoare :graydon]

Comment on attachment 418660 [details] [diff] [review]
patch that does the simple fix as per above comment

Oh goodness, sorry for letting that go. Yes, this is fine by me.

Comment 7

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/f909e6aacc87

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/f909e6aacc87