Bug 1948775 - Spurious -Wnonnull warning after __builtin_expect
Summary: Spurious -Wnonnull warning after __builtin_expect
Keywords:
Status: NEW
Alias: None
Product: Fedora
Classification: Fedora
Component: gcc
Version: 37
Hardware: x86_64
OS: Linux
unspecified
low
Target Milestone: ---
Assignee: Jakub Jelinek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2021-04-12 21:37 UTC by Michael Catanzaro
Modified: 2022-08-09 13:11 UTC (History)
10 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
Type: Bug


Attachments (Terms of Use)
preprocessed source (3.36 MB, text/plain)
2021-04-12 21:37 UTC, Michael Catanzaro
no flags Details
RefPtr.h (8.61 KB, text/x-csrc)
2021-04-12 21:38 UTC, Michael Catanzaro
no flags Details
HashMap.cpp (32.09 KB, text/x-csrc)
2021-04-12 21:38 UTC, Michael Catanzaro
no flags Details


Links
System ID Private Priority Status Summary Last Updated
GNU Compiler Collection 100086 0 P3 NEW [11 Regression] spurious -Wnonnull with __builtin_expect 2021-04-14 20:34:10 UTC

Description Michael Catanzaro 2021-04-12 21:37:07 UTC
Description of problem: With GCC 11, we have a warning spam when building WebKit, coming from RefPtr.h. It looks like this:

[1019/5391] Building CXX object Tools/TestWebKitAPI/CMakeFiles/TestWTF.dir/Tests/WTF/HashMap.cpp.o
In file included from WTF/Headers/wtf/HashFunctions.h:26,
                 from ../../Tools/TestWebKitAPI/Tests/WTF/DeletedAddressOfOperator.h:28,
                 from ../../Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:30:
In static member function ‘static void WTF::DefaultRefDerefTraits< <template-parameter-1-1> >::derefIfNotNull(T*) [with T = TestWebKitAPI::RefLogger]’,
    inlined from ‘WTF::RefPtr<T, <template-parameter-1-2>, <template-parameter-1-3> >::~RefPtr() [with T = TestWebKitAPI::RefLogger; _PtrTraits = WTF::RawPtrTraits<TestWebKitAPI::RefLogger>; _RefDerefTraits = WTF::DefaultRefDerefTraits<TestWebKitAPI::RefLogger>]’ at WTF/Headers/wtf/RefPtr.h:73:61,
    inlined from ‘virtual void TestWebKitAPI::WTF_HashMap_Ref_Value_Test::TestBody()’ at ../../Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:977:36:
WTF/Headers/wtf/RefPtr.h:42:23: warning: ‘this’ pointer is null [-Wnonnull]
   42 |             ptr->deref();
      |             ~~~~~~~~~~^~
In file included from ../../Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:32:
../../Tools/TestWebKitAPI/Tests/WTF/RefLogger.h: In member function ‘virtual void TestWebKitAPI::WTF_HashMap_Ref_Value_Test::TestBody()’:
../../Tools/TestWebKitAPI/Tests/WTF/RefLogger.h:35:10: note: in a call to non-static member function ‘void TestWebKitAPI::RefLogger::deref()’
   35 |     void deref();
      |          ^~~~~

I believe this is a GCC bug because we just checked for null on the previous line.

template<typename T> struct DefaultRefDerefTraits {
    static ALWAYS_INLINE void refIfNotNull(T* ptr)
    {
        if (LIKELY(ptr != nullptr))
            ptr->ref();
    }

    static ALWAYS_INLINE void derefIfNotNull(T* ptr)
    {
        if (LIKELY(ptr != nullptr))
            ptr->deref(); // <------------ -Wnonnull warning here
    }
};

The preprocessed source (attached) looks like this:

    static inline __attribute__((__always_inline__)) void derefIfNotNull(T* ptr)
    {
        if (__builtin_expect(!!(ptr != nullptr), 1))
            ptr->deref();
    }

Version-Release number of selected component (if applicable): gcc-11.0.1-0.3.fc34


How reproducible: Always


Steps to Reproduce:
1. gcc HashMap.cpp.ii -O2 -Wnonnull -c

Actual results: warning prints


Expected results: no warning


Additional info: I'm attaching HashMap.cpp, HashMap.cpp.ii, and also RefPtr.h. Note it only happens with -O2, doesn't happen at lower optimization levels. Also does not happen if I remove the use of LIKELY()/__builtin_expect().

Comment 1 Michael Catanzaro 2021-04-12 21:37:30 UTC
Created attachment 1771439 [details]
preprocessed source

Comment 2 Michael Catanzaro 2021-04-12 21:38:34 UTC
Created attachment 1771450 [details]
RefPtr.h

Comment 3 Michael Catanzaro 2021-04-12 21:38:50 UTC
Created attachment 1771451 [details]
HashMap.cpp

Comment 4 Martin Sebor 2021-04-12 22:38:26 UTC
The warning works as designed, but it runs relatively early in the optimization pipeline in hopes of avoiding false positives, but as result, doesn't benefit from transformations that happen later on that might eliminate code subsequently determined to be unreachable.  That can, in turn, trigger false positives.

The warning doesn't try to compensate for this by analyzing the control flow and is simply issued for calls to member functions with constant null this pointers like the one in basic block 233 below (seen in the output of -fdump-tree-post_ipa_warn), without trying to figure out if the call may be unreachable.  In this case, one of the optimization that runs after the warning has been issued eliminates the invalid call (the last dump the call is seen in is thread1 and it's eliminated in vrp1).  This could and should be improved but it's too late for GCC 11.

;; Function TestWebKitAPI::WTF_HashMap_Ref_Value_Test::TestBody (_ZN13TestWebKitAPI26WTF_HashMap_Ref_Value_Test8TestBodyEv, funcdef_no=10522, decl_uid=259574, cgraph_uid=4096, symbol_order=5311)

void TestWebKitAPI::WTF_HashMap_Ref_Value_Test::TestBody (struct WTF_HashMap_Ref_Value_Test * const this)
{
  ...

  <bb 232> [local count: 92578498]:
  gtest_ar_ ={v} {CLOBBER};
  gtest_ar_ ={v} {CLOBBER};
  MEM[(struct RefLogger * &)&emptyTake] = 0B;
  _182 = (long int) _476;
  _169 = _182;
  if (_169 != 0)
    goto <bb 233>; [90.00%]
  else
    goto <bb 234>; [10.00%]

  <bb 233> [local count: 83320647]:
  TestWebKitAPI::RefLogger::deref (0B);   <<< -Wnonnull


If the warning is triggered by a test I would recommend suppressing it via #pragma GCC diagnostic.  I need to reduce the translation unit to a smaller test case to get a better idea of how to work around it in general (if it's possible).

Please note that -Wnonnull as well as all other flow-sensitive warnings are necessarily prone to false positives that we will never be able to completely eliminate.

Comment 5 Martin Sebor 2021-04-14 20:34:14 UTC
I finally reduced this monstrous translation unit to a small enough test case and submitted the following report upstream:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100086.

Comment 6 Martin Sebor 2021-04-14 23:06:36 UTC
To get back to the workaround idea, as you discovered removing __builtin_expect() changes the IL in a way that avoids the warning and even though it looks like it only makes a negligible difference to the emitted assembly on x86_64 I would not suggest it as a workaround.

If the warning comes up only in a test, I would suggest to suppress it there.  If it impacts other code as well, a strategically placed #pragma GCC diagnostic might help (but in GCC 11 the pragma isn't easy to use in a targeted way with inlining.)

Comment 7 Michael Catanzaro 2021-04-19 21:09:41 UTC
The pragma worked for me.

I spent some more time on this today and left a follow-up comment in https://bugs.webkit.org/show_bug.cgi?id=224452. TL;DR: I think the costs of the new behavior outweighs the benefit, because I don't see how I could ever track down what's causing the warnings. Maybe there really is some impressive way that the |this| pointer could wind up null in these cases, but if so, I would need to see a coverity-style walkthrough showing exactly how in order for the warning to be actionable. In contrast, traditional -Wnonnull warnings were quite simple, and false-positives were rare.

Comment 8 Martin Sebor 2021-04-20 15:00:45 UTC
The core -Wnonnull implementation hasn't changed recently.  What has changed in GCC is that the this pointer is implicitly considered nonnull.  In the test case I submitted upstream I replaced the this pointer with an argument to a nonmember nonnull function (puts) to show that the same warning is emitted regardless of the tighter GCC 11 checking.  So the underlying problem isn't new, and while GCC 11 will have more false positives, it should also find correspondingly more real bugs, and so have the same S/R ratio.

Comment 9 Ben Cotton 2022-05-12 15:31:43 UTC
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.

Comment 10 Ben Cotton 2022-08-09 13:11:45 UTC
This bug appears to have been reported against 'rawhide' during the Fedora Linux 37 development cycle.
Changing version to 37.


Note You need to log in before you can comment on or make changes to this bug.