1102964 - Fix various unused variable warnings in optimized builds

Status: RESOLVED FIXED

Description

[:Benjamin Peterson]

Created attachment 8526826 [details] [diff] [review]
jit-unused.patch

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);

Comment 1

[:Benjamin Peterson]

Created attachment 8526827 [details] [diff] [review]
dbg-unused.patch

Comment 2

[Jim Blandy :jimb]

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;

Comment 3

[:Benjamin Peterson]

(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.

Comment 4

[:Benjamin Peterson]

(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.

Comment 5

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a1e5fe6f289
https://hg.mozilla.org/integration/mozilla-inbound/rev/24bebb8f489a

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a1e5fe6f289
https://hg.mozilla.org/mozilla-central/rev/24bebb8f489a