[clang-tidy] Fixing segfault when there's no IdentifierInfo

[juliehockett]

Summary

Bug 36150 found a segfault on mac when a CXXRecordDecl has no IdentifierInfo, this fixes it.

[JonasToth]

clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp

44

Please move then return false onto the next line.

[JonasToth]

Is there a way to add a test, that would trigger the old segfault and show that it does not happen anymore with this fix?

[hokein]

In D49158#1158327, @JonasToth wrote:

Is there a way to add a test, that would trigger the old segfault and show that it does not happen anymore with this fix?

+1, we should have a minimal test case for this fix, https://bugs.llvm.org/show_bug.cgi?id=36150 provides a case, but we need to reduce it (getting rid of the STD header).

[juliehockett]

In D49158#1159882, @hokein wrote:

In D49158#1158327, @JonasToth wrote:

Is there a way to add a test, that would trigger the old segfault and show that it does not happen anymore with this fix?

+1, we should have a minimal test case for this fix, https://bugs.llvm.org/show_bug.cgi?id=36150 provides a case, but we need to reduce it (getting rid of the STD header).

Does anyone know of a case where the base would not be a CXXRecordDecl that doesn't involve std::functional? (for background, I could only reproduce the error in the test case in https://bugs.llvm.org/show_bug.cgi?id=36150 on a Mac)

[aaron.ballman]

In D49158#1172178, @juliehockett wrote:

In D49158#1159882, @hokein wrote:

In D49158#1158327, @JonasToth wrote:

Is there a way to add a test, that would trigger the old segfault and show that it does not happen anymore with this fix?

+1, we should have a minimal test case for this fix, https://bugs.llvm.org/show_bug.cgi?id=36150 provides a case, but we need to reduce it (getting rid of the STD header).

Does anyone know of a case where the base would not be a CXXRecordDecl that doesn't involve std::functional? (for background, I could only reproduce the error in the test case in https://bugs.llvm.org/show_bug.cgi?id=36150 on a Mac)

Perhaps something like:

template <typename Ty>
struct S : Ty {
  S(Ty Fn) : Ty(Fn) {}
};

void f() {
  S s{[](){}};
}

Not certain if this is what you're looking for, but it is a trick used in <functional> implementations sometimes.

clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp

AST_MATCHER(CXXRecordDecl, hasBases) {
   return false;
 }
 } // namespace
 
 // Adds a node (by name) to the interface map, if it was not present in the map
 // previously.
 void MultipleInheritanceCheck::addNodeToInterfaceMap(const CXXRecordDecl *Node,
                                                      bool isInterface) {
-  StringRef Name = Node->getIdentifier()->getName();
+  if (const auto *Id = Node->getIdentifier()) {
+    StringRef Name = Id->getName();
     InterfaceMap.insert(std::make_pair(Name, isInterface));
   }
+}
 
 // Returns "true" if the boolean "isInterface" has been set to the
 // interface status of the current Node. Return "false" if the
 // interface status for the current node is not yet known.
 bool MultipleInheritanceCheck::getInterfaceStatus(const CXXRecordDecl *Node,
                                                   bool &isInterface) const {
-  StringRef Name = Node->getIdentifier()->getName();
+  if (!Node) return false;
+  if (const auto *Id = Node->getIdentifier()) {
+    StringRef Name = Id->getName();
     llvm::StringMapConstIterator<bool> Pair = InterfaceMap.find(Name);
     if (Pair == InterfaceMap.end())
       return false;
     isInterface = Pair->second;
     return true;
   }
+  return false;
+}
 
 bool MultipleInheritanceCheck::isCurrentClassInterface(
     const CXXRecordDecl *Node) const {
   // Interfaces should have no fields.
   if (!Node->field_empty()) return false;
 
   // Interfaces should have exclusively pure methods.
   return llvm::none_of(Node->methods(), [](const CXXMethodDecl *M) {
if (const auto *D = Result.Nodes.getNodeAs<CXXRecordDecl>("decl")) {
     unsigned NumConcrete = 0;
     for (const auto &I : D->bases()) {
       if (I.isVirtual()) continue;
       const auto *Ty = I.getType()->getAs<RecordType>();
       if (!Ty) continue;
       const auto *Base = cast<CXXRecordDecl>(Ty->getDecl()->getDefinition());
       if (!isInterface(Base)) NumConcrete++;
     }
-    
     // Check virtual bases to see if there is more than one concrete 
     // non-virtual base.
     for (const auto &V : D->vbases()) {
       const auto *Ty = V.getType()->getAs<RecordType>();
       if (!Ty) continue;
       const auto *Base = cast<CXXRecordDecl>(Ty->getDecl()->getDefinition());
       if (!isInterface(Base)) NumConcrete++;
     }