1125701 - Fix -Wmaybe-uninitialized warnings in js/src/jit

Status: RESOLVED DUPLICATE of bug 1195263

(Core :: JavaScript Engine: JIT, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: Wuninitialized-jit-warnings.patch

js/src/jit/IonBuilder.cpp:7586:56: warning: 'load' may be used uninitialized in this function [-Wmaybe-uninitialized]
js/src/jit/LiveRangeAllocator.cpp:66:15: warning: 'n' may be used uninitialized in this function [-Wmaybe-uninitialized]

The following -Wmaybe-uninitialized warnings are a little worrying because Instruction::extractCond() doesn't return an error or even assert if it returns without initializing its out-parameter:

js/src/jit/arm/Assembler-arm.cpp:1761:132: warning: 'orig_cond' may be used uninitialized in this function [-Wmaybe-uninitialized]
js/src/jit/arm/Assembler-arm.cpp:2509:44: warning: 'c' may be used uninitialized in this function [-Wmaybe-uninitialized]
js/src/jit/arm/Assembler-arm.cpp:551:65: warning: 'c' may be used uninitialized in this function [-Wmaybe-uninitialized]
js/src/jit/arm/Assembler-arm.cpp:2699:5: warning: 'c' may be used uninitialized in this function [-Wmaybe-uninitialized]
js/src/jit/arm/Assembler-arm.cpp:2681:5: warning: 'c' may be used uninitialized in this function [-Wmaybe-uninitialized]
js/src/jit/arm/Assembler-arm.cpp:2681:5: warning: 'c' may be used uninitialized in this function [-Wmaybe-uninitialized]
js/src/jit/arm/Assembler-arm.cpp:2681:5: warning: 'c' may be used uninitialized in this function [-Wmaybe-uninitialized]
js/src/jit/arm/Assembler-arm.cpp:2435:59: warning: 'c' may be used uninitialized in this function [-Wmaybe-uninitialized]
js/src/jit/arm/Assembler-arm.cpp:2518:48: warning: 'c' may be used uninitialized in this function [-Wmaybe-uninitialized]
js/src/jit/arm/Assembler-arm.cpp:2699:5: warning: 'c' may be used uninitialized in this function [-Wmaybe-uninitialized]
js/src/jit/arm/Assembler-arm.cpp:2681:5: warning: 'c' may be used uninitialized in this function [-Wmaybe-uninitialized]
js/src/jit/arm/Assembler-arm.cpp:2699:5: warning: 'c' may be used uninitialized in this function [-Wmaybe-uninitialized]
js/src/jit/arm/Assembler-arm.cpp:2681:5: warning: 'c' may be used uninitialized in this function [-Wmaybe-uninitialized]

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

See also bug 1125371, where the same issue appears.

Also, I think it's nice to keep |switch| statements over enum have explicit cases and no default case wherever possible, so that every time one adds a new enum value, there'll be a warning emitted by the compiler, complaining that the switch doesn't handle the newly added enum value.

Comment 2

[Chris Peterson [:cpeterson]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> See also bug 1125371, where the same issue appears.

> Also, I think it's nice to keep |switch| statements over enum have explicit
> cases and no default case wherever possible, so that every time one adds a
> new enum value, there'll be a warning emitted by the compiler, complaining
> that the switch doesn't handle the newly added enum value.

That makes sense. gcc's -Wmaybe-uninitialized warnings are often false positives and, in these cases, gcc is pessimistically assuming that switching on an enum type does not mean that the enum *value* is valid. I can remove these default case "fixes" if we can assume our enum variables are always in range. :)

I still think the -Wmaybe-uninitialized warnings for Instruction::extractCond() might be real bugs.

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: Wmaybe-uninitialized_extractCond-v2.patch

Patch v2 just fixes the -Wmaybe-uninitialized warnings about Instruction::extractCond().

I removed patch v1's new default cases as bbouvier recommended. The default cases are redundant because these switch statements have explicit cases for all these enum types' values.

Comment 4

[Chris Peterson [:cpeterson]]

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd6d23ec747c

Comment 5

[Chris Peterson [:cpeterson]]

I had to back out this patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/97ed0f3cf7d2

for mochitest-4 assertion failures on Android 4.0 armv7 API 11+:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bd6d23ec747c

https://treeherder.mozilla.org/logviewer.html#?job_id=6192803&repo=mozilla-inbound

21:21:50 WARNING - PROCESS-CRASH | dom/imptests/editing/selecttest/test_selectAllChildren.html | application crashed [@ js::jit::Instruction::extractCond(js::jit::Assembler::Condition*)]
21:21:50 INFO - Operating system: Android
21:21:50 INFO - 0.0.0 Linux 3.2.0+ #2 SMP PREEMPT Thu Nov 29 08:06:57 EST 2012 armv7l pandaboard/pandaboard/pandaboard:4.0.4/IMM76I/5:eng/test-keys
21:21:50 INFO - CPU: arm
21:21:50 INFO - 2 CPUs
21:21:50 INFO - Crash reason: SIGSEGV
21:21:50 INFO - Crash address: 0x0
21:21:50 INFO - Thread 12 (crashed)
21:21:50 INFO - 0 libxul.so!js::jit::Instruction::extractCond(js::jit::Assembler::Condition*) [Assembler-arm.h:bd6d23ec747c : 1852 + 0xa]
21:21:50 INFO - r4 = 0x0000073c r5 = 0x6b91d074 r6 = 0x6b91d078 r7 = 0x6b91d07c
21:21:50 INFO - r8 = 0x0000ffff r9 = 0x6ff09ba0 r10 = 0x6b91d074 fp = 0x6ff09ba0
21:21:50 INFO - sp = 0x5c4fe108 lr = 0x63ad54b9 pc = 0x63ad63fe
21:21:50 INFO - Found by: given as instruction pointer in context
21:21:50 INFO - 1 libxul.so!js::jit::Instruction::next() [Assembler-arm.cpp:bd6d23ec747c : 2825 + 0x11]
21:21:50 INFO - r4 = 0x0000073c r5 = 0x6b91d074 r6 = 0x6b91d078 r7 = 0x6b91d07c
21:21:50 INFO - r8 = 0x0000ffff r9 = 0x6ff09ba0 r10 = 0x6b91d074 fp = 0x6ff09ba0
21:21:50 INFO - sp = 0x5c4fe110 pc = 0x63ade20f
21:21:50 INFO - Found by: call frame info
21:21:50 INFO - 2 libxul.so!unsigned int const* js::jit::Assembler::GetCF32Target<js::jit::InstructionIterator>(js::jit::InstructionIterator*) [Assembler-arm.h:bd6d23ec747c : 2088 + 0x15]
21:21:50 INFO - r4 = 0x5c4fe164 r5 = 0x6b91d074 r6 = 0x6b91d078 r7 = 0x6b91d07c
21:21:50 INFO - r8 = 0x0000ffff r9 = 0x6ff09ba0 r10 = 0x6b91d074 fp = 0x6ff09ba0
21:21:50 INFO - sp = 0x5c4fe128 pc = 0x63ae8f11
21:21:50 INFO - Found by: call frame info
21:21:50 INFO - 3 libxul.so!js::jit::Assembler::TraceJumpRelocations(JSTracer*, js::jit::JitCode*, js::jit::CompactBufferReader&) [Assembler-arm.cpp:bd6d23ec747c : 783 + 0x3]
21:21:50 INFO - r4 = 0x6b91d354 r5 = 0x6b91d354 r6 = 0x67b24eb8 r7 = 0x644d71d8
21:21:50 INFO - r8 = 0x0000ffff r9 = 0x6ff09ba0 r10 = 0x6b91d074 fp = 0x6ff09ba0
21:21:50 INFO - sp = 0x5c4fe158 pc = 0x63ae9193
21:21:50 INFO - Found by: call frame info
21:21:50 INFO - 4 libxul.so!js::jit::JitCode::trace(JSTracer*) [Ion.cpp:bd6d23ec747c : 648 + 0x3]
21:21:50 INFO - r4 = 0x6ff09ba0 r5 = 0x67b24eb8 r6 = 0x6b900008 r7 = 0x000002d4
21:21:50 INFO - r8 = 0x727acc70 r9 = 0x725f5000 r10 = 0x00000000 fp = 0x6ff09ba0
21:21:50 INFO - sp = 0x5c4fe198 pc = 0x63a2f0e7
21:21:50 INFO - Found by: call frame info
21:21:50 INFO - 5 libxul.so!InvalidateActivation [Ion.cpp:bd6d23ec747c : 2585 + 0xb]
21:21:50 INFO - r4 = 0x00000002 r5 = 0x67b25ae0 r6 = 0x00000000 r7 = 0x644b799c
21:21:50 INFO - r8 = 0x727acc70 r9 = 0x725f5000 r10 = 0x00000000 fp = 0x6ff09ba0
21:21:50 INFO - sp = 0x5c4fe1b8 pc = 0x63a39657
21:21:50 INFO - Found by: call frame info
21:21:50 INFO - 6 libxul.so!js::jit::Invalidate(js::types::TypeZone&, js::FreeOp*, js::Vector<js::types::RecompileInfo, 0u, js::SystemAllocPolicy> const&, bool, bool) [Ion.cpp:bd6d23ec747c : 2685 + 0x9]
21:21:50 INFO - r4 = 0x00000001 r5 = 0x5c4fe2c0 r6 = 0x6e181ec8 r7 = 0x00000001
21:21:50 INFO - r8 = 0x00000001 r9 = 0x644baecc r10 = 0x71a5a9b0 fp = 0x725f5400
21:21:50 INFO - sp = 0x5c4fe250 pc = 0x63a50a61
21:21:50 INFO - Found by: call frame info
21:21:50 INFO - 7 libxul.so!js::jit::Invalidate(JSContext*, js::Vector<js::types::RecompileInfo, 0u, js::SystemAllocPolicy> const&, bool, bool) [Ion.cpp:bd6d23ec747c : 2723 + 0x15]
21:21:50 INFO - r4 = 0x00000001 r5 = 0x5c4fe2c0 r6 = 0x00000001 r7 = 0x6a6ae580
21:21:50 INFO - r8 = 0x00000001 r9 = 0x00000001 r10 = 0x5c4fe3b8 fp = 0x725f5400
21:21:50 INFO - sp = 0x5c4fe298 pc = 0x63a50d0b
21:21:50 INFO - Found by: call frame info
21:21:50 INFO - 8 libxul.so!js::jit::Invalidate(JSContext*, JSScript*, bool, bool) [Ion.cpp:bd6d23ec747c : 2768 + 0xb]
21:21:50 INFO - r4 = 0x00000001 r5 = 0x727acc70 r6 = 0x00000000 r7 = 0x6a6ae580
21:21:50 INFO - r8 = 0x00000001 r9 = 0x00000001 r10 = 0x5c4fe3b8 fp = 0x725f5400
21:21:50 INFO - sp = 0x5c4fe2b8 pc = 0x63a6c007
21:21:50 INFO - Found by: call frame info
21:21:50 INFO - 9 libxul.so!js::jit::GetPropertyIC::update(JSContext*, unsigned int, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) [IonCaches.cpp:bd6d23ec747c : 1833 + 0x9]
21:21:50 INFO - r4 = 0x6a6ae580 r5 = 0x00000000 r6 = 0x00000001 r7 = 0x5c4feb60
21:21:50 INFO - r8 = 0x5c4fe358 r9 = 0x5c4fe3d4 r10 = 0x5c4fe3b8 fp = 0x725f5400
21:21:50 INFO - sp = 0x5c4fe308 pc = 0x63a8e317
21:21:50 INFO - Found by: call frame info
21:21:50 INFO - 10 0x6544be42
21:21:50 INFO - r4 = 0x5c4fe3b8 r5 = 0x5c4fe3d0 r6 = 0xffffff81 r7 = 0x68d5f4b8
21:21:50 INFO - r8 = 0x00000001 r9 = 0x6b09e0e0 r10 = 0x681a0f40 fp = 0x5c4fe508
21:21:50 INFO - sp = 0x5c4fe3b0 pc = 0x6544be44
21:21:50 INFO - Found by: call frame info
21:21:50 INFO - 11 libxul.so!js::jit::InlineFrameIterator::InlineFrameIterator(JSContext*, js::jit::JitFrameIterator const*) [JitFrames.cpp:bd6d23ec747c : 2256 + 0x3]
21:21:50 INFO - sp = 0x5c4fe3b4 pc = 0x63a376d3
21:21:50 INFO - Found by: stack scanning

Comment 6

[Chris Peterson [:cpeterson]]

Marty: from the stack trace in comment 5, Instruction::next() calls InstIsGuard(), which calls extractCond() on an Instruction that fails the MOZ_ASSERT(data >> 28 != 0xf, "Instruction must have a condition code") assertion. Before I added that assertion, extractCond() would leave its Condition out-parameter uninitialized:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd6d23ec747c#l1.15

What Condition should extractCond() return for the (data >> == 0xf) case? Should InstIsGuard() check whether the Instruction has a condition before calling extractCond()?

Comment 7

[Chris Peterson [:cpeterson]]

Marty, needinfo ping?