226643 2021-06-04 07:29:32 -0700 -Wnonnull warning in JITCall.cpp 2021-06-12 16:10:00 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build PC Linux RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=226193 InRadar P2 Normal --- 155047 1 mcatanzaro mcatanzaro aperez darin ews-watchlist keith_miller mark.lam mcatanzaro msaboff saam tzagallo webkit-bug-importer ysuzuki oldest_to_newest 1766756 0 mcatanzaro 2021-06-04 07:29:32 -0700 [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). 1768927 1 webkit-bug-importer 2021-06-11 07:30:29 -0700 <rdar://problem/79197261> 1769185 2 ysuzuki 2021-06-11 22:38:50 -0700 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? 1769191 3 mcatanzaro 2021-06-12 05:35:43 -0700 (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. 1769192 4 431254 mcatanzaro 2021-06-12 05:37:39 -0700 Created attachment 431254 Patch 1769193 5 mcatanzaro 2021-06-12 05:39:56 -0700 (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 1769225 6 mcatanzaro 2021-06-12 15:34:57 -0700 (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 1769227 7 ews-feeder 2021-06-12 16:09:58 -0700 Committed r278816 (238769@main): <https://commits.webkit.org/238769@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 431254. 431254 2021-06-12 05:37:39 -0700 2021-06-12 16:09:59 -0700 Patch bug-226643-20210612073738.patch text/plain 1458 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMjc4NzE5CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBj NjIwYzBmMjE5N2JlY2NhYzNmMmU2Mzg0MjlmODQwMjhkZGE4NjU5Li40YTY0YTI5ODk4YmRmNDNm OTA0NDMxNjU5ZjhlNjllYTUxOTdmYzJmIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNyBAQAorMjAyMS0wNi0xMiAgTWljaGFlbCBDYXRhbnphcm8gIDxtY2F0YW56YXJvQGdu b21lLm9yZz4KKworICAgICAgICAtV25vbm51bGwgd2FybmluZyBpbiBKSVRDYWxsLmNwcAorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjI2NjQzCisgICAg ICAgIDxyZGFyOi8vcHJvYmxlbS83OTE5NzI2MT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBUaGlzIGlzIGEgZmFsc2UtcG9zaXRpdmUsIHNvIHN1cHBy ZXNzIGl0IHVzaW5nCisgICAgICAgIElHTk9SRV9FUlJPTkVPVVNfR0NDX05VTExfQ0hFQ0tfV0FS TklOR1NfQkVHSU4uCisKKyAgICAgICAgKiBqaXQvSklUQ2FsbC5jcHA6CisgICAgICAgIChKU0M6 OkpJVDo6Y29tcGlsZU9wQ2FsbCk6CisKIDIwMjEtMDYtMTAgIE1hcmsgTGFtICA8bWFyay5sYW1A YXBwbGUuY29tPgogCiAgICAgICAgIEFub3RoZXIgc3BlY3VsYXRpdmUgYnVpbGQgZml4IGZvciBX aW4zMi4KZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9qaXQvSklUQ2FsbC5jcHAg Yi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvaml0L0pJVENhbGwuY3BwCmluZGV4IDZhYjU0ODgyZDdk OTEyYzU2MTlmNmJhZjQwYTFjNjVhMDMxYmU2MDAuLmUzNjJiNzc0MzRhNDAxNzNhYzkxNDVhOWJh NTdmNGJjMTk3ZmQ4MDIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9qaXQvSklU Q2FsbC5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9KSVRDYWxsLmNwcApAQCAt MjUzLDcgKzI1Myw5IEBAIHZvaWQgSklUOjpjb21waWxlT3BDYWxsKGNvbnN0IEluc3RydWN0aW9u KiBpbnN0cnVjdGlvbiwgdW5zaWduZWQgY2FsbExpbmtJbmZvSW5kCiAgICAgICAgIHJldHVybjsK ICAgICB9CiAKK0lHTk9SRV9FUlJPTkVPVVNfR0NDX05VTExfQ0hFQ0tfV0FSTklOR1NfQkVHSU4K ICAgICBhdXRvIHNsb3dQYXRocyA9IGluZm8tPmVtaXRGYXN0UGF0aCgqdGhpcywgcmVnVDAsIHJl Z1QyLCBDYWxsTGlua0luZm86OlVzZURhdGFJQzo6WWVzKTsKK0lHTk9SRV9FUlJPTkVPVVNfR0ND X05VTExfQ0hFQ0tfV0FSTklOR1NfRU5ECiAgICAgYXV0byBkb25lTG9jYXRpb24gPSBsYWJlbCgp OwogICAgIGFkZFNsb3dDYXNlKHNsb3dQYXRocyk7CiAK