Gives warnings like the following, where the key will be garbage before an element is introduced. ==13821== 4 errors in context 6 of 917: ==13821== Conditional jump or move depends on uninitialised value(s) ==13821== at 0x43B57C9: JSC::NumericStrings::add(int) (NumericStrings.h:52) ==13821== by 0x43B67BE: JSC::JSValue::toString(JSC::ExecState*) const (JSString.h:595) ==13821== by 0x51578C8: JSC::jsAddSlowCase(JSC::ExecState*, JSC::JSValue, JSC::JSValue) (Operations.cpp:56) ==13821== by 0x50B1482: cti_op_add (JITStubs.cpp:1342) ==13821== by 0x50B0B4F: JSC::JITThunks::tryCacheGetByID(JSC::ExecState*, JSC::CodeBlock*, JSC::ReturnAddressPtr, JSC::JSValue, JSC::Identifier const&, JS$ ==13821== by 0x5080B09: JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*) (JITCode.h:77) ==13821== by 0x507D8DE: JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::ScopeChainNode*, JSC::JSObject*) (Interpreter.cpp:778) ==13821== by 0x5113D96: JSC::evaluate(JSC::ExecState*, JSC::ScopeChain&, JSC::SourceCode const&, JSC::JSValue) (Completion.cpp:62) ==13821== by 0x43FDE84: WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::ScopeChain&, JSC::SourceCode const&, JSC::JSValue) (JSMainThreadEx$ ==13821== by 0x441C67C: WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*, WebCore::ShouldAllowXSS) $ ==13821== by 0x441C816: WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&, WebCore::ShouldAllowXSS) (ScriptController.cpp:171) ==13821== by 0x44459EB: WebCore::ScriptController::executeScript(WebCore::ScriptSourceCode const&, WebCore::ShouldAllowXSS) (ScriptControllerBase.cpp:60) ==13821== Uninitialised value was created by a heap allocation ==13821== at 0x4025BDC: malloc (vg_replace_malloc.c:195) ==13821== by 0x517F063: WTF::fastMalloc(unsigned int) (FastMalloc.cpp:250) ==13821== by 0x4378915: WTF::FastAllocBase::operator new(unsigned int) (FastAllocBase.h:121) ==13821== by 0x5135C1F: JSC::JSGlobalData::create(JSC::ThreadStackType) (JSGlobalData.cpp:237) ==13821== by 0x5135C8C: JSC::JSGlobalData::createLeaked(JSC::ThreadStackType) (JSGlobalData.cpp:243) ==13821== by 0x43DFA15: WebCore::JSDOMWindowBase::commonJSGlobalData() (JSDOMWindowBase.cpp:185) ==13821== by 0x441C888: WebCore::ScriptController::getAllWorlds(WTF::Vector<WebCore::DOMWrapperWorld*, 0u>&) (ScriptController.cpp:181) ==13821== by 0x4824691: WebCore::FrameLoader::dispatchDidClearWindowObjectsInAllWorlds() (FrameLoader.cpp:3350) ==13821== by 0x481933E: WebCore::FrameLoader::receivedFirstData() (FrameLoader.cpp:618) ==13821== by 0x481B21F: WebCore::FrameLoader::willSetEncoding() (FrameLoader.cpp:1090) ==13821== by 0x4814BC4: WebCore::DocumentWriter::setEncoding(WTF::String const&, bool) (DocumentWriter.cpp:236) ==13821== by 0x480B35A: WebCore::DocumentLoader::commitData(char const*, int) (DocumentLoader.cpp:306)
Created attachment 75213 [details] cacheentryinit.diff Not sure if this is the proper way to fix it, but couldn't really think of anything else.
Comment on attachment 75213 [details] cacheentryinit.diff View in context: https://bugs.webkit.org/attachment.cgi?id=75213&action=review This patch doesn't fix a bug. There is no symptom. While the key has a random value in it, the value is guaranteed to have a null string, and the code always checks that. I suspect this patch just makes a memory debugging tool stop complaining. Is there some other way to accomplish that without generating a lot of code that will run whenever we construct a JSGlobalData object? > JavaScriptCore/runtime/NumericStrings.h:81 > + CacheEntry() > + : key(0) > + { > + } Initializing to zero is not all that helpful; zero itself has a cached string.
(In reply to comment #2) > (From update of attachment 75213 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75213&action=review > > This patch doesn't fix a bug. There is no symptom. While the key has a random value in it, the value is guaranteed to have a null string, and the code always checks that. > > I suspect this patch just makes a memory debugging tool stop complaining. Is there some other way to accomplish that without generating a lot of code that will run whenever we construct a JSGlobalData object? It's true that basically this can never result in any problem as the code is now, since the UString will have a null value at first and the code checks for that. It's just probably a good idea to shut up valgrind and protect ourselves against future refactorings where this could become an issue. > > > JavaScriptCore/runtime/NumericStrings.h:81 > > + CacheEntry() > > + : key(0) > > + { > > + } > > Initializing to zero is not all that helpful; zero itself has a cached string. Right, but because of the UString check in the if things will still work properly (the first time it will introduced, after that it will be returned from the cache). In any case as I said I realize this is not ideal.
(In reply to comment #3) > It's just probably a good idea to shut up valgrind I guess so. > protect ourselves against future refactorings where this could become an issue. I don’t think it really does protect us, because the initial value of 0 is not necessarily good after future refactoring either. I’d like to hear what the original author thinks.
If the goal was just to quiet valgrind, we could just reverse the if statements and check the string for null before looking at the hash table key. That would make cache misses slower, though, so not sure it’s worth it.
I think initializing key to 0 is misleading because it implies that 0 is a special sentry/invalid value, when it isn't. More work when initializing JSGlobalData is also not great, although it's not a huge concern, since creating JSGlobalData is relatively rare. I'm not too excited about making cache misses slower just to make a tool happy. That said, if SunSpider and v8 don't complain about the change, it's probably OK. Is there any way to fix this problem with valgrind-only annotations or code?
(In reply to comment #6) > Is there any way to fix this problem with valgrind-only annotations or code? Tomorrow I'll see if swapping the order of checks in the if is a noticeable perf hit, since as Daring suggested that should make valgrind happy too.
The results of reversing the if check order: TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: ?? 259.1ms +/- 0.5% 259.5ms +/- 0.5% not conclusive: might be *1.002x as slow* ============================================================================= 3d: ?? 35.3ms +/- 2.1% 35.4ms +/- 2.7% not conclusive: might be *1.003x as slow* cube: - 12.6ms +/- 5.6% 12.2ms +/- 7.1% morph: ?? 10.2ms +/- 1.2% 10.3ms +/- 1.9% not conclusive: might be *1.016x as slow* raytrace: *1.022x as slow* 12.5ms +/- 1.4% 12.8ms +/- 1.6% significant access: ?? 36.6ms +/- 0.9% 36.8ms +/- 1.0% not conclusive: might be *1.006x as slow* binary-trees: ?? 3.3ms +/- 3.9% 3.3ms +/- 4.0% not conclusive: might be *1.012x as slow* fannkuch: ?? 18.1ms +/- 0.8% 18.3ms +/- 0.7% not conclusive: might be *1.008x as slow* nbody: ?? 10.2ms +/- 1.1% 10.2ms +/- 1.2% not conclusive: might be *1.004x as slow* nsieve: - 5.0ms +/- 0.0% 5.0ms +/- 0.0% bitops: - 23.1ms +/- 0.4% 23.2ms +/- 0.5% 3bit-bits-in-byte: - 3.0ms +/- 0.0% 3.0ms +/- 0.0% bits-in-byte: ?? 6.0ms +/- 0.0% 6.0ms +/- 0.9% not conclusive: might be *1.007x as slow* bitwise-and: - 5.1ms +/- 1.9% 5.1ms +/- 1.8% nsieve-bits: - 9.0ms +/- 0.0% 9.0ms +/- 0.0% controlflow: - 2.0ms +/- 0.0% 2.0ms +/- 0.0% recursive: - 2.0ms +/- 0.0% 2.0ms +/- 0.0% crypto: - 15.0ms +/- 0.4% 15.0ms +/- 0.3% aes: - 9.0ms +/- 0.6% 9.0ms +/- 0.4% md5: - 3.0ms +/- 0.0% 3.0ms +/- 0.0% sha1: - 3.0ms +/- 0.0% 3.0ms +/- 0.0% date: ?? 29.1ms +/- 0.5% 29.4ms +/- 1.2% not conclusive: might be *1.010x as slow* format-tofte: ?? 15.0ms +/- 0.4% 15.2ms +/- 1.9% not conclusive: might be *1.009x as slow* format-xparb: ?? 14.1ms +/- 1.0% 14.2ms +/- 1.1% not conclusive: might be *1.010x as slow* math: ?? 26.1ms +/- 0.5% 26.2ms +/- 0.8% not conclusive: might be *1.004x as slow* cordic: ?? 8.0ms +/- 0.5% 8.0ms +/- 0.7% not conclusive: might be *1.002x as slow* partial-sums: ?? 14.0ms +/- 0.3% 14.1ms +/- 0.6% not conclusive: might be *1.003x as slow* spectral-norm: ?? 4.0ms +/- 1.0% 4.1ms +/- 1.7% not conclusive: might be *1.010x as slow* regexp: ?? 11.3ms +/- 1.1% 11.4ms +/- 2.3% not conclusive: might be *1.009x as slow* dna: ?? 11.3ms +/- 1.1% 11.4ms +/- 2.3% not conclusive: might be *1.009x as slow* string: - 80.6ms +/- 0.6% 80.3ms +/- 0.4% base64: ?? 9.0ms +/- 0.0% 9.0ms +/- 0.4% not conclusive: might be *1.002x as slow* fasta: - 11.0ms +/- 0.4% 11.0ms +/- 0.4% tagcloud: - 20.3ms +/- 1.1% 20.2ms +/- 0.7% unpack-code: 1.010x as fast 29.9ms +/- 0.7% 29.6ms +/- 0.6% significant validate-input: - 10.4ms +/- 1.5% 10.4ms +/- 1.5%
Comment on attachment 75213 [details] cacheentryinit.diff Removing from the review queue.