WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
229867
-Wodr warning spam caused by ENABLE(BINDING_INTEGRITY)
https://bugs.webkit.org/show_bug.cgi?id=229867
Summary
-Wodr warning spam caused by ENABLE(BINDING_INTEGRITY)
Michael Catanzaro
Reported
2021-09-03 07:29:09 PDT
When LTO is enabled (using -flto=auto), GCC is able to report violations of C++'s one definition rule (ODR). Currently we have a huge spam of such warnings from WebCore caused by our ENABLE(BINDING_INTEGRITY) feature. CodeGeneratorJS.pm generates code that looks like this: extern "C" { extern void* ${vtableNameGnu}[]; } For example, JSTestGenerateIsReachable.cpp includes: extern "C" { extern void* _ZTVN7WebCore23TestGenerateIsReachableE[]; } It eventually gets used here: // If you hit this assertion you either have a use after free bug, or // ${implType} has subclasses. If ${implType} has subclasses that get passed // to toJS() we currently require $interfaceName you to opt out of binding hardening // by adding the SkipVTableValidation attribute to the interface IDL definition RELEASE_ASSERT(actualVTablePointer == expectedVTablePointer); This seems like a reasonable thing for us to do, and it does not seem like it should be an ODR violation because we are not actually defining the vtable symbol at all! We are merely declaring it. So I don't understand why GCC would be complaining about this. I'm tempted to consider this not a bug and suppress the warnings, but I want to check with the GCC developers first. Anyway, since this is caused by our code generation, there are separate warnings for each generated bindings file, which adds up to a nice spam. The first few look like this: ../../Source/WebCore/page/UserMessageHandlersNamespace.h:45: warning: virtual table of type ‘struct UserMessageHandlersNamespace’ violates one definition rule [-Wodr] 45 | class UserMessageHandlersNamespace : public RefCounted<UserMessageHandlersNamespace>, public FrameDestructionObserver, public UserContentProviderInvalidationClient { | WebCore/DerivedSources/JSUserMessageHandlersNamespace.cpp:270: note: variable of same assembler name as the virtual table is defined in another translation unit 270 | extern "C" { extern void* _ZTVN7WebCore28UserMessageHandlersNamespaceE[]; } | ../../Source/WebCore/page/UserMessageHandler.h:38: warning: virtual table of type ‘struct UserMessageHandler’ violates one definition rule [-Wodr] 38 | class UserMessageHandler : public RefCounted<UserMessageHandler>, public FrameDestructionObserver { | WebCore/DerivedSources/JSUserMessageHandler.cpp:251: note: variable of same assembler name as the virtual table is defined in another translation unit 251 | extern "C" { extern void* _ZTVN7WebCore18UserMessageHandlerE[]; } | ../../Source/WebCore/Modules/gamepad/GamepadEvent.h:36: warning: virtual table of type ‘struct GamepadEvent’ violates one definition rule [-Wodr] 36 | class GamepadEvent final : public Event { | WebCore/DerivedSources/JSGamepadEvent.cpp:304: note: variable of same assembler name as the virtual table is defined in another translation unit 304 | extern "C" { extern void* _ZTVN7WebCore12GamepadEventE[]; } | ../../Source/WebCore/xml/XMLHttpRequest.h:55: warning: virtual table of type ‘struct XMLHttpRequest’ violates one definition rule [-Wodr] 55 | class XMLHttpRequest final : public ActiveDOMObject, public RefCounted<XMLHttpRequest>, private ThreadableLoaderClient, public XMLHttpRequestEventTarget { | WebCore/DerivedSources/JSXMLHttpRequest.cpp:819: note: variable of same assembler name as the virtual table is defined in another translation unit 819 | extern "C" { extern void* _ZTVN7WebCore14XMLHttpRequestE[]; } | ../../Source/WebCore/xml/XMLHttpRequestUpload.h:33: warning: virtual table of type ‘struct XMLHttpRequestUpload’ violates one definition rule [-Wodr] 33 | class XMLHttpRequestUpload final : public XMLHttpRequestEventTarget { | WebCore/DerivedSources/JSXMLHttpRequestUpload.cpp:213: note: variable of same assembler name as the virtual table is defined in another translation unit 213 | extern "C" { extern void* _ZTVN7WebCore20XMLHttpRequestUploadE[]; } |
Attachments
Preprocessed sources for two affected translation units
(1.71 MB, application/x-xz)
2021-09-03 12:44 PDT
,
Michael Catanzaro
no flags
Details
Preprocessed sources for two affected translation units
(1.72 MB, application/x-xz)
2021-09-07 11:00 PDT
,
Michael Catanzaro
no flags
Details
Patch
(1.56 KB, patch)
2021-11-11 10:02 PST
,
Michael Catanzaro
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Michael Catanzaro
Comment 1
2021-09-03 12:43:00 PDT
I'm going to attach the preprocessed source for the two translation units that trigger this issue for the vtable of class UserMessageHandlersNamespace. We'll probably wind up needing a GCC bug report, but I don't have a Bugzilla account there, so I'll start here.
Michael Catanzaro
Comment 2
2021-09-03 12:44:57 PDT
Created
attachment 437300
[details]
Preprocessed sources for two affected translation units
Michael Catanzaro
Comment 3
2021-09-03 12:51:35 PDT
(In reply to Michael Catanzaro from
comment #2
)
> Created
attachment 437300
[details]
> Preprocessed sources for two affected translation units
Amazingly, I messed up and included a local change in this build: a pragma intended to suppress these warnings. It's in UnifiedSource-3a52ce78-115.cpp.ii: # 270 "WebCore/DerivedSources/JSUserMessageHandlersNamespace.cpp" #pragma GCC diagnostic push # 270 "WebCore/DerivedSources/JSUserMessageHandlersNamespace.cpp" # 270 "WebCore/DerivedSources/JSUserMessageHandlersNamespace.cpp" #pragma GCC diagnostic ignored "-W" "odr" # 270 "WebCore/DerivedSources/JSUserMessageHandlersNamespace.cpp" extern "C" { extern void* _ZTVN7WebCore28UserMessageHandlersNamespaceE[]; } # 272 "WebCore/DerivedSources/JSUserMessageHandlersNamespace.cpp" #pragma GCC diagnostic pop # 272 "WebCore/DerivedSources/JSUserMessageHandlersNamespace.cpp" Oops. I don't want to rebuild everything unless really needed, so best just manually delete the pragmas.
Michael Catanzaro
Comment 4
2021-09-07 11:00:40 PDT
Created
attachment 437536
[details]
Preprocessed sources for two affected translation units Uploading fixed preprocessed sources that are unmodified, no pragmas added this time
Michael Catanzaro
Comment 5
2021-09-07 13:42:25 PDT
I tried: diff --git a/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm b/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm index 1f24f1eb010b..014a99f87b8b 100644 --- a/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm +++ b/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm @@ -5106,7 +5106,9 @@ sub GenerateImplementation #pragma warning(disable: 4483) extern "C" { extern void (*const ${vtableRefWin}[])(); } #else +IGNORE_GCC_WARNINGS_BEGIN("odr") extern "C" { extern void* ${vtableNameGnu}[]; } +IGNORE_GCC_WARNINGS_END #endif #endif But apparently these warnings are not suppressible at all. :/
Michael Catanzaro
Comment 6
2021-09-07 13:50:39 PDT
(In reply to Michael Catanzaro from
comment #5
)
> But apparently these warnings are not suppressible at all. :/
Removing the generated files from unified build so that we can use -Wno-odr just for these files seems like a nonstarter to me. The only reasonable option seems to be to globally disable -Wodr, like I did for -Warray-bounds and -Wnonnull in
bug #228601
.
Radar WebKit Bug Importer
Comment 7
2021-09-10 09:14:12 PDT
<
rdar://problem/82975115
>
Michael Catanzaro
Comment 8
2021-11-11 07:55:09 PST
I think expecting a GCC solution to this is unrealistic. Since we cannot suppress these warnings, our only option is to either suppress -Wodr globally or else disable the bindings integrity feature. Opinions welcome, but I'm inclined to disable -Wodr. It's sad that this will obscure real ODR bugs, though, which are inevitable in a codebase as large as WebKit's (in fact, I fixed several earlier this year only thanks to this warning).
Michael Catanzaro
Comment 9
2021-11-11 10:02:09 PST
Created
attachment 443962
[details]
Patch
Michael Catanzaro
Comment 10
2021-11-22 11:05:53 PST
Ping reviewers
Michael Catanzaro
Comment 11
2021-11-29 09:29:45 PST
Ping reviewers
EWS
Comment 12
2022-02-28 08:33:47 PST
Committed
r290597
(?): <
https://commits.webkit.org/r290597
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 443962
[details]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug