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.
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.
Created attachment 418660 [details] [diff] [review] patch that does the simple fix as per above comment
Attachment #418660 - Flags: review?(graydon)
Another option would be to only allocate longer-lived guards if LC_FragProfile is true. Julian -- what do you think?
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.
Graydon, review ping.
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.
Attachment #418660 - Flags: review?(graydon) → review+
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.