Bug 191626

JSDOMGlobalObject is WEBCORE_EXPORT-ed. https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/bindings/js/JSDOMGlobalObject.h?rev=238039#L45 WebCore::JSDOMGlobalObject::createStructure is using JSC::Structure::create without including StructureInlines.h. https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/bindings/js/JSDOMGlobalObject.h?rev=238039#L87 r238039 is the culprit. https://trac.webkit.org/changeset/238039/

I found a long discussion. [webkit-dev] …Inlines.h vs …InlineMethods.h https://lists.webkit.org/pipermail/webkit-dev/2012-November/022703.html

Created attachment 354901 [details] Patch

I tested forward inline method declarations with GCC, Clang, MSVC and confirmed they work as expected. https://godbolt.org/z/slYYOj This shows it is no problem even if the inline function is defined after it is used. https://godbolt.org/z/TrUmSL This shows GCC and Clang report warning if the inline function is not defined. > <source>:6:16: warning: inline function 'bar::y' is not defined [-Wundefined-inline] > inline int y(); > ^ > <source>:12:14: note: used here > return b.y(); > ^

Could anyone reivew this patch to fix clang-cl builds? I don't want to lang this patch without reviewed by JSC forks. It is safe to move Structure::create to Structure.h because Structure.h already has code to access VM members.

Comment on attachment 354901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354901&action=review r=me > Source/JavaScriptCore/runtime/Structure.h:135 > + inline static Structure* create(VM&, JSGlobalObject*, JSValue prototype, const TypeInfo&, const ClassInfo*, IndexingType = NonArray, unsigned inlineCapacity = 0); I think this is `inline` is not necessary.

Comment on attachment 354901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354901&action=review >> Source/JavaScriptCore/runtime/Structure.h:135 >> + inline static Structure* create(VM&, JSGlobalObject*, JSValue prototype, const TypeInfo&, const ClassInfo*, IndexingType = NonArray, unsigned inlineCapacity = 0); > > I think this is `inline` is not necessary. Oh😭 I believe this is really nice, and I want to mark all other inline method declarations to match with the definitions. This was fixed by Mark Lam in Bug 155657. And, reappeared by r238039. We need a effective prevention of recurrence.

Created attachment 355250 [details] Patch for landing Addressed the review feedback.

Comment on attachment 355250 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=355250&action=review > Source/JavaScriptCore/runtime/Structure.h:831 > +inline Structure* Structure::create(VM& vm, JSGlobalObject* globalObject, JSValue prototype, const TypeInfo& typeInfo, const ClassInfo* classInfo, IndexingType indexingType, unsigned inlineCapacity) Would be nice to add a comment above this to indicate why you put it here instead of StructureInlines.h. I can see someone coming along later and just moving it because there's no apparent reason why it should not be. A comment would warn against that.

Comment on attachment 355250 [details] Patch for landing Clearing flags on attachment: 355250 Committed r238366: <https://trac.webkit.org/changeset/238366>

All reviewed patches have been landed. Closing bug.

<rdar://problem/46161064>

(In reply to Mark Lam from comment #9) > Would be nice to add a comment above this to indicate why you put it here > instead of StructureInlines.h. I can see someone coming along later and > just moving it because there's no apparent reason why it should not be. A > comment would warn against that. My proposal (Comment 7) is much better than just a comment. If someone will use the inline fuctions without including *Inlines.h, s/he will be busted by a bunch of compilation warnings (Comment 4). I will file a new bug for the proposal patch you will like.

(In reply to Fujii Hironori from comment #13) > (In reply to Mark Lam from comment #9) > > Would be nice to add a comment above this to indicate why you put it here > > instead of StructureInlines.h. I can see someone coming along later and > > just moving it because there's no apparent reason why it should not be. A > > comment would warn against that. > > My proposal (Comment 7) is much better than just a comment. > If someone will use the inline fuctions without including *Inlines.h, s/he > will be busted by a bunch of compilation warnings (Comment 4). > I will file a new bug for the proposal patch you will like. Declaring the function as inline doesn't tell us that we shouldn't put it in StructureInlines.h. I think the reason to keep it in Structure.h warrants a comment.

(In reply to Mark Lam from comment #14) > Declaring the function as inline doesn't tell us that we shouldn't put it in > StructureInlines.h. I think the reason to keep it in Structure.h warrants a > comment. There is no problem for me adding a comment. What comment do you want to add in this case? The reason why we can't put it in StructureInlines.h is that r238039 reverted your change (Bug 155657) for macOS internal builds. https://trac.webkit.org/changeset/238039/ Your change was absolutely right because WebCore::JSDOMGlobalObject::createStructure is using JSC::Structure::create. If JSDOMGlobalObject.h properly includes StructureInlines.h, we don't have any reason not to put the inline function in StructureInlines.h.

(In reply to Fujii Hironori from comment #15) > (In reply to Mark Lam from comment #14) > > Declaring the function as inline doesn't tell us that we shouldn't put it in > > StructureInlines.h. I think the reason to keep it in Structure.h warrants a > > comment. > > There is no problem for me adding a comment. > What comment do you want to add in this case? > > The reason why we can't put it in StructureInlines.h is that > r238039 reverted your change (Bug 155657) for macOS internal builds. > > https://trac.webkit.org/changeset/238039/ > > Your change was absolutely right because > WebCore::JSDOMGlobalObject::createStructure is using > JSC::Structure::create. > > If JSDOMGlobalObject.h properly includes StructureInlines.h, we > don't have any reason not to put the inline function in > StructureInlines.h. Just to clarify, I think a comment is necessary because: 1. Mac builds were never broken i.e. we're not seeing a linker error. 2. The only way we don't see a linker error is because our compilation sees the correct Structure::create() definition from StructureInlines.h. 3. The reason we see the correct definition is probably because Mac builds use unified sources, and we lucked out on inadvertantly #include'ing StructureInlines.h. Hence, while adding the "inline" declaration could trigger a compilation error if StructureInlines.h is not #include'd, luck would have it that Mac (and probably linux) builds won't see this error. Only Windows build would. Basically, if we're not seeing a linker error before, we won't see a compilation error as you would on the Windows build. I also think that Chris' fix in r238039 is correct. JSDOMGlobalObject.h should not #include a Inlines.h file (I was wrong). No non-Inlines .h file should #include a Inlines.h file. The standard approach to fixing the Windows build breakage should be to #include "StructureInlines.h" in all the .cpp files that need it, or maybe to #include it in JSCInlines.h. However, I'm willing to go with your solution because of the reason you gave in the ChangeLog. But, to prevent someone from moving the definition back into StructureInlines.h (because Mac/linux builds don't see the build breakage), I think a comment would help. As for the comment, I was thinking of something like this: "We deliberately putting Structure::create here in Structure.h instead of StructureInlines.h, because it is used everywhere. This is so we don't have to hunt down all the places where we would need to #include StructureInlines.h otherwise." Does this clarify my point?

(In reply to Mark Lam from comment #16) > As for the comment, I was thinking of something like this: > > "We deliberately putting Structure::create here in Structure.h instead of > StructureInlines.h, because it is used everywhere. This is so we don't have > to hunt down all the places where we would need to #include > StructureInlines.h otherwise." Typo: I meant to say: "We deliberately put Structure::create here in Structure.h instead of StructureInlines.h, because it is used everywhere. This is so we don't have to hunt down all the places where we would need to #include StructureInlines.h otherwise."

If I mark JSC::Structure::create declaration 'inline', mac port can't compile. (it is using -Werror to convert warnings to errors. I tested with r238365.) > CompileC /Users/fujii/work/webkit/g1/WebKitBuild/JavaScriptCore.build/Release/JSCLLIntOffsetsExtractor.build/Objects-normal/x86_64/LLIntOffsetsExtractor.o llint/LLIntOffsetsExtractor.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler > cd /Users/fujii/work/webkit/g1/Source/JavaScriptCore > export LANG=en_US.US-ASCII > /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang -x c++ -arch x86_64 -fmessage-length=0 -fdiagnostics-show-note-include-stack -fmacro-backtrace-limit=0 -std=gnu++14 -stdlib=libc++ -Wno-trigraphs -fno-exceptions -fno-rtti -fno-sanitize=vptr -fpascal-strings -O3 -fno-common -Werror -Wno-missing-field-initializers -Wmissing-prototypes -Wdocumentation -Wunreachable-code -Wnon-virtual-dtor -Wno-overloaded-virtual -Wno-exit-time-destructors -Wno-missing-braces -Wparentheses -Wswitch -Wunused-function -Wno-unused-label -Wno-unused-parameter -Wunused-variable -Wunused-value -Wempty-body -Wuninitialized -Wno-unknown-pragmas -Wno-shadow -Wno-four-char-constants -Wno-conversion -Wconstant-conversion -Wint-conversion -Wbool-conversion -Wenum-conversion -Wno-float-conversion -Wnon-literal-null-conversion -Wobjc-literal-conversion -Wsign-compare -Wno-shorten-64-to-32 -Wnewline-eof -Wno-c++11-extensions -DNDEBUG -DENABLE_3D_TRANSFORMS -DENABLE_APPLE_PAY -DENABLE_APPLE_PAY_SESSION_V3 -DENABLE_APPLE_PAY_SESSION_V4 -DENABLE_APPLICATION_MANIFEST -DENABLE_ATTACHMENT_ELEMENT -DENABLE_AVF_CAPTIONS -DENABLE_CACHE_PARTITIONING -DENABLE_CHANNEL_MESSAGING -DENABLE_CONTENT_FILTERING -DENABLE_CSS_BOX_DECORATION_BREAK -DENABLE_CSS_COMPOSITING -DENABLE_CSS_PAINTING_API -DENABLE_CSS_SCROLL_SNAP -DENABLE_CSS_SELECTORS_LEVEL4 -DENABLE_CSS_TRAILING_WORD -DENABLE_CURSOR_VISIBILITY -DENABLE_CSS_CONIC_GRADIENTS -DENABLE_DARK_MODE_CSS -DENABLE_DASHBOARD_SUPPORT -DENABLE_DATACUE_VALUE -DENABLE_DATALIST_ELEMENT -DENABLE_ENCRYPTED_MEDIA -DENABLE_EXPERIMENTAL_FEATURES -DENABLE_FILTERS_LEVEL_2 -DENABLE_FTL_JIT -DENABLE_FULLSCREEN_API -DENABLE_GAMEPAD -DENABLE_GEOLOCATION -DENABLE_ICONDATABASE -DENABLE_INDEXED_DATABASE -DENABLE_INDEXED_DATABASE_IN_WORKERS -DENABLE_INPUT_TYPE_COLOR -DENABLE_INTERSECTION_OBSERVER -DENABLE_INTL -DENABLE_KEYBOARD_CODE_ATTRIBUTE -DENABLE_KEYBOARD_KEY_ATTRIBUTE -DENABLE_LEGACY_CSS_VENDOR_PREFIXES -DENABLE_LEGACY_CUSTOM_PROTOCOL_MANAGER -DENABLE_LEGACY_ENCRYPTED_MEDIA -DENABLE_MATHML -DENABLE_MEDIA_CONTROLS_SCRIPT -DENABLE_MEDIA_SOURCE -DENABLE_MEDIA_STREAM -DENABLE_METER_ELEMENT -DENABLE_MOUSE_CURSOR_SCALE -DENABLE_NOTIFICATIONS -DENABLE_PAYMENT_REQUEST -DENABLE_PDFKIT_PLUGIN -DENABLE_POINTER_LOCK -DENABLE_PUBLIC_SUFFIX_LIST -DENABLE_REMOTE_INSPECTOR -DENABLE_RESOURCE_USAGE -DENABLE_RUBBER_BANDING -DENABLE_SERVICE_CONTROLS -DENABLE_SERVICE_WORKER -DENABLE_SPEECH_SYNTHESIS -DENABLE_STREAMS_API -DENABLE_SUBTLE_CRYPTO -DENABLE_SVG_FONTS -DENABLE_TELEPHONE_NUMBER_DETECTION -DENABLE_TEXT_AUTOSIZING -DENABLE_USER_MESSAGE_HANDLERS -DENABLE_USERSELECT_ALL -DENABLE_VARIATION_FONTS -DENABLE_VIDEO -DENABLE_VIDEO_PRESENTATION_MODE -DENABLE_VIDEO_TRACK -DENABLE_VIDEO_USES_ELEMENT_FULLSCREEN -DENABLE_WEB_AUDIO -DENABLE_WEB_AUTHN -DENABLE_WEB_RTC -DENABLE_WEBGL -DENABLE_WEBGL2 -DENABLE_WEBGPU -DENABLE_WEBMETAL -DENABLE_WIRELESS_PLAYBACK_TARGET -DENABLE_XSLT -DU_HIDE_DEPRECATED_API -DU_DISABLE_RENAMING=1 -DU_SHOW_CPLUSPLUS_API=0 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk -fasm-blocks -fstrict-aliasing -Wdeprecated-declarations -Winvalid-offsetof -mmacosx-version-min=10.14 -g -fvisibility=hidden -fvisibility-inlines-hidden -fno-threadsafe-statics -Wno-sign-conversion -Winfinite-recursion -Wmove -Wcomma -Wblock-capture-autoreleasing -Wstrict-prototypes -Wrange-loop-analysis -Wno-semicolon-before-method-body -iquote /Users/fujii/work/webkit/g1/WebKitBuild/JavaScriptCore.build/Release/JSCLLIntOffsetsExtractor.build/JSCLLIntOffsetsExtractor-generated-files.hmap -I/Users/fujii/work/webkit/g1/WebKitBuild/JavaScriptCore.build/Release/JSCLLIntOffsetsExtractor.build/JSCLLIntOffsetsExtractor-own-target-headers.hmap -I/Users/fujii/work/webkit/g1/WebKitBuild/JavaScriptCore.build/Release/JSCLLIntOffsetsExtractor.build/JSCLLIntOffsetsExtractor-all-target-headers.hmap -iquote /Users/fujii/work/webkit/g1/WebKitBuild/JavaScriptCore.build/Release/JSCLLIntOffsetsExtractor.build/JSCLLIntOffsetsExtractor-project-headers.hmap -I/Users/fujii/work/webkit/g1/WebKitBuild/Release/include -I/Users/fujii/work/webkit/g1/WebKitBuild/Release/DerivedSources/JavaScriptCore -I/Users/fujii/work/webkit/g1/WebKitBuild/Release/LLIntOffsets/x86_64 -I/System/Library/Frameworks/JavaScriptCore.framework/PrivateHeaders -I. -I/Users/fujii/work/webkit/g1/WebKitBuild/Release/usr/local/include -I/Users/fujii/work/webkit/g1/WebKitBuild/JavaScriptCore.build/Release/JSCLLIntOffsetsExtractor.build/DerivedSources/x86_64 -I/Users/fujii/work/webkit/g1/WebKitBuild/JavaScriptCore.build/Release/JSCLLIntOffsetsExtractor.build/DerivedSources -Wall -Wextra -Wcast-qual -Wchar-subscripts -Wextra-tokens -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -F/Users/fujii/work/webkit/g1/WebKitBuild/Release -isystem /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/System.framework/PrivateHeaders -MMD -MT dependencies -MF /Users/fujii/work/webkit/g1/WebKitBuild/JavaScriptCore.build/Release/JSCLLIntOffsetsExtractor.build/Objects-normal/x86_64/LLIntOffsetsExtractor.d --serialize-diagnostics /Users/fujii/work/webkit/g1/WebKitBuild/JavaScriptCore.build/Release/JSCLLIntOffsetsExtractor.build/Objects-normal/x86_64/LLIntOffsetsExtractor.dia -c /Users/fujii/work/webkit/g1/Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp -o /Users/fujii/work/webkit/g1/WebKitBuild/JavaScriptCore.build/Release/JSCLLIntOffsetsExtractor.build/Objects-normal/x86_64/LLIntOffsetsExtractor.o > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp:29: > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/bytecode/ArrayProfile.h:29: > /Users/fujii/work/webkit/g1/Source/JavaScriptCore/runtime/Structure.h:135:30: error: inline function 'JSC::Structure::create' is not defined [-Werror,-Wundefined-inline] > inline static Structure* create(VM&, JSGlobalObject*, JSValue prototype, const TypeInfo&, const ClassInfo*, IndexingType = NonArray, unsigned inlineCapacity = 0); > ^ > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp:31: > In file included from /Users/fujii/work/webkit/g1/WebKitBuild/Release/DerivedSources/JavaScriptCore/BytecodeStructs.h:32: > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/bytecode/BytecodeDumper.h:29: > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/bytecode/CallLinkInfo.h:31: > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/jit/PolymorphicCallStubRoutine.h:30: > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/bytecode/CallEdge.h:28: > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/bytecode/CallVariant.h:28: > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/runtime/FunctionExecutable.h:29: > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/runtime/ScriptExecutable.h:28: > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/runtime/ExecutableBase.h:33: > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/runtime/JSGlobalObject.h:24: > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/bytecode/ArrayAllocationProfile.h:29: > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/runtime/JSArray.h:25: > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/runtime/ButterflyInlines.h:30: > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/runtime/JSObject.h:32: > /Users/fujii/work/webkit/g1/Source/JavaScriptCore/runtime/CustomGetterSetter.h:56:27: note: used here > return Structure::create(vm, globalObject, prototype, TypeInfo(CustomGetterSetterType, StructureFlags), info()); > ^ > 1 error generated. This means JSC::Structure::create might happen to be not inlined, but become a normal function call.

(In reply to Fujii Hironori from comment #18) > If I mark JSC::Structure::create declaration 'inline', mac port > can't compile. (it is using -Werror to convert warnings to > errors. I tested with r238365.) > > > CompileC /Users/fujii/work/webkit/g1/WebKitBuild/JavaScriptCore.build/Release/JSCLLIntOffsetsExtractor.build/Objects-normal/x86_64/LLIntOffsetsExtractor.o llint/LLIntOffsetsExtractor.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler > > cd /Users/fujii/work/webkit/g1/Source/JavaScriptCore ... /Users/fujii/work/webkit/g1/Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp -o /Users/fujii/work/webkit/g1/WebKitBuild/JavaScriptCore.build/Release/JSCLLIntOffsetsExtractor.build/Objects-normal/x86_64/LLIntOffsetsExtractor.o > > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp:29: > > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/bytecode/ArrayProfile.h:29: > > /Users/fujii/work/webkit/g1/Source/JavaScriptCore/runtime/Structure.h:135:30: error: inline function 'JSC::Structure::create' is not defined [-Werror,-Wundefined-inline] > > inline static Structure* create(VM&, JSGlobalObject*, JSValue prototype, const TypeInfo&, const ClassInfo*, IndexingType = NonArray, unsigned inlineCapacity = 0); > > ^ > > In file included from /Users/fujii/work/webkit/g1/Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp:31: ... > > /Users/fujii/work/webkit/g1/Source/JavaScriptCore/runtime/CustomGetterSetter.h:56:27: note: used here > > return Structure::create(vm, globalObject, prototype, TypeInfo(CustomGetterSetterType, StructureFlags), info()); > > ^ > > 1 error generated. > > This means JSC::Structure::create might happen to be not inlined, but become > a normal function call. Interesting. However, because we were not getting link errors before, this only means that all these files (that now have a compilation error) does not really need the implementation of Structure::create() i.e. Structure::create() might be referenced by other inline methods, but those methods were never use in these files; else, we would have seen a link error. I can see someone "fixing" this compilation error by naively *removing* the inline declaration on Structure::create() (especially since it appears to be redundant). I still think a comment would help reduce the chance of this regressing, but I understand that the inline declaration you added does help raise a significant red flag to deter such a move. I believe I've made my point. I'll leave it to you as to whether you want to add the comment or not.

Thank you very much for the explanation. It makes sense. I am going to add the comment.

Committed r238392: <https://trac.webkit.org/changeset/238392>