[Bug c++/114309] Undesirable warning with [[unlikely]]
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309
--- Comment #11 from M Welinder ---
> 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;
[Bug c++/114309] Undesirable warning with [[unlikely]]
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309 --- Comment #10 from Jonathan Wakely --- (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]]
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309
--- Comment #9 from Jonathan Wakely ---
(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]]
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309
Andrew Pinski changed:
What|Removed |Added
Resolution|--- |INVALID
Status|UNCONFIRMED |RESOLVED
--- Comment #8 from Andrew Pinski ---
(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]]
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309
--- Comment #7 from Andrew Pinski ---
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]]
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309 --- Comment #6 from Andrew Pinski --- (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]]
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309 --- Comment #5 from Andrew Pinski --- Speficially this email: https://gcc.gnu.org/pipermail/gcc-patches/2018-November/511208.html
[Bug c++/114309] Undesirable warning with [[unlikely]]
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309 Jakub Jelinek changed: What|Removed |Added CC||jakub at gcc dot gnu.org --- Comment #4 from Jakub Jelinek --- 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]]
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309 --- Comment #3 from Andrew Pinski --- 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]]
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309 --- Comment #2 from Andrew Pinski --- 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]]
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114309 --- Comment #1 from M Welinder --- Created attachment 57672 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57672=edit Preprocessed source code
