Page MenuHomePhabricator

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

Authored by nickdesaulniers on Apr 6 2020, 5:14 PM.

Details

Reviewers
dblaikie
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

Diff Detail

Event Timeline

nickdesaulniers created this revision.Apr 6 2020, 5:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2020, 5:14 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nickdesaulniers planned changes to this revision.Apr 6 2020, 5:17 PM

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.

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; });
      |    ^

(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)

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.

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?

  • emit warning just for CallExprs
nickdesaulniers added inline comments.Apr 7 2020, 2:22 PM
clang/lib/Sema/SemaStmt.cpp
252

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

  • tighten up condition, eagerly emit diag, test with kernel before pushing
nickdesaulniers marked an inline comment as done.Apr 7 2020, 2:52 PM
nickdesaulniers retitled this revision from [Sema] fix -Wunused-result in StmtExpr to [Sema] Check calls to __attribute__((warn_unused_result)) from StmtExprs.
nickdesaulniers edited the summary of this revision. (Show Details)

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 added a comment.EditedApr 7 2020, 4:06 PM

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

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?