ISSUE - incorrect -Winfinite-recursion warning on potentially-unevaluated operand #21668

[appmonster007]

Summary

Fixing issue "incorrect -Winfinite-recursion warning on potentially-unevaluated operand #21668”

By having dedicated visit function (VisitCXXTypeidExpr) for typeid, instead of using default (VisitStmt).
In this new function we skip over CFG build for unevaluated operands of typeid

[shafik]

Thank for looking at this bug, just a small comment on the test.

clang/test/SemaCXX/warn-infinite-recursion.cpp

176

Looks like your missing // expected-warning{{call itself}}

[appmonster007]

fixed warning comments for test cases

[appmonster007]

included typeinfo file, in clang/test/SemaCXX directory

[appmonster007]

#include "typeinfo"

[appmonster007]

clang/test/SemaCXX/warn-infinite-recursion.cpp

3

#include <typeinfo> resulted in 'typeinfo' file not found with <angled> include; use "quotes" instead

[aaron.ballman]

Thank you for this fix, it's coming along well! Can you also add a release note to clang/docs/ReleaseNotes.rst mentioning the fix?

clang/lib/Analysis/CFG.cpp

4062

4064–4068

clang/test/SemaCXX/typeinfo

1–16 ↗

(On Diff #440859)

You should stick this into the place where it's used rather than a separate file, more comments below about why.

clang/test/SemaCXX/warn-infinite-recursion.cpp

3

Yup, that's actually by design. We don't want to include headers from the system because then we're testing against whatever happens to be installed on the machine running the test, which makes for poor test coverage. What we usually do is put the system header definitions directly into the file under test, or if it's code that's going to be shared by multiple tests, we'll make a file for it in the Inputs directory, and tell the RUN line to look there when doing an include. Then we always know exactly what we're testing against.

In this case, I'd just lower the type_info class into this file.

176

206

Please add a newline back to the end of the file.

[appmonster007]

inserted bullet point for -Winfinite-recursion diagnostics fix, where diagnostic will not warn for unevaluated operands of `typeid` expression, under "Improvements to Clang’s diagnostics" in clang/docs/ReleaseNotes.rst

fixed issue with typeinfo header, just included it directly in warn-infinite-recursion.cpp file

fixed recommended minor changes

[aaron.ballman]

Thank you for the fix, the changes LGTM! From our off-review discussion, I understand you need me to commit this on your behalf. What name and email address would you like me to use for patch attribution? I did have a suggestion on changing the release notes slightly, but I can do that for you when I land the changes, so don't feel obligated to push up a new patch unless you'd like to.

clang/docs/ReleaseNotes.rst

278–281

Reworded a bit.

[appmonster007]

Name : Prathit Aswar
github username : appmonster007
email : [e-mail]

[NoQ]

Thanks, very nice!

@usama54321 you recently fixed a similar problem in a clang-tidy check, do you know if it also requires a similar clause for polymorphic expressions inside typeid?

clang/docs/ReleaseNotes.rst

 - ``-Wshift-overflow`` will not warn for signed left shifts in C++20 mode
   (and newer), as it will always wrap and never overflow. This fixes
   `Issue 52873 <https://github.com/llvm/llvm-project/issues/52873>`_.
 - When using class templates without arguments, clang now tells developers
   that template arguments are missing in certain contexts.
   This fixes `Issue 55962 <https://github.com/llvm/llvm-project/issues/55962>`_.
 - Printable Unicode characters within `static_assert` messages are no longer
   escaped.
+- The ``-Winfinite-recursion`` diagnostic no longer warns about
+  unevaluated operands of a ``typeid`` expression, as they are now
+  modeled correctly in the CFG. This fixes
+  `Issue 21668 <https://github.com/llvm/llvm-project/issues/21668>`_.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 - Improve __builtin_dump_struct:
 
   - Support bitfields in struct and union.
   - Improve the dump format, dump both bitwidth(if its a bitfield) and field

clang/lib/Analysis/CFG.cpp

private:
   CFGBlock *VisitCXXDeleteExpr(CXXDeleteExpr *DE, AddStmtChoice asc);
   CFGBlock *VisitCXXForRangeStmt(CXXForRangeStmt *S);
   CFGBlock *VisitCXXFunctionalCastExpr(CXXFunctionalCastExpr *E,
                                        AddStmtChoice asc);
   CFGBlock *VisitCXXTemporaryObjectExpr(CXXTemporaryObjectExpr *C,
                                         AddStmtChoice asc);
   CFGBlock *VisitCXXThrowExpr(CXXThrowExpr *T);
   CFGBlock *VisitCXXTryStmt(CXXTryStmt *S);
+  CFGBlock *VisitCXXTypeidExpr(CXXTypeidExpr *S, AddStmtChoice asc);
   CFGBlock *VisitDeclStmt(DeclStmt *DS);
   CFGBlock *VisitDeclSubExpr(DeclStmt *DS);
   CFGBlock *VisitDefaultStmt(DefaultStmt *D);
   CFGBlock *VisitDoStmt(DoStmt *D);
   CFGBlock *VisitExprWithCleanups(ExprWithCleanups *E,
                                   AddStmtChoice asc, bool ExternallyDestructed);
   CFGBlock *VisitForStmt(ForStmt *F);
   CFGBlock *VisitGotoStmt(GotoStmt *G);
case Stmt::CXXTemporaryObjectExprClass:
       return VisitCXXTemporaryObjectExpr(cast<CXXTemporaryObjectExpr>(S), asc);
 
     case Stmt::CXXThrowExprClass:
       return VisitCXXThrowExpr(cast<CXXThrowExpr>(S));
 
     case Stmt::CXXTryStmtClass:
       return VisitCXXTryStmt(cast<CXXTryStmt>(S));
 
+    case Stmt::CXXTypeidExprClass:
+      return VisitCXXTypeidExpr(cast<CXXTypeidExpr>(S), asc);
+
     case Stmt::CXXForRangeStmtClass:
       return VisitCXXForRangeStmt(cast<CXXForRangeStmt>(S));
 
     case Stmt::DeclStmtClass:
       return VisitDeclStmt(cast<DeclStmt>(S));
 
     case Stmt::DefaultStmtClass:
       return VisitDefaultStmt(cast<DefaultStmt>(S));
else
     // otherwise the Exit block is the only successor.
     addSuccessor(Block, &cfg->getExit());
 
   // Add the statement to the block.  This may create new blocks if S contains
   // control-flow (short-circuit operations).
   return VisitStmt(T, AddStmtChoice::AlwaysAdd);
 }
 
+CFGBlock *CFGBuilder::VisitCXXTypeidExpr(CXXTypeidExpr *S, AddStmtChoice asc) {
+  if (asc.alwaysAdd(*this, S)) {
+    autoCreateBlock();
+    appendStmt(Block, S);
+  }
+
+  // C++ [expr.typeid]p3:
+  //   When typeid is applied to an expression other than an glvalue of a
+  //   polymorphic class type [...] [the] expression is an unevaluated
+  //   operand. [...]
+  // We add only potentially evaluated statements to the block to avoid
+  // CFG generation for unevaluated operands.
+  if (S && !S->isTypeDependent() && S->isPotentiallyEvaluated())
+    return VisitChildren(S);
+
+  // Return block without CFG for unevaluated operands.
+  return Block;
+}
+
 CFGBlock *CFGBuilder::VisitDoStmt(DoStmt *D) {
   CFGBlock *LoopSuccessor = nullptr;
 
   addLoopExit(D);
 
   // "do...while" is a control-flow statement.  Thus we stop processing the
   // current block.
   if (Block) {

clang/test/SemaCXX/warn-infinite-recursion.cpp

 // RUN: %clang_cc1 %s -fsyntax-only -verify -Winfinite-recursion
 
 void a() {  // expected-warning{{call itself}}
   a();
 }
 
 void b(int x) {  // expected-warning{{call itself}}
   if (x)
     b(x);
   else
     b(x+1);

 int test_wrapper() {
   if (x != 0)
     return Wrapper<x>::run() +
            Wrapper<x>::run2();  // expected-note{{instantiation}}
   return 0;
 }
 
 int wrapper_sum = test_wrapper<2>();  // expected-note{{instantiation}}
+
+namespace std {
+class type_info {
+public:
+  virtual ~type_info();
+  const char *name() const { return __name; }
+  bool operator==(const type_info &__arg) const {
+    return __name == __arg.__name;
+  }
+
+  bool operator!=(const type_info &__arg) const {
+    return !operator==(__arg);
+  }
+
+protected:
+  const char *__name;
+};
+} // namespace std
+struct Q {
+  virtual ~Q() = default;
+};
+
+Q q;
+Q &evaluated_recursive_function(int x) {         // expected-warning{{call itself}}
+  (void)typeid(evaluated_recursive_function(x)); // expected-warning {{expression with side effects will be evaluated despite being used as an operand to 'typeid'}}
+  return q;
+}
+
+int unevaluated_recursive_function() {
+  (void)typeid(unevaluated_recursive_function());
+  return 0;
+}