[Sema] Check calls to __attribute__((warn_unused_result)) from StmtExprs

[nickdesaulniers]

Summary

When a GNU C statement expression is used in a macro
Expr::isUnusedResultAWarning was returning true, but the block touched
in this commit from Sema::DiagnoseUnusedExprResult was returning without
emitting a Diag.

Fixes pr/45394.
Link: https://github.com/ClangBuiltLinux/linux/issues/968

[nickdesaulniers]

ah, using this to build a Linux kernel triggers tons of warnings. So the behavior of GCC is even more subtle than captured in the test cases here.

[dblaikie]

In D77611#1965824, @nickdesaulniers wrote:

ah, using this to build a Linux kernel triggers tons of warnings. So the behavior of GCC is even more subtle than captured in the test cases here.

At least one thing I noticed - GCC only warns about the first unused statement in a statement expression (whether or not it's in a macro):

$ g++-tot warn.cpp -Wunused-value -c
In file included from warn.cpp:1:
warn.cpp: In function ‘int main()’:
warn.cpp:2:14: warning: statement has no effect [-Wunused-value]
    2 | #define X ({ y; z; })
      |              ^
warn.cpp:6:15: note: in expansion of macro ‘X’
    6 |   assert(4 == X);
      |               ^
warn.cpp:7:3: warning: statement has no effect [-Wunused-value]
    7 |   y;
      |   ^
warn.cpp:8:3: warning: statement has no effect [-Wunused-value]
    8 |   z;
      |   ^
warn.cpp:9:4: warning: statement has no effect [-Wunused-value]
    9 | ({ y; z; });
      |    ^

[dblaikie]

(side note: Clang doesn't strive to emulate GCC's warnings bug-for-bug/exactly - certainly about tradeoffs in terms of true and false positives to decide what's appropriate for a clang warning)

[nickdesaulniers]

Note your test case is related to -Wunused-value, mine is -Wunused-result (via the use of __attribute__((warn_result_unused)) which we should always warn about.

[dblaikie]

In D77611#1967488, @nickdesaulniers wrote:

Note your test case is related to -Wunused-value, mine is -Wunused-result (via the use of __attribute__((warn_result_unused)) which we should always warn about.

Right, but the code you're looking at/proposing to change is related to all unused expression warnings, by the looks of it?

[nickdesaulniers]

• emit warning just for CallExprs

[nickdesaulniers]

clang/lib/Sema/SemaStmt.cpp

252

sigh, this again produces different from GCC for the kernel. Let me tighten this constraint up further.

[nickdesaulniers]

• tighten up condition, eagerly emit diag, test with kernel before pushing

[dblaikie]

Looks like GCC warns on everything but the last unused expression (the last one being the return statement) - only warning on that last one if it's ultimately unused by the statement expression function call, as it were (& is annotated warn_unused_result). Basically, it models it like a function in a way that Clang perhaps does not?

[nickdesaulniers]

In D77611#1968227, @dblaikie wrote:

Looks like GCC warns on everything but the last unused expression (the last one being the return statement) - only warning on that last one if it's ultimately unused by the statement expression function call, as it were (& is annotated warn_unused_result). Basically, it models it like a function in a way that Clang perhaps does not?

For the case of warn_unused_result, it doesn't matter whether the unused call is the first or last statement in the statement expression: https://godbolt.org/z/DDgsVC (notice that trunk of LLVM warns now, just not for the return value case from my added test).

So GCC treats the return value from statement expressions differently from return values from functions in this regard: https://godbolt.org/z/83wP7W

[dblaikie]

In D77611#1968236, @nickdesaulniers wrote:

In D77611#1968227, @dblaikie wrote:

Looks like GCC warns on everything but the last unused expression (the last one being the return statement) - only warning on that last one if it's ultimately unused by the statement expression function call, as it were (& is annotated warn_unused_result). Basically, it models it like a function in a way that Clang perhaps does not?

For the case of warn_unused_result, it doesn't matter whether the unused call is the first or last statement in the statement expression: https://godbolt.org/z/DDgsVC (notice that trunk of LLVM warns now, just not for the return value case from my added test).

Not sure I follow - if you swap the statements in the example there, the warning seems to go away. Compare (yours) https://godbolt.org/z/DDgsVC (warning with Clang and GCC) with https://godbolt.org/z/j65QXo (Clang and GCC do not warn).

So GCC treats the return value from statement expressions differently from return values from functions in this regard: https://godbolt.org/z/83wP7W

I don't think it does (based on above clarification/test cases). But your change would make Clang treat them differently from each other (& different from GCC), I think?

clang/lib/Sema/SemaStmt.cpp

void Sema::DiagnoseUnusedExprResult(const Stmt *S) {
   SourceLocation Loc;
   SourceRange R1, R2;
   if (!E->isUnusedResultAWarning(WarnExpr, Loc, R1, R2, Context))
     return;
 
   // If this is a GNU statement expression expanded from a macro, it is probably
   // unused because it is a function-like macro that can be used as either an
   // expression or statement.  Don't warn, because it is almost certainly a
-  // false positive.
-  if (isa<StmtExpr>(E) && Loc.isMacroID())
+  // false positive. But we do want to check CallExprs with
+  // WarnUnusedResultAttr.
+  if (isa<StmtExpr>(E) && Loc.isMacroID()) {
+    if (const CallExpr *CE = dyn_cast<CallExpr>(WarnExpr))
+      if (const Decl *FD = CE->getCalleeDecl())
+        if (FD->hasAttr<WarnUnusedResultAttr>())
+          DiagnoseNoDiscard(*this,
+                            cast_or_null<WarnUnusedResultAttr>(
+                                CE->getUnusedResultAttr(Context)),
+                            Loc, R1, R2, /*isCtor=*/false);
     return;
+  }
 
   // Check if this is the UNREFERENCED_PARAMETER from the Microsoft headers.
   // That macro is frequently used to suppress "unused parameter" warnings,
   // but its implementation makes clang's -Wunused-value fire.  Prevent this.
   if (isa<ParenExpr>(E->IgnoreImpCasts()) && Loc.isMacroID()) {
     SourceLocation SpellLoc = Loc;
     if (findMacroSpelling(SpellLoc, "UNREFERENCED_PARAMETER"))
       return;

clang/test/Frontend/macros.c

-// RUN: %clang_cc1 -DA= -DB=1 -verify -fsyntax-only %s
-// expected-no-diagnostics
+// RUN: %clang_cc1 -Wunused-result -DA= -DB=1 -verify -fsyntax-only %s
 
 int a[(B A) == 1 ? 1 : -1];
 
 
 // PR13747 - Don't warn about unused results with statement exprs in macros.
 void stuff(int,int,int);
 #define memset(x,y,z) ({ stuff(x,y,z); x; })
 
 void foo(int a, int b, int c) {
   memset(a,b,c);  // No warning!
 }
+
+__attribute__((warn_unused_result)) static inline int bar(void) {
+  return 42;
+}
+
+#define baz() ({ \
+  bar();         \
+})
+
+void quux(void) {
+  baz(); // expected-warning {{ignoring return value of function declared with 'warn_unused_result' attribute}}
+}