Bug 217585 - REGRESSION(r267727): Warning spam from JSC_DECLARE_CUSTOM_GETTER
Summary: REGRESSION(r267727): Warning spam from JSC_DECLARE_CUSTOM_GETTER
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:
 
Reported: 2020-10-11 13:50 PDT by Michael Catanzaro
Modified: 2020-10-27 15:13 PDT (History)
14 users (show)

See Also:


Attachments
Patch (227.77 KB, patch)
2020-10-11 14:54 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (240.84 KB, patch)
2020-10-11 15:04 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.09 KB, patch)
2020-10-27 11:04 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 2020-10-11 13:50:14 PDT
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);
      |        ^~~~~~~~~~~~~~~~~~~~~~~~~
Comment 1 Michael Catanzaro 2020-10-11 14:12:43 PDT
I think the problem is that the declarations are static, so WTF_INTERNAL is redundant with that. (File-static declarations are always hidden.)
Comment 2 Michael Catanzaro 2020-10-11 14:36:10 PDT
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.
Comment 3 Michael Catanzaro 2020-10-11 14:54:32 PDT
Created attachment 411065 [details]
Patch
Comment 4 Michael Catanzaro 2020-10-11 15:04:33 PDT
Created attachment 411067 [details]
Patch
Comment 5 Michael Catanzaro 2020-10-14 11:28:38 PDT
Ping JSC reviewers
Comment 6 Yusuke Suzuki 2020-10-15 16:51:46 PDT
Comment on attachment 411067 [details]
Patch

r=me
Comment 7 EWS 2020-10-16 07:12:13 PDT
Committed r268587: <https://trac.webkit.org/changeset/268587>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411067 [details].
Comment 8 Radar WebKit Bug Importer 2020-10-16 07:13:22 PDT
<rdar://problem/70376946>
Comment 9 Michael Catanzaro 2020-10-27 11:03:48 PDT
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.
Comment 10 Michael Catanzaro 2020-10-27 11:04:07 PDT
Created attachment 412446 [details]
Patch
Comment 11 EWS 2020-10-27 11:47:00 PDT
Committed r269058: <https://trac.webkit.org/changeset/269058>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412446 [details].
Comment 12 Sam Weinig 2020-10-27 13:44:55 PDT
(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.
Comment 13 Carlos Alberto Lopez Perez 2020-10-27 14:27:21 PDT
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
Comment 14 Michael Catanzaro 2020-10-27 15:13:02 PDT
Surprise, sorry about that! Lauro fixed the tests in r269069. Thanks Lauro.