[Bug c++/114309] New: Undesirable warning with [[unlikely]]

[Bug c++/114309] New: Undesirable warning with [[unlikely]]

[terra at gnome dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309

            Bug ID: 114309
           Summary: Undesirable warning with [[unlikely]]
           Product: gcc
           Version: 13.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: terra at gnome dot org
  Target Milestone: ---

g++ warns over the following program which uses [[unlikely]] for aborting error
reporting and conditionally chooses between two error messages:

# /usr/local/products/gcc/13.1.0/bin/g++ -O2 -c ttt-if-dual-unlikely.C
ttt-if-dual-unlikely.C: In function ‘void if_dual_warning(int)’:
ttt-if-dual-unlikely.C:3:19: warning: both branches of ‘if’ statement marked as
‘unlikely’ [-Wattributes]
    3 | #define barf(msg) [[unlikely]] crash(msg)
      |                   ^~~~~~~~~~~~
ttt-if-dual-unlikely.C:22:5: note: in expansion of macro ‘barf’
   22 |     barf("foo");
      |     ^~~~

g++ is correct that both branches have [[unlikely]].  What is not correct is to
warn over it.  g++ should instead simply infer that the second "if" is itself
unlikely to be reached.

The standard, quoted from
https://en.cppreference.com/w/cpp/language/attributes/likely, clearly
contemplates this case:

"Applies to a statement to allow the compiler to optimize for the case where
paths of execution including that statement are less likely than any
alternative path of execution that does not include such a statement."

Note that the standard expressions itself in terms of "paths of execution"
whereas g++ appears to have a narrower "branches of `if'" world view.  I am not
sure whether that's relevant.

Issuing this warning is a made a bit worse by the lack of a simple, local way
to suppress the warning in the same way that "if ((var = val)) { ... }" is a
way to suppress the warning about assignment in condition.


# cat ttt-if-dual-unlikely.C
#include <iostream>

#define barf(msg) [[unlikely]] crash(msg)

void
crash (const char*msg)
{
  std::cerr << msg << std::endl;
  abort ();
}


void
if_dual_warning (int i)
{
  bool runtime_cond0 = i > 0;
  bool runtime_cond1 = i > 1;

  if (runtime_cond0) {
    std::cerr << "Something\n";
  } else if (runtime_cond1) {
    barf("foo");
  } else {
    barf("bar");
  }
}

[Bug c++/114309] Undesirable warning with [[unlikely]]

[terra at gnome dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309

--- Comment #1 from M Welinder <terra at gnome dot org> ---
Created attachment 57672
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57672&action=edit
Preprocessed source code

[Bug c++/114309] Undesirable warning with [[unlikely]]

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The warning was added when this attribute was added in r9-4186-g2674fa47de9ecf
and even added a testcase for this warning g++.dg/cpp2a/attr-likely4.C .

[Bug c++/114309] Undesirable warning with [[unlikely]]

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
https://gcc.gnu.org/pipermail/gcc-patches/2018-November/510776.html

```
Would you please consider an error diagnostics for situations written in
test4.C?
Such situation is then silently ignored in profile_estimate pass:

Predictions for bb 2
  hot label heuristics of edge 2->4 (edge pair duplicate): 85.00%
  hot label heuristics of edge 2->3 (edge pair duplicate): 85.00%
```

[Bug c++/114309] Undesirable warning with [[unlikely]]

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think the warning is very much desirable.  It is not an error, just a warning
that the code does something weird.

[Bug c++/114309] Undesirable warning with [[unlikely]]

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Speficially this email:
https://gcc.gnu.org/pipermail/gcc-patches/2018-November/511208.html

[Bug c++/114309] Undesirable warning with [[unlikely]]

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #4)
> I think the warning is very much desirable.  It is not an error, just a
> warning that the code does something weird.

Maybe it should have its own enable/disable and not tied to -Wattribute though.

[Bug c++/114309] Undesirable warning with [[unlikely]]

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Also this works just fine to disable the warning around the unlikely:
#define push_warning _Pragma("GCC diagnostic push")
#define pop_warning _Pragma("GCC diagnostic pop")
#define disable_warning _Pragma("GCC diagnostic ignored  \"-Wattributes\"")

#define barf(msg) do { push_warning disable_warning [[unlikely]] crash(msg);
pop_warning } while(0)

[Bug c++/114309] Undesirable warning with [[unlikely]]

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #7)
> Also this works just fine to disable the warning around the unlikely:
> #define push_warning _Pragma("GCC diagnostic push")
> #define pop_warning _Pragma("GCC diagnostic pop")
> #define disable_warning _Pragma("GCC diagnostic ignored  \"-Wattributes\"")
> 
> #define barf(msg) do { push_warning disable_warning [[unlikely]] crash(msg);
> pop_warning } while(0)

Actually you don't even need the push/pop/disable.

This is enough to disable the warning:

#define barf(msg) do {  [[unlikely]] crash(msg); } while(0)

[Bug c++/114309] Undesirable warning with [[unlikely]]

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to M Welinder from comment #0)
> The standard, quoted from
> https://en.cppreference.com/w/cpp/language/attributes/likely, clearly
> contemplates this case:

N.B. cppreference is not the standard, and the text you quote is paraphrasing a
note from the standard, it's not the standard's actual wording.

> Note that the standard expressions itself in terms of "paths of execution"
> whereas g++ appears to have a narrower "branches of `if'" world view.  I am
> not sure whether that's relevant.

I don't think it's relevant, no. The "paths of execution" case covers switch
cases and things that aren't `if`, but since your code uses `if` that's what
GCC's warning refers to.

Anyway, in GCC's testcase we have:

   9   if (a == 123)
  10     [[likely]] c = 5;           // { dg-warning "both" }
  11   else
  12     [[likely]] b = 77;

Here we have two possible paths, and the attributes tell the compiler that the
first is more likely than the second, and the second is more likely than the
first. Obviously that's suspicious.

But code in comment 0 is more like:

  if (a)
    ;
  else if (b) [[unlikely]]
    ;
  else [[unlikely]]
    ;

In this case, not all paths through the function are marked unlikely. A
compiler treats the code above as:

  if (a)
    ;
  else {
    if (b) [[unlikely]]
      ;
    else [[unlikely]]
      ;
  }

And here it's certainly true that both branches of the second `if` are marked.
But many users do not think of it like this, they consider a series of if-else
branches to all be alternatives to each other. GCC seems to be thinking too
locally and not considering the surrounding context. There are paths through
the function that never even reach the second `if` and so there are paths of
execution that the compiler should consider to be more likely than any that
reach the second `if`. So I do think this is a bug.


Maybe the compiler should treat that code like:

  if (a)
    ;
  else {
    [[unlikely]] if (b)
      ;
    else
      ;
  }

i.e. if both branches are [[unlikely]] then "hoist" the attribute to before the
`if`. That doesn't warn, because now the compiler sees that there's another
branch not marked unlikely.

Aside: It might make more sense to mark the crash function as [[gnu::cold]]
instead of marking calls to it as unlikely.

[Bug c++/114309] Undesirable warning with [[unlikely]]

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #6)
> Maybe it should have its own enable/disable and not tied to -Wattribute
> though.

Yes, -Wattributes is going to keep covering more and more different things as
new attributes get added. It doesn't really make sense to group them together.

[Bug c++/114309] Undesirable warning with [[unlikely]]

[terra at gnome dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309

--- Comment #11 from M Welinder <terra at gnome dot org> ---
> Anyway, in GCC's testcase we have:
>
>   9   if (a == 123)
>  10     [[likely]] c = 5;           // { dg-warning "both" }
>  11   else
>  12     [[likely]] b = 77;

> Here we have two possible paths, and the attributes tell the compiler that the
> first is more likely than the second, and the second is more likely than the
> first. Obviously that's suspicious.

That interpretation is contrary to the C++ standard.

Ok, so I don't actually have the final standard but here's the proposed
wording: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0479r5.html

"Note: The use of the likely attribute is intended to allow implementations to
optimize for the case where paths of execution including it are arbitrarily
more likely than any alternative path of execution that does not include such
an attribute on a statement or label."

The use of "[[likely]]" in the above example therefore does not say that either
branch is more likely than the other.  Since both paths of execution include
"[[likely]]" the standard has no opinion on the matter.

"[[likely]]" and "[[unlikely]]" are not a perfect fit with the old
"builtin_expect".  They are more expressive and not tied to "if":
   {
     [[likely]] foo();
     [[unlikely]] bar();
   }
No "if" in sight, yet this tells us that foo is likely to be called, but that
it is unlikely to return normally.

Unrelatedly, the do-while work-around is fine for the simplified case I posted.
 It won't work in the much more complicated case where this comes from because
that hairy macro allows printing extra information:

   barf("foo") << something << " and " << something_else;