Bug 226193

Summary: Fix more GCC warnings
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: aperez, benjamin, bugs-noreply, cdumez, cmarcelo, ews-watchlist, keith_miller, mark.lam, mcatanzaro, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Linux   
See Also: https://bugs.webkit.org/show_bug.cgi?id=226557
https://bugs.webkit.org/show_bug.cgi?id=226643
Bug Depends on:    
Bug Blocks: 155047    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Michael Catanzaro 2021-05-24 15:42:58 PDT
Let's fix another round of newly-introduced GCC warnings.
Comment 1 Michael Catanzaro 2021-05-24 15:52:44 PDT
One of these is really frustrating me. I wonder if anybody else sees a problem with this code, or whether GCC's warning is just bogus here:

[643/1468] Building CXX object Source/WebCore/CMakeFiles/W...vedSources/unified-sources/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:3:
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:194: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:390:52:
WTF/Headers/wtf/Packed.h:143:15: warning: ‘void* memcpy(void*, const void*, size_t)’ offset [0, 5] is out of the bounds [0, 0] [-Warray-bounds]
  143 |         memcpy(static_cast<void*>(&value), static_cast<void*>(const_cast<uint8_t*>(m_storage.data())), storageSize);
      |         ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


GCC's -Warray-bounds warning is frankly pretty crappy. It's usually a false-positive, so I normally wouldn't worry too much about suppressing it. But in this case, suppressing it introduces a -Wstringop-overread warning:

[641/1468] Building CXX object Source/WebCore/CMakeFiles/W...vedSources/unified-sources/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:3:
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:194: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:390:52:
WTF/Headers/wtf/Packed.h:143:15: warning: ‘void* memcpy(void*, const void*, size_t)’ reading 6 bytes from a region of size 0 [-Wstringop-overread]
  143 |         memcpy(static_cast<void*>(&value), static_cast<void*>(const_cast<uint8_t*>(m_storage.data())), storageSize);
      |         ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


And that has got me worried, because -Wstringop-overread is a good warning and I do not remember seeing a false-positive -Wstringop-overread before. But I also do not see any problems with this code. The code is not going crazy here: it seems to be careful to check the size of its buffers. Maybe I am missing something?

Anyway, I assume they are both false-positives, because I don't see anything wrong, but I'm calling them out because they leave me a little nervous.

(There's also a new -Wnonnull in JITCall.cpp complaining that the "this" pointer can be nullptr in JIT::compileOpCall. As usual, GCC shows no evidence for why we should believe this extraordinary claim. If it's possible, it is far from clear bbhow.)
Comment 2 Michael Catanzaro 2021-05-24 15:55:11 PDT
Created attachment 429581 [details]
Patch
Comment 3 Michael Catanzaro 2021-05-24 16:00:53 PDT
(CC: Robin for reminder about -Wclass-memaccess. The way to suppress when you're doing something weird and intentionally want to avoid running constructors/destructors is to static_cast<void*>() when passing any object pointer to memset()/memcpy() etc.)
Comment 4 Michael Catanzaro 2021-05-26 05:30:52 PDT
Ping reviewers
Comment 5 Michael Catanzaro 2021-06-02 15:17:24 PDT
Comment on attachment 429581 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429581&action=review

> Source/WTF/wtf/Packed.h:149
> +// https://bugs.webkit.org/show_bug.cgi?id=226193
> +IGNORE_GCC_WARNINGS_BEGIN("stringop-overread")
> +IGNORE_GCC_WARNINGS_BEGIN("array-bounds")
>  #if CPU(LITTLE_ENDIAN)
> -        memcpy(&value, m_storage.data(), storageSize);
> +        memcpy(static_cast<void*>(&value), static_cast<void*>(const_cast<uint8_t*>(m_storage.data())), storageSize);
>  #else
> -        memcpy(bitwise_cast<uint8_t*>(&value) + (sizeof(void*) - storageSize), m_storage.data(), storageSize);
> +        memcpy(static_cast<void*>(bitwise_cast<uint8_t*>(&value) + (sizeof(void*) - storageSize)), static_cast<void*>(const_cast<uint8_t*>(m_storage.data())), storageSize);
>  #endif
> +IGNORE_GCC_WARNINGS_END
> +IGNORE_GCC_WARNINGS_END

IMO this merits an entirely separate bug report.

Let's use this bug for the easy warnings.
Comment 6 Michael Catanzaro 2021-06-02 15:42:15 PDT
(In reply to Michael Catanzaro from comment #5)
> IMO this merits an entirely separate bug report.

Bug #226557

> Let's use this bug for the easy warnings.

There are several more than last week, because of course there are.
Comment 7 Michael Catanzaro 2021-06-02 16:03:27 PDT
Created attachment 430415 [details]
Patch
Comment 8 Robin Morisset 2021-06-02 16:44:33 PDT
(In reply to Michael Catanzaro from comment #3)
> (CC: Robin for reminder about -Wclass-memaccess. The way to suppress when
> you're doing something weird and intentionally want to avoid running
> constructors/destructors is to static_cast<void*>() when passing any object
> pointer to memset()/memcpy() etc.)

Thanks for the tip!
Comment 9 Michael Catanzaro 2021-06-03 11:06:48 PDT
Ping reviewers, let's land this please.
Comment 10 Yusuke Suzuki 2021-06-03 12:20:48 PDT
Comment on attachment 430415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430415&action=review

> Source/JavaScriptCore/jit/JITCall.cpp:258
> +IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_BEGIN
>      auto slowPaths = info->emitFastPath(*this, regT0, regT2, CallLinkInfo::UseDataIC::Yes);
> +IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_END

This looks not great to me. Can you bypass this warning without messing up the code?
Comment 11 Michael Catanzaro 2021-06-03 12:43:43 PDT
(In reply to Yusuke Suzuki from comment #10)
> This looks not great to me. Can you bypass this warning without messing up
> the code?

Not that I know of. We have a special suppression macro that says ERRONEOUS for a reason, unfortunately. :( The null check warning is for "this" and I assume it's bogus. If you see some way "this" can plausibly be null there, then of course that would be a disaster that we ought to fix, but GCC isn't going to give us any hints as to how it might be possible. I'll note that normally when "this" becomes invalid during a function call, it's normally a use-after-free vulnerability where "this" is left dangling because it got destroyed somewhere else by mistake. It would be *very* unusual for "this" to somehow become set to null.

I'm tempted to disable -Wnonnull globally when building with GCC, because since GCC 11 it's just a bunch of false-positives.
Comment 12 Michael Catanzaro 2021-06-03 12:54:33 PDT
Comment on attachment 430415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430415&action=review

>> Source/JavaScriptCore/jit/JITCall.cpp:258
>> +IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_END
> 
> This looks not great to me. Can you bypass this warning without messing up the code?

Adrian suggested adding an assert, so I tried: RELEASE_ASSERT(this) right before the call. This made it worse by introducing a long chain of -Wnonnull-compare warnings and didn't fix the original -Wnonnull. FWIW the full warning with our current unmodified code is:

[3/3414] Building CXX object Source/JavaScriptCore/CMakeF...vedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp.o
In file included from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp:7:
../../Source/JavaScriptCore/jit/JITCall.cpp: In member function ‘void JSC::JIT::compileOpCall(const JSC::Instruction*, unsigned int) [with Op = JSC::OpCallEval]’:
../../Source/JavaScriptCore/jit/JITCall.cpp:256:10: warning: ‘this’ pointer is null [-Wnonnull]
  256 |     auto slowPaths = info->emitFastPath(*this, regT0, regT2, CallLinkInfo::UseDataIC::Yes);
      |          ^~~~~~~~~
In file included from ../../Source/JavaScriptCore/bytecode/CodeBlock.h:34,
                 from ../../Source/JavaScriptCore/jit/AssemblyHelpers.h:30,
                 from ../../Source/JavaScriptCore/jit/CCallHelpers.h:30,
                 from ../../Source/JavaScriptCore/jit/JITAddGenerator.h:30,
                 from ../../Source/JavaScriptCore/jit/JITAddGenerator.cpp:27,
                 from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp:1:
../../Source/JavaScriptCore/bytecode/CallLinkInfo.h:178:30: note: in a call to non-static member function ‘JSC::AbstractMacroAssembler<JSC::X86Assembler>::JumpList JSC::CallLinkInfo::emitFastPath(JSC::CCallHelpers&, JSC::GPRReg, JSC::GPRReg, JSC::CallLinkInfo::UseDataIC)’
  178 |     MacroAssembler::JumpList emitFastPath(CCallHelpers&, GPRReg calleeGPR, GPRReg callLinkInfoGPR, UseDataIC) WARN_UNUSED_RETURN;
      |                              ^~~~~~~~~~~~
Comment 13 Michael Catanzaro 2021-06-03 12:56:40 PDT
Ugh sorry, it's warning that *info* might be null, it's only "this" inside the call to CallLinkInfo::emitFastPath. So confusing. :/ Let me try RELEASE_ASSERT(info) and see what happens....
Comment 14 Michael Catanzaro 2021-06-03 13:33:05 PDT
Comment on attachment 430415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430415&action=review

>>> Source/JavaScriptCore/jit/JITCall.cpp:258
>>> +IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_END
>> 
>> This looks not great to me. Can you bypass this warning without messing up the code?
> 
> Adrian suggested adding an assert, so I tried: RELEASE_ASSERT(this) right before the call. This made it worse by introducing a long chain of -Wnonnull-compare warnings and didn't fix the original -Wnonnull. FWIW the full warning with our current unmodified code is:
> 
> [3/3414] Building CXX object Source/JavaScriptCore/CMakeF...vedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp.o
> In file included from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp:7:
> ../../Source/JavaScriptCore/jit/JITCall.cpp: In member function ‘void JSC::JIT::compileOpCall(const JSC::Instruction*, unsigned int) [with Op = JSC::OpCallEval]’:
> ../../Source/JavaScriptCore/jit/JITCall.cpp:256:10: warning: ‘this’ pointer is null [-Wnonnull]
>   256 |     auto slowPaths = info->emitFastPath(*this, regT0, regT2, CallLinkInfo::UseDataIC::Yes);
>       |          ^~~~~~~~~
> In file included from ../../Source/JavaScriptCore/bytecode/CodeBlock.h:34,
>                  from ../../Source/JavaScriptCore/jit/AssemblyHelpers.h:30,
>                  from ../../Source/JavaScriptCore/jit/CCallHelpers.h:30,
>                  from ../../Source/JavaScriptCore/jit/JITAddGenerator.h:30,
>                  from ../../Source/JavaScriptCore/jit/JITAddGenerator.cpp:27,
>                  from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp:1:
> ../../Source/JavaScriptCore/bytecode/CallLinkInfo.h:178:30: note: in a call to non-static member function ‘JSC::AbstractMacroAssembler<JSC::X86Assembler>::JumpList JSC::CallLinkInfo::emitFastPath(JSC::CCallHelpers&, JSC::GPRReg, JSC::GPRReg, JSC::CallLinkInfo::UseDataIC)’
>   178 |     MacroAssembler::JumpList emitFastPath(CCallHelpers&, GPRReg calleeGPR, GPRReg callLinkInfoGPR, UseDataIC) WARN_UNUSED_RETURN;
>       |                              ^~~~~~~~~~~~

OK, RELEASE_ASSERT() works after all. So we can do:

RELEASE_ASSERT(info);
auto slowPaths = info->emitFastPath(*this, regT0, regT2, CallLinkInfo::UseDataIC::Yes);

Yusuke, do you prefer that to using IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_BEGIN?

Now that I realize GCC is complaining about "info" rather than "this" -- it's highlighting the totally wrong portion of the line there and I claim its error message is incorrect, it should say "info" rather than "this" -- well it makes more sense why it's complaining, and I'm less-inclined to label this case erroneous. It sure does look like info can be null there. Could you look at this case please?
Comment 15 Adrian Perez 2021-06-04 00:38:08 PDT
Comment on attachment 430415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430415&action=review

>>>> Source/JavaScriptCore/jit/JITCall.cpp:258
>>>> +IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_END
>>> 
>>> This looks not great to me. Can you bypass this warning without messing up the code?
>> 
>> Adrian suggested adding an assert, so I tried: RELEASE_ASSERT(this) right before the call. This made it worse by introducing a long chain of -Wnonnull-compare warnings and didn't fix the original -Wnonnull. FWIW the full warning with our current unmodified code is:
>> 
>> [3/3414] Building CXX object Source/JavaScriptCore/CMakeF...vedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp.o
>> In file included from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp:7:
>> ../../Source/JavaScriptCore/jit/JITCall.cpp: In member function ‘void JSC::JIT::compileOpCall(const JSC::Instruction*, unsigned int) [with Op = JSC::OpCallEval]’:
>> ../../Source/JavaScriptCore/jit/JITCall.cpp:256:10: warning: ‘this’ pointer is null [-Wnonnull]
>>   256 |     auto slowPaths = info->emitFastPath(*this, regT0, regT2, CallLinkInfo::UseDataIC::Yes);
>>       |          ^~~~~~~~~
>> In file included from ../../Source/JavaScriptCore/bytecode/CodeBlock.h:34,
>>                  from ../../Source/JavaScriptCore/jit/AssemblyHelpers.h:30,
>>                  from ../../Source/JavaScriptCore/jit/CCallHelpers.h:30,
>>                  from ../../Source/JavaScriptCore/jit/JITAddGenerator.h:30,
>>                  from ../../Source/JavaScriptCore/jit/JITAddGenerator.cpp:27,
>>                  from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-3a3c4ec0-3.cpp:1:
>> ../../Source/JavaScriptCore/bytecode/CallLinkInfo.h:178:30: note: in a call to non-static member function ‘JSC::AbstractMacroAssembler<JSC::X86Assembler>::JumpList JSC::CallLinkInfo::emitFastPath(JSC::CCallHelpers&, JSC::GPRReg, JSC::GPRReg, JSC::CallLinkInfo::UseDataIC)’
>>   178 |     MacroAssembler::JumpList emitFastPath(CCallHelpers&, GPRReg calleeGPR, GPRReg callLinkInfoGPR, UseDataIC) WARN_UNUSED_RETURN;
>>       |                              ^~~~~~~~~~~~
> 
> OK, RELEASE_ASSERT() works after all. So we can do:
> 
> RELEASE_ASSERT(info);
> auto slowPaths = info->emitFastPath(*this, regT0, regT2, CallLinkInfo::UseDataIC::Yes);
> 
> Yusuke, do you prefer that to using IGNORE_ERRONEOUS_GCC_NULL_CHECK_WARNINGS_BEGIN?
> 
> Now that I realize GCC is complaining about "info" rather than "this" -- it's highlighting the totally wrong portion of the line there and I claim its error message is incorrect, it should say "info" rather than "this" -- well it makes more sense why it's complaining, and I'm less-inclined to label this case erroneous. It sure does look like info can be null there. Could you look at this case please?

I had a suspicion that asserting that something is non-null might make the compiler
assume that the value will be non-NULL afterwards — the first time I saw this kind
of behavior was in the Rust compiler a few years ago, which was mind-blowing because
then it also went ahead and did additional optimizations possible under the asserted
assumption. C/C++ were not that smart then :]

This being the only change there are doubts about, I think we can land the patch
as-is sans this change so we can avoid most of the warning spam during compilation
sooner; then do a follow-up to address this one.

Michael, feel free to land the patch *without* this first, if you feel so inclined.

By all means if you or Yusuke manage to figure out whether “info“ can be NULL (or not)
here, updating the patch is also an option — sadly, I won't have time today to help
with that.
Comment 16 Michael Catanzaro 2021-06-04 07:25:29 PDT
Created attachment 430574 [details]
Patch for landing
Comment 17 EWS 2021-06-04 08:03:07 PDT
Committed r278458 (238478@main): <https://commits.webkit.org/238478@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430574 [details].
Comment 18 Michael Catanzaro 2021-06-04 13:54:15 PDT
There are two remaining warnings: bug #226557 and bug #226643.