Closed
Bug 1102964
Opened 10 years ago
Closed 9 years ago
Fix various unused variable warnings in optimized builds
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: Benjamin, Assigned: Benjamin)
Details
Attachments
(2 files)
2.79 KB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
2.05 KB,
patch
|
jimb
:
review+
|
Details | Diff | Splinter 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)
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8526827 -
Flags: review?(jimb)
Updated•10 years ago
|
Attachment #8526826 -
Flags: review?(sunfish) → review+
Comment 2•10 years ago
|
||
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+
Assignee | ||
Comment 3•10 years ago
|
||
(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.
Assignee | ||
Comment 4•10 years ago
|
||
(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 | ||
Comment 5•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a1e5fe6f289 https://hg.mozilla.org/integration/mozilla-inbound/rev/24bebb8f489a
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → benjamin
Comment 6•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2a1e5fe6f289 https://hg.mozilla.org/mozilla-central/rev/24bebb8f489a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•