Created attachment 1821311 [details] Preprocessed sources for two files Description of problem: Since enabling LTO for WebKitGTK in bug #1990111, we now have a warning spam caused by -Wodr. A few of these are real ODR violations, but most of them appear to be a false positive caused by our binding integrity feature that crashes the process if it detects that a vtable pointer has been corrupted. The bindings generator generates code that looks like this: extern "C" { extern void* _ZTVN7WebCore28UserMessageHandlersNamespaceE[]; } Note that's *not* a definition, it's just a declaration, so I don't understand why it would be an ODR violation. But that is definitely the line that triggers the warning: ../../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[]; } | Version-Release number of selected component (if applicable): gcc-11.2.1-1.fc34 How reproducible: Always Steps to Reproduce: 1. Build WebKitGTK with LTO enabled: -DCMAKE_C_FLAGS="-flto=auto" -DCMAKE_CXX_FLAGS="-flto=auto" Actual results: GCC prints -Wodr warnings that do not appear to correspond to actual ODR violations. Expected results: Only -Wodr warnings for actual ODR violations that are WebKit bugs. Additional info: Jakub requested preprocessed source for two of the files that trigger this issue. I'm not sure how to use them to reproduce the bug, but I've attached them. Note there are many dozens of affected files and many dozens of warnings, so I've just picked two to show a single -Wodr warning caused by "redefinition" of the vtable between the two files.
The warnings can't be suppressed with #pragma diagnostic ignored either. :S
(In reply to Michael Catanzaro from comment #0) > Note that's *not* a definition, it's just a declaration, so I don't > understand why it would be an ODR violation. I think it's because one is a vtable and one is an array of void*, so the types don't match. Maybe it's not strictly violating the one-DEFINITION rule, because there is still only one definition with that symbol name. But if that variable is declared then it's reasonable to assume there is a definition somewhere, and that would violate the ODR. Or maybe it shouldn't be part of the -Wodr warning and should be a -Wmismatched-types-of-symbols warning ... but it isn't, it's part of -Wodr.
No idea if it would work for your purposes, but could you declare a symbol with a different name, and make it an alias of the vtable symbol? extern "C" { extern void* __attribute__((alias("_ZTVN7WebCore28UserMessageHandlersNamespaceE"))) vtable_of_WebCore_UserMessageHandlersNamespace[]; } This has to be in the same translation unit as the vtable, which will be in the same TU as the key function (the first non-inline, non-pure, virtual function declared in the class).
Sadly that won't work. We could have the code generator do that for the generated code, no problem, but then it would be in the wrong translation unit. Doing it in the translation unit of the class itself, with the vtable and key function, would be too messy since that's not generated code.
(In reply to Michael Catanzaro from comment #1) > The warnings can't be suppressed with #pragma diagnostic ignored either. :S So this is the main problem: because the warnings are un-suppressible with pragmas, I think the only practical solution is to disable -Wodr globally. Or remove all affected files from unified build so that I can pass -Wodr specially to just those files, but that would be a non-starter for Apple as they don't use GCC and aren't going to want to reduce build performance to work around a GCC bug. I previously globally disabled -Warray-bounds and -Wnonnull (in https://bugs.webkit.org/show_bug.cgi?id=228601, largely due to a non-suppressible warning I fought in https://bugs.webkit.org/show_bug.cgi?id=226557), so -Wodr would be just one more such warning, but it's not an amazing result.
The problem with suppressing -Warray-bounds in GCC 11 and prior had to do with inlining (GCC didn't consider the full inlining stack so whether the pragma was effective or not depended on where in the inlining stack the warning occurred). It's fixed in GCC 12. The problem with -Wodr is quite likely different. As an aside, the -Warray-bounds/-Wstringop-overread analysis in https://bugs.webkit.org/show_bug.cgi?id=226557 doesn't sound right. The warnings are about the read access, not the store, so the problem is likely the definition of m_storage.data(). (It's fine to use memcpy() to write into a scalar like uintptr_t). When referring to scalar objects the warnings might refer to an array of one element. That's just for convenience because there is no semantic difference between the two; it has no bearing on false positives or negatives.
(In reply to Martin Sebor from comment #6) > As an aside, the -Warray-bounds/-Wstringop-overread analysis in > https://bugs.webkit.org/show_bug.cgi?id=226557 doesn't sound right. The > warnings are about the read access, not the store, so the problem is likely > the definition of m_storage.data(). (It's fine to use memcpy() to write > into a scalar like uintptr_t). When referring to scalar objects the > warnings might refer to an array of one element. That's just for > convenience because there is no semantic difference between the two; it has > no bearing on false positives or negatives. Yeah, "So I guess the compiler is reasonable to treat one uintptr_t as a zero-length memory region and emit the warning" is nonsense.
(In reply to Martin Sebor from comment #6) > The warnings are about the read access, not the store, Shouldn't it say so? There are two accesses, both of which need to be in bounds, and the warning doesn't tell you which one might be a problem.
(In reply to Martin Sebor from comment #6) > The > warnings are about the read access, not the store, so the problem is likely > the definition of m_storage.data(). Umm... but the length of m_storage.data() is storageSize, and that's also the length passed to memcpy. So to a human, it's obvious that nothing is wrong with the read. I understand GCC might not know that, but if so, it should be quiet and not risk emitting a false-positive warning. I guess we're a bit off-topic now. My fault, sorry. Do you want me to report a separate bug for this?
Yes, please submit a bug for each distinct problem, it helps us track and fix them. Ideally, if the problem can be reproduced with upstream GCC, submit it there (https://gcc.gnu.org/bugzilla). This includes missing detail like which function argument the access warning applies to, as Jonathan suggests. If you're not sure something's a bug (or some behavior/warning seems unhelpful), either bring it up on the gcc-help mailing list, or submit a bug anyway. The worst that might happen is that it will get closed as NOTABUG.
So I have to choose between (a) globally suppress -Wodr for all of WebKit, since it cannot be suppressed with pragmas (is this itself a GCC bug?) nor for particular files (because we do not want to remove these files from unified build), or else (b) disable WebKit's binding integrity feature. (b) seems out of the question, so I'm going to go with (a) I suppose. It's a real shame we cannot suppress the warnings with pragmas, like usual. (In reply to Martin Sebor from comment #10) > Yes, please submit a bug for each distinct problem, it helps us track and > fix them. Ideally, if the problem can be reproduced with upstream GCC, > submit it there (https://gcc.gnu.org/bugzilla). This includes missing > detail like which function argument the access warning applies to, as > Jonathan suggests. I don't want to build upstream GCC, sorry. I can copy/paste my bug reports here to upstream Bugzilla, but I don't have time to spend trying to get minimal reproducers. If you still want me to create an account on GCC Bugzilla, I can do that and forward these complaints there.
(In reply to Michael Catanzaro from comment #9) > Umm... but the length of m_storage.data() is storageSize, and that's also > the length passed to memcpy. So to a human, it's obvious that nothing is > wrong with the read. I understand GCC might not know that, but if so, it > should be quiet and not risk emitting a false-positive warning. Ugh, I know this is off-topic, but I feel like complaining somewhere: because this is yet another mysteriously non-suppressable warning, where #pragma GCC diagnostic ignored cannot turn it off, and because I'm not able to easily alter the build flags for the affected files (they are generated sources, and included in unified build), I'm going to turn off -Wstringop-overread for all of WebKit, same as -Warray-bounds and -Wnonnull. Warnings that don't respect pragmas are a real shame....
This message is a reminder that Fedora Linux 34 is nearing its end of life. Fedora will stop maintaining and issuing updates for Fedora Linux 34 on 2022-06-07. It is Fedora's policy to close all bug reports from releases that are no longer maintained. At that time this bug will be closed as EOL if it remains open with a 'version' of '34'. Package Maintainer: If you wish for this bug to remain open because you plan to fix it in a currently maintained version, change the 'version' to a later Fedora Linux version. Thank you for reporting this issue and we are sorry that we were not able to fix it before Fedora Linux 34 is end of life. If you would still like to see this bug fixed and are able to reproduce it against a later version of Fedora Linux, you are encouraged to change the 'version' to a later version prior to this bug being closed.
This bug appears to have been reported against 'rawhide' during the Fedora Linux 37 development cycle. Changing version to 37.
This message is a reminder that Fedora Linux 37 is nearing its end of life. Fedora will stop maintaining and issuing updates for Fedora Linux 37 on 2023-12-05. It is Fedora's policy to close all bug reports from releases that are no longer maintained. At that time this bug will be closed as EOL if it remains open with a 'version' of '37'. Package Maintainer: If you wish for this bug to remain open because you plan to fix it in a currently maintained version, change the 'version' to a later Fedora Linux version. Note that the version field may be hidden. Click the "Show advanced fields" button if you do not see it. Thank you for reporting this issue and we are sorry that we were not able to fix it before Fedora Linux 37 is end of life. If you would still like to see this bug fixed and are able to reproduce it against a later version of Fedora Linux, you are encouraged to change the 'version' to a later version prior to this bug being closed.
Fedora Linux 37 entered end-of-life (EOL) status on None. Fedora Linux 37 is no longer maintained, which means that it will not receive any further security or bug fix updates. As a result we are closing this bug. If you can reproduce this bug against a currently maintained version of Fedora Linux please feel free to reopen this bug against that version. Note that the version field may be hidden. Click the "Show advanced fields" button if you do not see the version field. If you are unable to reopen this bug, please file a new report against an active release. Thank you for reporting this bug and we are sorry it could not be fixed.