Closed Bug 1102964 Opened 11 years ago Closed 11 years ago

Fix various unused variable warnings in optimized builds

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: Benjamin, Assigned: Benjamin)

Details

Attachments

(2 files)

Attached patch jit-unused.patchSplinter Review
Compiling SM with optimization currently gives: /home/benjamin/dev/mozilla/inbound/js/src/jit/BaselineDebugModeOSR.cpp:395:43: warning: unused variable 'info' [-Wunused-variable] if (BaselineDebugModeOSRInfo *info = iter.baselineFrame()->getDebugModeOSRInfo()) { ^ Unified_cpp_js_src8.o TestTypedEnum Unified_cpp_js_src9.o Unified_cpp_js_src10.o Unified_cpp_js_src11.o In file included from /home/benjamin/dev/mozilla/inbound/js/src/_OPT.OBJ/js/src/Unified_cpp_js_src10.cpp:11: /home/benjamin/dev/mozilla/inbound/js/src/vm/Debugger.cpp:1283:15: warning: variable 'st' is used uninitialized whenever switch default is taken [-Wsometimes-uninitialized] default: ^~~~~~~ /home/benjamin/dev/mozilla/inbound/js/src/vm/Debugger.cpp:1286:17: note: uninitialized use occurs here if (st != JSTRAP_CONTINUE) ^~ /home/benjamin/dev/mozilla/inbound/js/src/vm/Debugger.cpp:1271:13: note: variable 'st' is declared here JSTrapStatus st; ^ /home/benjamin/dev/mozilla/inbound/js/src/vm/Debugger.cpp:1679:18: warning: unused variable 'status' [-Wunused-variable] JSTrapStatus status = dispatchHook(cx, &rval, hook, promise);
Attachment #8526826 - Flags: review?(sunfish)
Attached patch dbg-unused.patchSplinter Review
Attachment #8526827 - Flags: review?(jimb)
Attachment #8526826 - Flags: review?(sunfish) → review+
Comment on attachment 8526827 [details] [diff] [review] dbg-unused.patch Review of attachment 8526827 [details] [diff] [review]: ----------------------------------------------------------------- Aside from the change to dispatchHook, looks good. Please revise. ::: js/src/vm/Debugger.cpp @@ +1281,5 @@ > case OnPromiseSettled: > st = dbg->firePromiseHook(cx, which, payload, vp); > break; > default: > + MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("Unexpected debugger hook"); I do not want to grant the compiler permission to do anything it likes if control ever reaches that point, which is what MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE does. Since we do have a "safe" return path (with scare quotes because, where did that Hook value come from), MOZ_ASSERT_UNREACHABLE is the right thing here. The fix for the warning is probably to add: st = JSTRAP_CONTINUE;
Attachment #8526827 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #2) > Comment on attachment 8526827 [details] [diff] [review] > dbg-unused.patch > > Review of attachment 8526827 [details] [diff] [review]: > ----------------------------------------------------------------- > > Aside from the change to dispatchHook, looks good. Please revise. > > ::: js/src/vm/Debugger.cpp > @@ +1281,5 @@ > > case OnPromiseSettled: > > st = dbg->firePromiseHook(cx, which, payload, vp); > > break; > > default: > > + MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("Unexpected debugger hook"); > > I do not want to grant the compiler permission to do anything it likes if > control ever reaches that point, which is what > MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE does. Since we do have a "safe" > return path (with scare quotes because, where did that Hook value come > from), MOZ_ASSERT_UNREACHABLE is the right thing here. > > The fix for the warning is probably to add: > > st = JSTRAP_CONTINUE; I'm happy to make that change. Note that undefined behavior was what was happening in optimized builds anyway because the MOZ_ASSERT_UNREACHABLE was optimized away and |st| was simply used uninitialized in the comparison.
(In reply to :Benjamin Peterson from comment #3) > (In reply to Jim Blandy :jimb from comment #2) > > Comment on attachment 8526827 [details] [diff] [review] > > dbg-unused.patch > > > > Review of attachment 8526827 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Aside from the change to dispatchHook, looks good. Please revise. > > > > ::: js/src/vm/Debugger.cpp > > @@ +1281,5 @@ > > > case OnPromiseSettled: > > > st = dbg->firePromiseHook(cx, which, payload, vp); > > > break; > > > default: > > > + MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("Unexpected debugger hook"); > > > > I do not want to grant the compiler permission to do anything it likes if > > control ever reaches that point, which is what > > MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE does. Since we do have a "safe" > > return path (with scare quotes because, where did that Hook value come > > from), MOZ_ASSERT_UNREACHABLE is the right thing here. > > > > The fix for the warning is probably to add: > > > > st = JSTRAP_CONTINUE; > > I'm happy to make that change. Note that undefined behavior was what was > happening in optimized builds anyway because the MOZ_ASSERT_UNREACHABLE was > optimized away and |st| was simply used uninitialized in the comparison. Erm. Bad terminology. MOZ_ASSERT_UNREACHABLE does nothing in non-debug builds. It's not "optimized" by the compiler away.
Assignee: nobody → benjamin
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: