Bug 226557

<rdar://problem/79103658>

OK, at first I thought this was yet another false-positive, but now I see why GCC is complaining. Here is PackedAlignedPtr::get in Packed.h: T* get() const { // FIXME: PackedPtr<> can load memory with one mov by checking page boundary. // https://bugs.webkit.org/show_bug.cgi?id=197754 uintptr_t value = 0; #if CPU(LITTLE_ENDIAN) memcpy(&value, m_storage.data(), storageSize); #else memcpy(bitwise_cast<uint8_t*>(&value) + (sizeof(void*) - storageSize), m_storage.data(), storageSize); #endif if (isAlignmentShiftProfitable) value <<= alignmentShiftSize; #if CPU(X86_64) && !(OS(DARWIN) || OS(LINUX) || OS(WINDOWS)) // The AMD specification requires that the most significant 16 // bits of any virtual address, bits 48 through 63, must be // copies of bit 47 (in a manner akin to sign extension). // // The above-named OSes will never allocate user space addresses // with bit 47 set, thus are already in canonical form. // // Reference: https://en.wikipedia.org/wiki/X86-64#Virtual_address_space_details constexpr unsigned shiftBits = countOfBits<uintptr_t> - OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH); value = (bitwise_cast<intptr_t>(value) << shiftBits) >> shiftBits; #endif return bitwise_cast<T*>(value); } The only lines that matter are here: uintptr_t value = 0; #if CPU(LITTLE_ENDIAN) memcpy(&value, m_storage.data(), storageSize); Here storageSize is 5, so we copy five bytes of data from m_storage.data() to &value, which the compiler expects to be an array but is actually a uintptr_t declared on the stack. So I guess the compiler is reasonable to treat one uintptr_t as a zero-length memory region and emit the warning. We know this ought to be OK for 64-bit systems because we know that uintptr_t will be 8-bytes long, so it's not really zero-length. It is clearly intentional, so the warning should be suppressed manually. The safety depends on (a) storageSize <= sizeof(uintptr_t) -- so maybe we should add a static_assert for that --, and (b) compiler not deciding the memcpy is undefined behavior that can be "optimized" away.

Created attachment 431366 [details] Patch

Committed r278878 (238821@main): <https://commits.webkit.org/238821@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 431366 [details].

Reopening because I forgot to suppress the -Wstringop-overread warning that appears when the -Warray-bounds warning is suppressed. The analysis in comment #2 applies just the same to this: [2629/5494] Building CXX object Source...ources/UnifiedSource-4babe430-49.cpp.o In file included from WTF/Headers/wtf/text/StringImpl.h:32, from WTF/Headers/wtf/text/WTFString.h:31, from ../../Source/WebCore/dom/Exception.h:30, from ../../Source/WebCore/dom/ExceptionOr.h:29, from ../../Source/WebCore/dom/Event.h:29, from ../../Source/WebCore/Modules/websockets/CloseEvent.h:33, from ../../Source/WebCore/Modules/websockets/CloseEvent.cpp:27, from WebCore/DerivedSources/unified-sources/UnifiedSource-4babe430-49.cpp:1: In member function ‘T* WTF::PackedAlignedPtr<T, <anonymous> >::get() const [with T = JSC::SharedArrayBufferContents; long unsigned int passedAlignment = 1]’, inlined from ‘WTF::PackedAlignedPtr<T, <anonymous> >::operator bool() const [with T = JSC::SharedArrayBufferContents; long unsigned int passedAlignment = 1]’ at WTF/Headers/wtf/Packed.h:197:48, inlined from ‘WTF::RefPtr<T, <template-parameter-1-2>, <template-parameter-1-3> >::operator WTF::RefPtr<T, <template-parameter-1-2>, <template-parameter-1-3> >::UnspecifiedBoolType() const [with T = JSC::SharedArrayBufferContents; _PtrTraits = WTF::PackedPtrTraits<JSC::SharedArrayBufferContents>; _RefDerefTraits = WTF::DefaultRefDerefTraits<JSC::SharedArrayBufferContents>]’ at WTF/Headers/wtf/RefPtr.h:91:57, inlined from ‘bool JSC::ArrayBufferContents::isShared() const’ at JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBuffer.h:84:36, inlined from ‘bool JSC::ArrayBuffer::isShared() const’ at JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBuffer.h:202:31, inlined from ‘WTF::RefPtr<JSC::ArrayBuffer> JSC::ArrayBufferView::unsharedBuffer() const’ at JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBufferView.h:60:9, inlined from ‘WebCore::ExceptionOr<void> WebCore::WebSocket::send(JSC::ArrayBufferView&)’ at ../../Source/WebCore/Modules/websockets/WebSocket.cpp:389:52: WTF/Headers/wtf/Packed.h:147:15: warning: ‘void* memcpy(void*, const void*, size_t)’ reading 6 bytes from a region of size 0 [-Wstringop-overread] 147 | memcpy(&value, m_storage.data(), storageSize); | ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ We know it's "safe" because storageSize <= sizeof(uintptr_t), and since r278878 we now have a static_assert to guarantee that. But the compiler not-unreasonably thinks it's bad because value is not an array.

Created attachment 432395 [details] Patch

Comment on attachment 432395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432395&action=review > Source/WTF/wtf/Packed.h:149 > +IGNORE_WARNINGS_BEGIN("stringop-overread") It doesn't work. GCC is just ignoring the pragma. We might have to rewrite this code.

Created attachment 432610 [details] Patch

Comment on attachment 432610 [details] Patch Ugh, I made a last-minute improvement that reintroduced the -Wstringop-overread warning that this patch was intended to fix. This warning is brutal. I don't understand why the usual warning pragmas are unable to suppress it. Normally they work for all warnings except those implemented by the preprocessor (cpp)....

We'll use -Wno-array-bounds to eliminate the -Warray-bounds warning, bug #228601. That will introduce the -Wstringop-truncation warning. The only solution I found is -Wno-stringop-truncation. So far I've tested using it globally, but since this warning does not have an excessive amount of false positives like -Warray-bounds does, I think we can safely remove WebSocket.cpp from the unified build and apply the flag only to this file.

Created attachment 434570 [details] Patch

Ah, I need to add WebSocket.cpp to the XCode project, because that is a thing.

Created attachment 434621 [details] Patch

Created attachment 434622 [details] Patch

Created attachment 434624 [details] Patch

(In reply to Michael Catanzaro from comment #15) > Created attachment 434624 [details] > Patch If this doesn't work, I'm going to give up on editing the XCode project, and suppress -Wstringop-overread globally instead.

(In reply to Michael Catanzaro from comment #16) > If this doesn't work, I'm going to give up on editing the XCode project I am defeated. :/

Maybe an Apple developer would be willing to help with removing WebSocket.cpp from the unified build? Otherwise, I will suppress -Wstringop-truncation globally when built with GCC in bug #228601. It's very frustrating that it cannot be suppressed with pragmas. :/

(In reply to Michael Catanzaro from comment #18) > Maybe an Apple developer would be willing to help with removing > WebSocket.cpp from the unified build? Otherwise, I will suppress > -Wstringop-truncation globally when built with GCC in bug #228601. It's very > frustrating that it cannot be suppressed with pragmas. :/ Darin handled this in bug #228808. Thanks Darin!

Created attachment 434985 [details] Patch

Committed r280778 (240362@main): <https://commits.webkit.org/240362@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 434985 [details].

Reopening. When building with LTO enabled, there are more warnings here: [5489/5749] Linking CXX executable bin/TestWebKitAPI/TestWebCore /home/mcatanzaro/Projects/WebKit/Source/ThirdParty/ANGLE/src/compiler/translator/ParseContext.h:33: note: type name ‘sh::TParseContext’ should match type name ‘angle::pp::Tokenizer::Context’ 33 | class TParseContext : angle::NonCopyable | /home/mcatanzaro/Projects/WebKit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/Tokenizer.h:25: note: the incompatible type is defined here 25 | struct Context | In member function ‘get’, inlined from ‘__conv_op ’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WTF/Headers/wtf/Packed.h:198:48, inlined from ‘__conv_op ’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WTF/Headers/wtf/RefPtr.h:89:57, inlined from ‘isShared’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBuffer.h:93:36, inlined from ‘isShared’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBuffer.h:211:31, inlined from ‘unsharedBuffer’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBufferView.h:61:9, inlined from ‘send’ at /home/mcatanzaro/Projects/WebKit/Source/WebCore/Modules/websockets/WebSocket.cpp:386:52, inlined from ‘operator()’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WebCore/DerivedSources/JSWebSocket.cpp:537:5, inlined from ‘toJS’ at /home/mcatanzaro/Projects/WebKit/Source/WebCore/bindings/js/JSDOMConvertBase.h:168:18, inlined from ‘jsWebSocketPrototypeFunction_send2Body’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WebCore/DerivedSources/JSWebSocket.cpp:537:5, inlined from ‘jsWebSocketPrototypeFunction_sendOverloadDispatcher’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WebCore/DerivedSources/JSWebSocket.cpp:578:13, inlined from ‘call’ at /home/mcatanzaro/Projects/WebKit/Source/WebCore/bindings/js/JSDOMOperation.h:63:9, inlined from ‘jsWebSocketPrototypeFunction_send’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WebCore/DerivedSources/JSWebSocket.cpp:588:96: /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WTF/Headers/wtf/Packed.h:146:15: warning: ‘__builtin_memcpy’ reading 6 bytes from a region of size 0 [-Wstringop-overread] 146 | memcpy(&value, m_storage.data(), storageSize); | ^ [5490/5749] Linking CXX shared library lib/libwebkit2gtk-4.1.so.0.1.0 /home/mcatanzaro/Projects/WebKit/Source/ThirdParty/ANGLE/src/compiler/translator/ParseContext.h:33: note: type name ‘sh::TParseContext’ should match type name ‘angle::pp::Tokenizer::Context’ 33 | class TParseContext : angle::NonCopyable | /home/mcatanzaro/Projects/WebKit/Source/ThirdParty/ANGLE/src/compiler/preprocessor/Tokenizer.h:25: note: the incompatible type is defined here 25 | struct Context | In member function ‘get’, inlined from ‘__conv_op ’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WTF/Headers/wtf/Packed.h:198:48, inlined from ‘__conv_op ’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WTF/Headers/wtf/RefPtr.h:89:57, inlined from ‘isShared’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBuffer.h:93:36, inlined from ‘isShared’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBuffer.h:211:31, inlined from ‘unsharedBuffer’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/JavaScriptCore/PrivateHeaders/JavaScriptCore/ArrayBufferView.h:61:9, inlined from ‘send’ at /home/mcatanzaro/Projects/WebKit/Source/WebCore/Modules/websockets/WebSocket.cpp:386:52, inlined from ‘operator()’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WebCore/DerivedSources/JSWebSocket.cpp:537:5, inlined from ‘toJS’ at /home/mcatanzaro/Projects/WebKit/Source/WebCore/bindings/js/JSDOMConvertBase.h:168:18, inlined from ‘jsWebSocketPrototypeFunction_send2Body’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WebCore/DerivedSources/JSWebSocket.cpp:537:5, inlined from ‘jsWebSocketPrototypeFunction_sendOverloadDispatcher’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WebCore/DerivedSources/JSWebSocket.cpp:578:13, inlined from ‘call’ at /home/mcatanzaro/Projects/WebKit/Source/WebCore/bindings/js/JSDOMOperation.h:63:9, inlined from ‘jsWebSocketPrototypeFunction_send’ at /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WebCore/DerivedSources/JSWebSocket.cpp:588:96: /home/mcatanzaro/Projects/WebKit/WebKitBuild/GNOME/WTF/Headers/wtf/Packed.h:146:15: warning: ‘memcpy’ reading 6 bytes from a region of size 0 [-Wstringop-overread] 146 | memcpy(&value, m_storage.data(), storageSize); | ^ I'm not sure if it is possible to suppress them only for affected source files because they are derived sources and would have to be somehow removed from the unified build. Might need to revert r240362 and suppress -Wstringop-overread globally when building with GCC, since this is yet another case where pragmas fail to suppress the warning.

I'm going to turn off -Wstringop-overread for all of WebKit when building with GCC 11 or newer. Note this particular instance of the warning is a false-positive because we read storageSize bytes from m_storage.data(), and m_storage.data() is guaranteed to storageSize, and m_storage is a std::array of size storageSize. So this seems is an extremely simple case, where it is obvious that GCC is wrong, but we can't use the IGNORE_WARNINGS scalpel, and therefore must resort to the sledgehammer....

(In reply to Michael Catanzaro from comment #22) > Might need to revert r240362 I meant 240362@main, which is r280778.

In general, when we have these kinds of warning issues, I think it’s OK to first turn off the warning globally (for the particular problematic compiler, and no more) and then try to restore the warning, but without the time pressure caused by having builds failing with -Werror. Do you think that can work? Of course we are worried that eventually we’ll have many useful warnings left turned off, but the harder we work at it, the more we can restore those warnings. Meanwhile, we’ll still have a project that can build.

Note we don't use -Werror at all currently, but I'm trying to change that in bug #155047, so this does block the effort to enable it. (In reply to Darin Adler from comment #25) > In general, when we have these kinds of warning issues, I think it’s OK to > first turn off the warning globally (for the particular problematic > compiler, and no more) I have a patch to turn it off specifically for GCC 11 and newer versions. > and then try to restore the warning, but without the > time pressure caused by having builds failing with -Werror. Realistically, it will require changes in GCC. I've complained to GCC devs and we'll see what they say.

Created attachment 443980 [details] Patch

Committed r285652 (244150@main): <https://commits.webkit.org/244150@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 443980 [details].