Bug 226643 - -Wnonnull warning in JITCall.cpp
Summary: -Wnonnull warning in JITCall.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on:
Blocks: 155047
  Show dependency treegraph
 
Reported: 2021-06-04 07:29 PDT by Michael Catanzaro
Modified: 2021-06-12 16:10 PDT (History)
11 users (show)

See Also:


Attachments
Patch (1.42 KB, patch)
2021-06-12 05:37 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2021-06-04 07:29:32 PDT
[3/3414] Building CXX object Source/JavaScriptCore/CMakeF...vedSources/unified-sources/UnifiedSource-3a3c4ec0-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;
      |                              ^~~~~~~~~~~~

Despite the very misleading text of the warning, GCC is concerned that *info* may be NULL at JITCall.cpp:256. (It's concerned about the value of 'this' *inside* the call to info->emitFastPath.) Would be great if a JavaScriptCore developer could investigate it, please. From my amateurish read of the code, it sure *looks* like GCC's warning is reasonable as there appear to be codepaths where info is NULL at that point.

The warning may be suppressed either with IGNORE_WARNINGS_BEGIN/END or by adding a RELEASE_ASSERT(info).
Comment 1 Radar WebKit Bug Importer 2021-06-11 07:30:29 PDT
<rdar://problem/79197261>
Comment 2 Yusuke Suzuki 2021-06-11 22:38:50 PDT
GCC's warning looks wrong. it is nullptr only when opcodeID is op_call_eval. And these cases are handled in

if (compileCallEval(bytecode))
    return;

So, L256, it must be a non nullptr.
My intuition is that GCC's implementation of this warning looks broken :(

Did we found a real bug with this warning so far?
Comment 3 Michael Catanzaro 2021-06-12 05:35:43 PDT
(In reply to Yusuke Suzuki from comment #2)
> Did we found a real bug with this warning so far?

Yes, that's what the IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_BEGIN macro is for. 

The GCC developers are going to say it's not a bug because the warning is not intended to avoid all possible false-positives, so let's use IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_BEGIN.
Comment 4 Michael Catanzaro 2021-06-12 05:37:39 PDT
Created attachment 431254 [details]
Patch
Comment 5 Michael Catanzaro 2021-06-12 05:39:56 PDT
(Alternatively: we could use -Wno-nonnull globally when building with GCC, and rely on developers building with Clang to catch real -Wnonnull problems.)

I think it's few enough cases so far that the macro is OK for now, but they are unfortunately in some awkward places (e.g. RefPtr.h):

$ git grep IGNORE_ERRONEOUS
WTF/wtf/Compiler.h:#define IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_BEGIN IGNORE_GCC_WARNINGS_BEGIN("nonnull")
WTF/wtf/Compiler.h:#define IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_END IGNORE_GCC_WARNINGS_END
WTF/wtf/RefPtr.h:IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_BEGIN
WTF/wtf/RefPtr.h:IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_END
WebCore/css/CSSValue.h:IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_BEGIN
WebCore/css/CSSValue.h:IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_END
WebCore/css/StyleRule.h:IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_BEGIN
WebCore/css/StyleRule.h:IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_END
WebCore/dom/Node.h:IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_BEGIN
WebCore/dom/Node.h:IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_END
WebKit/WebProcess/Plugins/PluginView.cpp:IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_BEGIN
WebKit/WebProcess/Plugins/PluginView.cpp:IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_END
Comment 6 Michael Catanzaro 2021-06-12 15:34:57 PDT
(In reply to Michael Catanzaro from comment #3) 
> The GCC developers are going to say it's not a bug because the warning is
> not intended to avoid all possible false-positives, so let's use
> IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_BEGIN.

Reference for this: https://bugzilla.redhat.com/show_bug.cgi?id=1948775#c4
Comment 7 EWS 2021-06-12 16:09:58 PDT
Committed r278816 (238769@main): <https://commits.webkit.org/238769@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431254 [details].