Bug 229867 – -Wodr warning spam caused by ENABLE(BINDING_INTEGRITY)

• Component: Bindings
• Status: RESOLVED FIXED

[Michael Catanzaro]

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[]; }
      |

[Michael Catanzaro]

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]

Created attachment 437300 [details]
Preprocessed sources for two affected translation units

[Michael Catanzaro]

(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]

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]

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]

(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]

<rdar://problem/82975115>

[Michael Catanzaro]

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]

Created attachment 443962 [details]
Patch

[Michael Catanzaro]

Ping reviewers

[Michael Catanzaro]

Ping reviewers

[EWS]

Committed r290597 (?): <https://commits.webkit.org/r290597>

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