Wunreachable-code warns that [[likely]] and [[unlikely]] are unreachable

[nico]

Bugzilla Link	52103
Resolution	FIXED
Resolved on	Oct 21, 2021 12:33
Version	unspecified
OS	Linux
Blocks	#51489
CC	@AaronBallman,@dwblaikie,@imallett,@LebedevRI,@zygoloid,@Weverything,@tstellar
Fixed by commit(s)	c74ab84 2ac023c

Extended Description

$ cat foo.cc
int main(int argc, char* argv[]) {
if (argc > 100) [[unlikely]]
return 1;

return 0;
}

$ out/gn/bin/clang -Wunreachable-code -c foo.cc -std=c++20
foo.cc:2:19: warning: code will never be executed [-Wunreachable-code]
if (argc > 100) [[unlikely]]
^~~~~~~~~~~~

That's pretty silly.

(There's also bug 19020; not quite sure if this is a dupe of that or not.)

[nico]

assigned to @nico

[AaronBallman]

This smells somewhat related to #48798 . I haven't looked very deeply, but I wonder how well the CFG facilities support attributed statements.

[nico]

Only happens if the if contains a return. Doesn't fire on:

int main(int argc, char* argv[]) {
if (argc > 100) [[unlikely]] {
argc = 4;
}
return 0;
}

[nico]

I'll look at this for a bit.

Here are some useful commands:

% cat likely.cc
int main(int argc, char* argv[]) {
  if (argc > 100) [[unlikely]] {
    argc = 2;
    return 1;
  }

  if (argc > 100) [[unlikely]] {
    argc = 4;
    argc = 5;
  }

  return 0;
}

% out/gn/bin/clang -c likely.cc -Xclang -ast-dump

The ifs look like:

    |-IfStmt 0x7f9233881560 <line:2:3, line:5:3>
      ... condition omitted ...
    | `-AttributedStmt 0x7f9233881548 <col:19, line:5:3>
    |   |-UnlikelyAttr 0x7f9233881520 <line:2:21>
    |   `-CompoundStmt 0x7f9233881500 <col:32, line:5:3>
      ... body ...

So there's an AttributedStmt as body that contains the CompoundStmt.

# (needs static analyzer enabled in clang. seems a bit silly that
# there isn't a way to dump the cfg without that, but that's for
# another day...)
% out/gn/bin/clang -c likely.cc -Xclang -analyze -Xclang -analyzer-checker=debug.DumpCFG

Here's the basic block for the body of the 2nd if:

 [B2]
   1: 4
   2: argc
   3: [B2.2] = [B2.1]
   4: 5
   5: argc
   6: [B2.5] = [B2.4]
   7:  [[unlikely]]{
    [B2.3];
    [B2.6];
}
   Preds (1): B3
   Succs (1): B1

Here are the two basic blocks for the body of the 1st if:

 [B5]
   1: 2
   2: argc
   3: [B5.2] = [B5.1]
   4: 1
   5: return [B5.4];
   Preds (1): B6
   Succs (1): B0

 [B4]
   1:  [[unlikely]]{
    [B5.3];
[B5.5]}
   Succs (1): B3

So the AttributedStmt is inserted at the end of the basic block for currently unknown-to-me reasons (*), and since there's a return in the BB, that really ends up being unreachable in the CFG.

Maybe the AttributedStmt's attribute should show up at the start of the CFG BB? Maybe AttributedStmt was added for [[fallthrough]] originally and for that it makes sense at the end? (But [[fallthrough]] shouldn't have children.) Or maybe it just happens to fall out that way because the CFGBuilder builds the CFG backwards. Need to do some more reading.

[nico]

Looking through other StmtAttr entries in clang/include/clang/Basic/Attr.td, this incorrectly warns as well, for the same reasons:

% cat likely.cc
int null(int, char**) { return 0; }
int main(int argc, char* argv[]) {
  [[clang::musttail]] return null(argc, argv);
}

% out/gn/bin/clang -c likely.cc -Wunreachable-code-aggressive
likely.cc:3:3: warning: code will never be executed [-Wunreachable-code]
  [[clang::musttail]] return null(argc, argv);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

[nico]

https://reviews.llvm.org/rG84837d5b5aa0d added

    AC.getCFGBuildOptions()
      ...
      .setAlwaysAdd(Stmt::AttributedStmtClass);

to clang/lib/Sema/AnalysisBasedWarnings.cpp (for -Wimplicit-fallthrough / the FallThrough attr). (Actually I'm not sure why this is needed since addStmt() in CFG.cpp always passes AddStmtChoice::AlwaysAdd to Visit()?)

The default VisitStmt() / VisitChildren() in clang/lib/Analysis/CFG.cpp do:

CFGBlock *CFGBuilder::VisitStmt(Stmt *S, AddStmtChoice asc) {
  if (asc.alwaysAdd(*this, S)) {
    autoCreateBlock();
    appendStmt(Block, S);
  }

  return VisitChildren(S);
}

/// VisitChildren - Visit the children of a Stmt.
CFGBlock *CFGBuilder::VisitChildren(Stmt *S) {
  CFGBlock *B = Block;

  // Visit the children in their reverse order so that they appear in
  // left-to-right (natural) order in the CFG.
  reverse_children RChildren(S);
  for (Stmt *Child : RChildren) {
    if (Child)
      if (CFGBlock *R = Visit(Child))
        B = R;
  }
  return B;
}

So alwaysAdd() is always true for AttributedStmts and is always called before the children (and adds the AttributedStmt itself to the block), and the children are added in reverse order, and a jump statement child causes the block to be replaced.

That explains what we're seeing.

Options:

1. add VisitAttributedStmt() and make it add the AttributedStmt only if it contains a FallThrough attrib (which is only allowed on a NullStmt so there won't be unruly children). The other stmt attribs we currently have (some omp loop thing, likely/unlikely, and musttail) don't have much semantic meaning in the CFG

2. process child stmts first and add the AttributedStmt to the top of the block that's returned; that kind of makes semantic sense for likely/unlikely and musttail (and arguably the omp thing too)

[nico]

For the example from bug 49454:

  switch (argc) {
    [[likely]] case 0:
      [[clang::fallthrough]];
    case 3:
      break;
  }

Without that [[likely]], the CFG dump for that looks like so:

 [B4 (ENTRY)]
   Succs (1): B1

 [B1]
   1: argc
   2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
   T: switch [B1.2]
   Preds (1): B4
   Succs (3): B2 B3 B0

 [B2]
  case 3:
   T: break;
   Preds (2): B1 B3
   Succs (1): B0

 [B3]
  case 0:
   1:  [[clang::fallthrough]];
   Preds (1): B1
   Succs (1): B2

 [B0 (EXIT)]
   Preds (2): B2 B1

With the [[likely]], B3 becomes:

 [B3]
  case 0:
   1:  [[clang::fallthrough]];
   2:  [[likely]]case 0:
[B3.1]   Preds (1): B1
   Succs (1): B2

So it adds the full AttributedStmt with the case child to the BB for the case label itself. That's not great.

[nico]

patch
I'm reasonably happy with this approach. Just need to add tests.

[nico]

*** Bug #48798 has been marked as a duplicate of this bug. ***

[nico]

https://reviews.llvm.org/D111568

[LebedevRI]

This seems like a pretty bad bug.
Can the fix be picked into 13.x?

[AaronBallman]

This seems like a pretty bad bug.
Can the fix be picked into 13.x?

The fix seems safe enough to add into 13.x, but the issue existed before 13.0 so it feels less pressing to me: https://godbolt.org/z/dqMxTjdca

[nico]

[[likely]] is in c++20, which was ratified ~1 year ago. It'll likely become more prevalent over the window where llvm 13 is the active release, so merging makes sense to me. Tom, what's the process for llvm 13.x merges?

[nico]

(back to open to track the merge)

[tstellar]

[[likely]] is in c++20, which was ratified ~1 year ago. It'll likely become
more prevalent over the window where llvm 13 is the active release, so
merging makes sense to me. Tom, what's the process for llvm 13.x merges?

It will get backported automatically once I kick off the 13.0.1 release process.

[AaronBallman]

*** Bug #48769 has been marked as a duplicate of this bug. ***

[tstellar]

Merged: 2ac023c

[tstellar]

mentioned in issue #51489