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

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has granted Michael Catanzaro
<mcatanzaro at gnome.org>'s request for review:
Bug 226193: Fix more GCC warnings
https://bugs.webkit.org/show_bug.cgi?id=226193

Attachment 430415: Patch

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




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

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

>>>> Source/JavaScriptCore/jit/JITCall.cpp:258
>>>> +IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_END
>>> 
>>> 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
Source/JavaScriptCore/CMakeF...vedSources/unified-sources/UnifiedSource-3a3c4ec
0-3.cpp.o
>> In file included from
JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp:7:
>> ../../Source/JavaScriptCore/jit/JITCall.cpp: In member function ‘void
JSC::JIT::compileOpCall(const JSC::Instruction*, unsigned int) [with Op =
JSC::OpCallEval]’:
>> ../../Source/JavaScriptCore/jit/JITCall.cpp:256:10: warning: ‘this’ pointer
is null [-Wnonnull]
>>   256 |     auto slowPaths = info->emitFastPath(*this, regT0, regT2,
CallLinkInfo::UseDataIC::Yes);
>>	 |	    ^~~~~~~~~
>> 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
../../Source/JavaScriptCore/jit/JITAddGenerator.cpp:27,
>>		    from
JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp:1:
>> ../../Source/JavaScriptCore/bytecode/CallLinkInfo.h:178:30: note: in a call
to non-static member function
‘JSC::AbstractMacroAssembler<JSC::X86Assembler>::JumpList
JSC::CallLinkInfo::emitFastPath(JSC::CCallHelpers&, JSC::GPRReg, JSC::GPRReg,
JSC::CallLinkInfo::UseDataIC)’
>>   178 |     MacroAssembler::JumpList emitFastPath(CCallHelpers&, GPRReg
calleeGPR, GPRReg callLinkInfoGPR, UseDataIC) WARN_UNUSED_RETURN;
>>	 |				^~~~~~~~~~~~
> 
> OK, RELEASE_ASSERT() works after all. So we can do:
> 
> RELEASE_ASSERT(info);
> auto slowPaths = info->emitFastPath(*this, regT0, regT2,
CallLinkInfo::UseDataIC::Yes);
> 
> Yusuke, do you prefer that to using
IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_BEGIN?
> 
> 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
compiler
assume that the value will be non-NULL afterwards — the first time I saw this
kind
of behavior was in the Rust compiler a few years ago, which was mind-blowing
because
then it also went ahead and did additional optimizations possible under the
asserted
assumption. C/C++ were not that smart then :]

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

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

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
help
with that.