1438842 - Remove unused parameters in js/src

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[André Bargull [:anba]]

Remove unused parameters reported by "misc-unused-parameters" clang-tidy check.

Comment 1

[Nicolas B. Pierron [:nbp]]

I am not sure we always want that!

In some cases we do have unused parameters on purpose, such as the AutoSaveLiveRegisters argument of icBuildOOLFakeExitFrame [1]. This API ignores its argument on purpose, in order to enforce developers to create an AutoSaveLiveRegisters instance with icSaveLive function.

[1] https://searchfox.org/mozilla-central/source/js/src/jit/MacroAssembler.h#2605,2609

Comment 2

[André Bargull [:anba]]

Attachment: bug1438842-part1-gc.patch

This patch removes unused parameters from js/src/gc and js/src/jsgc.cpp as reported by clang-tidy.

If some of these unused parameters are intentionally unused, e.g. for symmetry with other functions, I can restore them.

Comment 3

[André Bargull [:anba]]

Attachment: bug1438842-part2-other.patch

This patch removes unused parameters from js/src/builtin, js/src/frontend, and js/src/*.cpp as reported by clang-tidy.

If some of these unused parameters are intentionally unused, e.g. for symmetry with other functions, I can restore them.

Comment 4

[André Bargull [:anba]]

Attachment: bug1438842-part3-jit.patch

This patch removes unused parameters from js/src/jit as reported by clang-tidy.

As written above, if some of the unused parameters are intentionally unused, I can restore them.

Comment 5

[André Bargull [:anba]]

Attachment: bug1438842-part4-vm.patch

This patch removes unused parameters from js/src/vm as reported by clang-tidy.

As written above, if some of the unused parameters are intentionally unused, I can restore them.

Comment 6

[André Bargull [:anba]]

Attachment: bug1438842-part5-wasm.patch

This patch removes unused parameters from js/src/jit as reported by clang-tidy.

As written above, if some of the unused parameters are intentionally unused, I can restore them.

The "errorNumber" parameter of |CompileStreamTask::rejectAndDestroyAfterHelperThreadStarted(...)| was unused, but I assume it should be used for the streamError_ field.

Comment 7

[André Bargull [:anba]]

(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> I am not sure we always want that!

Yes, I didn't expect anything else. :-)

Nevertheless I thought it'd make sense to see if any of these unused parameters can actually be removed, for example if we missed to remove them when refactoring code, or to check if these unused parameters should actually be used and not using them is a bug in the current code. 

> In some cases we do have unused parameters on purpose, such as the
> AutoSaveLiveRegisters argument of icBuildOOLFakeExitFrame [1]. This API
> ignores its argument on purpose, in order to enforce developers to create an
> AutoSaveLiveRegisters instance with icSaveLive function.

I didn't touch any functions with "AutoSomething" parameters, for example GC code has many AutoLockGC parameters.

Comment 8

[Jon Coppeard (:jonco)]

Comment on attachment 8951607 [details] [diff] [review]
bug1438842-part1-gc.patch

Review of attachment 8951607 [details] [diff] [review]:
-----------------------------------------------------------------

This is great, thanks for doing this!

::: js/src/gc/GCInternals.h
@@ -59,5 @@
>  {
>      mozilla::Maybe<AutoTraceSession> session_;
>  
>    public:
> -    AutoPrepareForTracing(JSContext* cx, ZoneSelector selector);

I removed this one yesterday :)

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8951613 [details] [diff] [review]
bug1438842-part5-wasm.patch

Review of attachment 8951613 [details] [diff] [review]:
-----------------------------------------------------------------

That's really sweet, thanks.

::: js/src/wasm/WasmJS.cpp
@@ +2185,1 @@
>          Handle<PromiseObject*> promise, bool instantiate, HandleObject importObj)

nit: can probably reorganize line to fit more arguments on the first line

@@ +2497,5 @@
>      // See setClosedAndDestroyAfterHelperThreadStarted() comment.
>      bool rejectAndDestroyAfterHelperThreadStarted(unsigned errorNumber) {
>          MOZ_ASSERT(streamState_.lock() == Code || streamState_.lock() == Tail);
>          MOZ_ASSERT(!streamError_);
> +        streamError_ = Some(errorNumber);

Nice catch.

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8951611 [details] [diff] [review]
bug1438842-part4-vm.patch

Review of attachment 8951611 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent work, thanks!

::: js/src/vm/NativeObject.cpp
@@ +2272,5 @@
>   *     web, as there are too many false positives, but anecdotally useful in
>   *     Gecko code.)
>   */
>  static bool
> +GetNonexistentProperty(JSContext* cx, HandleId id, IsNameLookup nameLookup, MutableHandleValue vp)

Great change.

Comment 11

[Ted Campbell [:tcampbell]]

Comment on attachment 8951610 [details] [diff] [review]
bug1438842-part3-jit.patch

Review of attachment 8951610 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

One 'script' var looks still in use. A couple cases I'd prefer to keep. I've also tagged a few follows for someone to cleanup since I was reading through all this code.

::: js/src/jit/BaselineBailouts.cpp
@@ +1665,5 @@
>          bool passExcInfo = handleException || propagatingExceptionForDebugMode;
>  
>          jsbytecode* callPC = nullptr;
>          RootedFunction nextCallee(cx, nullptr);
> +        if (!InitFromBailout(cx, callerPC, fun, scr,

Follow-up: we should remove the local caller, callerPC variables and pass frameNo instead.

::: js/src/jit/BaselineCompiler.cpp
@@ +233,5 @@
>      baselineScript->copyPCMappingEntries(pcEntries);
>  
>      // Copy IC entries
>      if (icEntries_.length())
> +        baselineScript->copyICEntries(script, &icEntries_[0]);

Follow-up: CodeLocationLabel::repoint should be fixed so IonScript::copyICEntries can remove the masm arg as well.

@@ +4635,5 @@
>      pushArg(R0.scratchReg());
>      return callVM(DebugAfterYieldInfo);
>  }
>  
> +typedef bool (*FinalSuspendFn)(JSContext*, HandleObject, jsbytecode*);

\o/

::: js/src/jit/BaselineDebugModeOSR.cpp
@@ +267,5 @@
>      return true;
>  }
>  
>  static bool
> +CollectInterpreterStackScripts(const Debugger::ExecutionObservableSet& obs,

Slight preference to keeping.

::: js/src/jit/BaselineInspector.cpp
@@ +755,5 @@
>      return true;
>  }
>  
>  static bool
> +AddCacheIRGlobalGetter(ICCacheIR_Monitored* stub,

Leave this.

Follow-up: |if (innerized) return false;|

::: js/src/jit/BaselineJIT.cpp
@@ +949,5 @@
>  }
>  
>  #ifdef JS_TRACE_LOGGING
>  void
> +BaselineScript::initTraceLogger(JSScript* script, const Vector<CodeOffset>& offsets)

This still looks used:
https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/js/src/jit/BaselineJIT.cpp#966

::: js/src/jit/IonBuilder.cpp
@@ +7556,5 @@
> +    return jsop_getname();
> +}
> +
> +AbortReasonOr<Ok>
> +IonBuilder::jsop_getname()

Keep this for consistency with opcode definition.

::: js/src/jit/MCallOptimize.cpp
@@ +3675,5 @@
>          return inlineSimdCheck(callInfo, native, type);
>        case SimdOperation::Fn_splat:
>          return inlineSimdSplat(callInfo, native, type);
>        case SimdOperation::Fn_extractLane:
> +        return inlineSimdExtractLane(callInfo, type);

Keep for consistency

@@ +3741,5 @@
>          return inlineSimdShift(callInfo, native, MSimdShift::rshForSign(GetSimdSign(type)), type);
>  
>          // Boolean unary.
>        case SimdOperation::Fn_allTrue:
> +        return inlineSimdAnyAllTrue(callInfo, /* IsAllTrue= */true, type);

Keep these for consistency.

::: js/src/jit/shared/BaselineCompiler-shared.cpp
@@ +19,5 @@
>    : cx(cx),
>      script(script),
>      pc(script->code()),
> +    ionCompileable_(jit::IsIonEnabled(cx) && CanIonCompileScript(cx, script)),
> +    ionOSRCompileable_(jit::IsIonEnabled(cx) && CanIonCompileScript(cx, script)),

Follow-up: Remove ionOSRCompileable_

Comment 12

[André Bargull [:anba]]

Hey Benjamin, I've hit a small problem with the WASM patches. This assertion [1] fails on try [2], but if undo the change for the ParseBranchTable function, the try build succeeds again [3]. I guess what happens here is that avoiding calling the copy-constructor for |WasmToken| changes the stack size for ParseBranchTable resp. its caller ParseExprBody, which then leads to a different behaviour for the recursion check in ParseExprBody. Or something like that...?
This is only a problem for Linux32-opt builds (no error on Linux32-debug or Linux64 [4]). So should I update the check in [1] to also allow OOM failures for Linux32-opt, or should I rather remove the complete build configuration check?

Thanks,
André

[1] https://searchfox.org/mozilla-central/rev/5536f71c3833018c4f4e2c73f37eae635aab63ff/js/src/jit-test/tests/wasm/compiler-frame-depth.js#25
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=45e8b4630080c8af0fa77a8c2edaa1fcf4af244b
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=39b2952bd9b11b67fd7b4a592f8371a9055ba405
[4] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3de8d443c6af6cdd3aefd6cbd86a911e18c045b

Comment 13

[André Bargull [:anba]]

(In reply to Ted Campbell [:tcampbell] from comment #11)
> ::: js/src/jit/BaselineJIT.cpp
> @@ +949,5 @@
> >  }
> >  
> >  #ifdef JS_TRACE_LOGGING
> >  void
> > +BaselineScript::initTraceLogger(JSScript* script, const Vector<CodeOffset>& offsets)
> 
> This still looks used:
> https://searchfox.org/mozilla-central/rev/
> 74b7ffee403c7ffd05b8b476c411cbf11d134eb9/js/src/jit/BaselineJIT.cpp#966
> 

Hmm, the patch only removes the |JSRuntime*| argument for initTraceLogger. Did you mean to point to a different function?

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Strange. Can you add debug annotations in wasmFullPass jit-tests/tests/lib/wasm.js to know which step in particular is failing? Of course I can't repro locally, so I'd suggest to just remove the builds option assertion if there's nothing weird that shows up in the try build logs.

Comment 15

[André Bargull [:anba]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #14)
> Strange. Can you add debug annotations in wasmFullPass
> jit-tests/tests/lib/wasm.js to know which step in particular is failing?

Per [1], the error is thrown when |wasmTextToBinary| is called here [2].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c078a7681b8f1946c7ab2fc9900fa8e90665bca
[2] https://searchfox.org/mozilla-central/rev/5536f71c3833018c4f4e2c73f37eae635aab63ff/js/src/jit-test/lib/wasm.js#112

> Of course I can't repro locally, so I'd suggest to just remove the builds
> option assertion if there's nothing weird that shows up in the try build
> logs.

Me neither. (I forgot to mention that in comment #12, sorry!)

Comment 16

[Ted Campbell [:tcampbell]]

(In reply to André Bargull [:anba] from comment #13)
> Hmm, the patch only removes the |JSRuntime*| argument for initTraceLogger.
> Did you mean to point to a different function?

You are correct, I got myself confused looking at patch file. All the initTraceLogger stuff looks good to me.

Comment 17

[André Bargull [:anba]]

Attachment: bug1438842-part1-gc.patch

Rebased part 1 to apply cleanly on inbound. Carrying r+.

Comment 18

[André Bargull [:anba]]

Attachment: bug1438842-part2-other.patch

Rebased part 2 to apply cleanly on inbound. Carrying r+.

Comment 19

[André Bargull [:anba]]

Attachment: bug1438842-part3-jit.patch

Rebased part 3 to apply cleanly on inbound and to apply review comments. Carrying r+.

Comment 20

[André Bargull [:anba]]

Attachment: bug1438842-part4-vm.patch

Rebased part 4 to apply cleanly on inbound. Carrying r+.

Comment 21

[André Bargull [:anba]]

Attachment: bug1438842-part5-wasm.patch

Rebased part 5 to apply cleanly on inbound and to apply review comments and to adjust jit-test/tests/wasm/compiler-frame-depth.js per comment #14. Carrying r+.

Comment 22

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e62bbb438bf37a49e8c6403b0f794f91afacc429

Try (to verify builds on other platforms; from yesterday, missing the latest update for part 2): https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd38c96d5d8c2660030a79cde15dfcba8b70b70c

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ab40910b4cc
Part 1: Remove unused parameters in GC code. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c92666cad03
Part 2: Remove unused parameters in built-in, frontend, and general files. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4b06e10ae6
Part 3: Remove unused parameters in js/src/jit. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ec168a898ac
Part 4: Remove unused parameters in js/src/vm. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4136715d683
Part 5: Remove unused parameters in js/src/wasm. r=bbouvier

Comment 24

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/2ab40910b4cc
https://hg.mozilla.org/mozilla-central/rev/9c92666cad03
https://hg.mozilla.org/mozilla-central/rev/8b4b06e10ae6
https://hg.mozilla.org/mozilla-central/rev/9ec168a898ac
https://hg.mozilla.org/mozilla-central/rev/a4136715d683