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

178

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

2–17

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.

178

191

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 ↗

(On Diff #441014)

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/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 block to avoid
+  // CFG generation for unevaluated operands.
+  if (S && !S->isTypeDependent()) {
+    if (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/typeinfo

+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;
+  };
+}

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

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

 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}}
+
+struct Q {
+  virtual ~Q(){};
+};
+
+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;
+}
 No newline at end of file