Bug 217585 – REGRESSION(r267727): Warning spam from JSC_DECLARE_CUSTOM_GETTER

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Catanzaro]

I'm still trying to figure out what's going on here and how to solve it:

[2545/6600] Building CXX object Source/JavaScriptCore/CMa.../JavaScriptCore.dir/API/glib/JSAPIWrapperObjectGLib.cpp.o
In file included from ../../Source/JavaScriptCore/runtime/JSExportMacros.h:32,
                 from ../../Source/JavaScriptCore/config.h:33,
                 from ../../Source/JavaScriptCore/API/glib/JSAPIWrapperObjectGLib.cpp:27:
DerivedSources/ForwardingHeaders/wtf/ExportMacros.h:45:58: warning: ‘visibility’ attribute ignored [-Wattributes]
   45 | #define WTF_INTERNAL __attribute__((visibility("hidden")))
      |                                                          ^
DerivedSources/ForwardingHeaders/wtf/PlatformCallingConventions.h:105:85: note: in expansion of macro ‘WTF_INTERNAL’
  105 |     returnType JIT_OPERATION_ATTRIBUTES functionName parameters REFERENCED_FROM_ASM WTF_INTERNAL
      |                                                                                     ^~~~~~~~~~~~
DerivedSources/ForwardingHeaders/wtf/PlatformCallingConventions.h:107:49: note: in expansion of macro ‘JSC_DECLARE_JIT_OPERATION’
  107 | #define JSC_DECLARE_CUSTOM_GETTER(functionName) JSC_DECLARE_JIT_OPERATION(functionName, JSC::EncodedJSValue, (JSC::JSGlobalObject*, JSC::EncodedJSValue, JSC::PropertyName))
      |                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/JavaScriptCore/API/glib/JSAPIWrapperObjectGLib.cpp:72:8: note: in expansion of macro ‘JSC_DECLARE_CUSTOM_GETTER’
   72 | static JSC_DECLARE_CUSTOM_GETTER(callbackGetterJSAPIWrapperObjectCallbackObject);
      |        ^~~~~~~~~~~~~~~~~~~~~~~~~
DerivedSources/ForwardingHeaders/wtf/ExportMacros.h:45:58: warning: ‘visibility’ attribute ignored [-Wattributes]
   45 | #define WTF_INTERNAL __attribute__((visibility("hidden")))
      |                                                          ^
DerivedSources/ForwardingHeaders/wtf/PlatformCallingConventions.h:105:85: note: in expansion of macro ‘WTF_INTERNAL’
  105 |     returnType JIT_OPERATION_ATTRIBUTES functionName parameters REFERENCED_FROM_ASM WTF_INTERNAL
      |                                                                                     ^~~~~~~~~~~~
DerivedSources/ForwardingHeaders/wtf/PlatformCallingConventions.h:107:49: note: in expansion of macro ‘JSC_DECLARE_JIT_OPERATION’
  107 | #define JSC_DECLARE_CUSTOM_GETTER(functionName) JSC_DECLARE_JIT_OPERATION(functionName, JSC::EncodedJSValue, (JSC::JSGlobalObject*, JSC::EncodedJSValue, JSC::PropertyName))
      |                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~
../../Source/JavaScriptCore/API/glib/JSAPIWrapperObjectGLib.cpp:73:8: note: in expansion of macro ‘JSC_DECLARE_CUSTOM_GETTER’
   73 | static JSC_DECLARE_CUSTOM_GETTER(staticFunctionGetterJSAPIWrapperObjectCallbackObject);
      |        ^~~~~~~~~~~~~~~~~~~~~~~~~

[Michael Catanzaro]

I think the problem is that the declarations are static, so WTF_INTERNAL is redundant with that. (File-static declarations are always hidden.)

[Michael Catanzaro]

I considered remove WTF_INTERNAL from JSC_DECLARE_JIT_OPERATION and ensuring all uses are static, but it's not possible because it is frequently used in header files. It should only be static when used in .cpp files. Current usage appears to be correct.

So I decided to split JSC_DECLARE_JIT_OPERATION into two versions, the current one that uses WTF_INTERNAL, and JSC_DECLARE_JIT_OPERATION_WITHOUT_WTF_INTERNAL one that doesn't. That is not a great name, but it's only used in a couple places, so I guess that's OK. JSC_DECLARE_CUSTOM_GETTER and JSC_DECLARE_CUSTOM_SETTER use this new version, since those are only used in static declarations in .cpp files. JSDollarVM.cpp also needs the version without the WTF_INTERNAL. All other users can continue to use original JSC_DECLARE_JIT_OPERATION with WTF_INTERNAL.

[Michael Catanzaro]

Created attachment 411065 [details]
Patch

[Michael Catanzaro]

Created attachment 411067 [details]
Patch

[Michael Catanzaro]

Ping JSC reviewers

[Yusuke Suzuki]

Comment on attachment 411067 [details]
Patch

r=me

[EWS]

Committed r268587: <https://trac.webkit.org/changeset/268587>

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

[Radar WebKit Bug Importer]

<rdar://problem/70376946>

[Michael Catanzaro]

Looks like I missed a spot because I only tested through JSC compilation to ensure the warnings were fixed, but there is one place in WebCore bindings generation that causes warnings in various generated files. Reopening to attach follow-up patch.

[Michael Catanzaro]

Created attachment 412446 [details]
Patch

[EWS]

Committed r269058: <https://trac.webkit.org/changeset/269058>

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

[Sam Weinig]

(In reply to EWS from comment #11)
> Committed r269058: <https://trac.webkit.org/changeset/269058>
> 
> All reviewed patches have been landed. Closing bug and clearing flags on
> attachment 412446 [details].

Looks like this broke the bindings tests. Can you please look into updating the results.

[Carlos Alberto Lopez Perez]

Log of broken bindings-generation-tests on gtk-release test bot: https://build.webkit.org/builders/GTK-Linux-64-bit-Release-Tests/builds/16671/steps/bindings-generation-tests/logs/stdio

[Michael Catanzaro]

Surprise, sorry about that! Lauro fixed the tests in r269069. Thanks Lauro.