[webkit-reviews] review granted: [Bug 226193] Fix more GCC warnings : [Attachment 430415] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jun 4 00:38:08 PDT 2021

Adrian Perez <aperez at igalia.com> has granted Michael Catanzaro
<mcatanzaro at gnome.org>'s request for review:
Bug 226193: Fix more GCC warnings

Attachment 430415: Patch


--- Comment #15 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 430415
  --> https://bugs.webkit.org/attachment.cgi?id=430415

View in context: https://bugs.webkit.org/attachment.cgi?id=430415&action=review

>>>> Source/JavaScriptCore/jit/JITCall.cpp:258
>>> This looks not great to me. Can you bypass this warning without messing up
the code?
>> Adrian suggested adding an assert, so I tried: RELEASE_ASSERT(this) right
before the call. This made it worse by introducing a long chain of
-Wnonnull-compare warnings and didn't fix the original -Wnonnull. FWIW the full
warning with our current unmodified code is:
>> [3/3414] Building CXX object
>> In file included from
>> ../../Source/JavaScriptCore/jit/JITCall.cpp: In member function ‘void
JSC::JIT::compileOpCall(const JSC::Instruction*, unsigned int) [with Op =
>> ../../Source/JavaScriptCore/jit/JITCall.cpp:256:10: warning: ‘this’ pointer
is null [-Wnonnull]
>>   256 |     auto slowPaths = info->emitFastPath(*this, regT0, regT2,
>>	 |	    ^~~~~~~~~
>> In file included from ../../Source/JavaScriptCore/bytecode/CodeBlock.h:34,
>>		    from ../../Source/JavaScriptCore/jit/AssemblyHelpers.h:30,
>>		    from ../../Source/JavaScriptCore/jit/CCallHelpers.h:30,
>>		    from ../../Source/JavaScriptCore/jit/JITAddGenerator.h:30,
>>		    from
>>		    from
>> ../../Source/JavaScriptCore/bytecode/CallLinkInfo.h:178:30: note: in a call
to non-static member function
JSC::CallLinkInfo::emitFastPath(JSC::CCallHelpers&, JSC::GPRReg, JSC::GPRReg,
>>   178 |     MacroAssembler::JumpList emitFastPath(CCallHelpers&, GPRReg
calleeGPR, GPRReg callLinkInfoGPR, UseDataIC) WARN_UNUSED_RETURN;
>>	 |				^~~~~~~~~~~~
> OK, RELEASE_ASSERT() works after all. So we can do:
> auto slowPaths = info->emitFastPath(*this, regT0, regT2,
> Yusuke, do you prefer that to using
> Now that I realize GCC is complaining about "info" rather than "this" -- it's
highlighting the totally wrong portion of the line there and I claim its error
message is incorrect, it should say "info" rather than "this" -- well it makes
more sense why it's complaining, and I'm less-inclined to label this case
erroneous. It sure does look like info can be null there. Could you look at
this case please?

I had a suspicion that asserting that something is non-null might make the
assume that the value will be non-NULL afterwards — the first time I saw this
of behavior was in the Rust compiler a few years ago, which was mind-blowing
then it also went ahead and did additional optimizations possible under the
assumption. C/C++ were not that smart then :]

This being the only change there are doubts about, I think we can land the
as-is sans this change so we can avoid most of the warning spam during
sooner; then do a follow-up to address this one.

Michael, feel free to land the patch *without* this first, if you feel so

By all means if you or Yusuke manage to figure out whether “info“ can be NULL
(or not)
here, updating the patch is also an option — sadly, I won't have time today to
with that.

More information about the webkit-reviews mailing list