Closed
Bug 1102964
Opened 11 years ago
Closed 11 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•11 years ago
|
||
Attachment #8526827 -
Flags: review?(jimb)
Updated•11 years ago
|
Attachment #8526826 -
Flags: review?(sunfish) → review+
Comment 2•11 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•11 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•11 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•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → benjamin
Comment 6•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2a1e5fe6f289
https://hg.mozilla.org/mozilla-central/rev/24bebb8f489a
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.
Description
•