[llvm][clang] Add checks for the smart pointers with the possibility to be null

[OikawaKirie]

Summary

All these potential null pointer dereferences are reported by my static analyzer for null smart pointer dereferences, which has a different implementation from alpha.cplusplus.SmartPtr.

The checked pointers are:

• The return value of createArgument in file clang/utils/TableGen/ClangAttrEmitter.cpp. Although there are a lot of checks in the function, nullptr is still allowed to be returned. As a recursive function it is, I added checks to all the places where the function is called.
• The local variable Unit in function DWARFLinker::loadClangModule in file llvm/lib/DWARFLinker/DWARFLinker.cpp. If the variable is not set in the loop below its definition, it will trigger a null pointer dereference after the loop.
• The local variable Index in function ThinLTOCodeGenerator::run in file llvm/lib/LTO/ThinLTOCodeGenerator.cpp. When function ThinLTOCodeGenerator::linkCombinedIndex returns nullptr, the pointer Index will be null and be dereferenced below.
• The parameter variable Buffer in function InMemoryFileSystem::addFile in file llvm/lib/Support/VirtualFileSystem.cpp. The assertion in this function (assert(!(HardLinkTarget && Buffer))) only checks whether these two parameters can both be non-null. But It can be inferred that both pointers can be null together. A null Buffer pointer can be dereferenced without a check.
• The return value of function ModuleLazyLoaderCache::operator in file llvm/tools/llvm-link/llvm-link.cpp. According to the bug report of my static analyzer, the std::function variable ModuleLazyLoaderCache::createLazyModule points to function loadFile, which may return nullptr when error. And the pointer is returned as a reference without a check to the return value.
• The local variable Ret in function MarshallingKindInfo::create in file llvm/utils/TableGen/OptParserEmitter.cpp. If not all MarshallingKind's are handled, variable Ret will be kept as nullptr.

[dexonsmith]

Is it possible to split these up into separate patches for unrelated code?

clang/utils/TableGen/ClangAttrEmitter.cpp

1346–1353

Can we just add a single assertion here? It looks to me like every caller wants a valid return.

[OikawaKirie]

In D91844#2408897, @dexonsmith wrote:

Is it possible to split these up into separate patches for unrelated code?

Since these are reported by one static scan, and these reported places cannot be categorized with others, I choose to submit them in one patch for simplicity and avoiding spam. If it is necessary to separate them one by one, I will close this review and start a new one for each of them.

Or, maybe you are thinking of just separating the patch of clang with llvm? If so, I will start a new review just for the patch of clang and leave the patches of llvm here.

clang/utils/TableGen/ClangAttrEmitter.cpp

1346–1353

Ok, I will add an assertion here (below line 1353) in the new submits, and remove all other assertions I added in this file together with the checks on this pointer after the assertion (line 1355 and 1358).

[avl]

The change for DWARFLinker is fine. I also think it is better to split the patch up into separate patches.

[OikawaKirie]

Split to

• D97251 (clang/utils/TableGen/ClangAttrEmitter.cpp)
• D97185 (llvm/lib/DWARFLinker/DWARFLinker.cpp)
• D97254 (llvm/lib/LTO/ThinLTOCodeGenerator.cpp)
• D97255 (llvm/lib/Support/VirtualFileSystem.cpp)
• D97258 (llvm/tools/llvm-link/llvm-link.cpp)

And the last one (llvm/utils/TableGen/OptParserEmitter.cpp) has been fixed by others.

Please review the new revisions, and this revision will be closed.

Thanks to all reviewers and sorry for the delay.

clang/utils/TableGen/ClangAttrEmitter.cpp

else if (ArgName == "ParamIdxArgument")
     Ptr = std::make_unique<SimpleArgument>(Arg, Attr, "ParamIdx");
   else if (ArgName == "VariadicIdentifierArgument")
     Ptr = std::make_unique<VariadicIdentifierArgument>(Arg, Attr);
   else if (ArgName == "VersionArgument")
     Ptr = std::make_unique<VersionArgument>(Arg, Attr);
   else if (ArgName == "OMPTraitInfoArgument")
     Ptr = std::make_unique<SimpleArgument>(Arg, Attr, "OMPTraitInfo *");
 
   if (!Ptr) {
     // Search in reverse order so that the most-derived type is handled first.
     ArrayRef<std::pair<Record*, SMRange>> Bases = Search->getSuperClasses();
     for (const auto &Base : llvm::reverse(Bases)) {
       if ((Ptr = createArgument(Arg, Attr, Base.first)))
         break;
     }
   }
 
   if (Ptr && Arg.getValueAsBit("Optional"))
     Ptr->setOptional(true);
 
   if (Ptr && Arg.getValueAsBit("Fake"))
     Ptr->setFake(true);
 
   return Ptr;
for (const auto *Attr : Attrs) {
     std::vector<Record*> ArgRecords = R.getValueAsListOfDefs("Args");
     std::vector<std::unique_ptr<Argument>> Args;
     Args.reserve(ArgRecords.size());
 
     bool HasOptArg = false;
     bool HasFakeArg = false;
     for (const auto *ArgRecord : ArgRecords) {
       Args.emplace_back(createArgument(*ArgRecord, R.getName()));
+      assert(Args.back());
       if (Header) {
         Args.back()->writeDeclarations(OS);
         OS << "\n\n";
       }
 
       // For these purposes, fake takes priority over optional.
       if (Args.back()->isFake()) {
         HasFakeArg = true;
for (const auto *Attr : Attrs) {
     if (R.isSubClassOf(InhClass))
       OS << "    bool isInherited = Record.readInt();\n";
     OS << "    bool isImplicit = Record.readInt();\n";
     OS << "    bool isPackExpansion = Record.readInt();\n";
     ArgRecords = R.getValueAsListOfDefs("Args");
     Args.clear();
     for (const auto *Arg : ArgRecords) {
       Args.emplace_back(createArgument(*Arg, R.getName()));
+      assert(Args.back());
       Args.back()->writePCHReadDecls(OS);
     }
     OS << "    New = new (Context) " << R.getName() << "Attr(Context, Info";
     for (auto const &ri : Args) {
       OS << ", ";
       ri->writePCHReadArgs(OS);
     }
     OS << ");\n";
for (const auto *Attr : Attrs) {
     if (R.isSubClassOf(InhClass) || !Args.empty())
       OS << "    const auto *SA = cast<" << R.getName()
          << "Attr>(A);\n";
     if (R.isSubClassOf(InhClass))
       OS << "    Record.push_back(SA->isInherited());\n";
     OS << "    Record.push_back(A->isImplicit());\n";
     OS << "    Record.push_back(A->isPackExpansion());\n";
 
-    for (const auto *Arg : Args)
-      createArgument(*Arg, R.getName())->writePCHWrite(OS);
+    for (const auto *Arg : Args) {
+      auto A = createArgument(*Arg, R.getName());
+      assert(A);
+      A->writePCHWrite(OS);
+    }
     OS << "    break;\n";
     OS << "  }\n";
   }
   OS << "  }\n";
 }
 
 // Helper function for GenerateTargetSpecificAttrChecks that alters the 'Test'
 // parameter with only a single check type, if applicable.
OS << "template <typename Derived>\n"
        << "bool VISITORCLASS<Derived>::Traverse"
        << R.getName() << "Attr(" << R.getName() << "Attr *A) {\n"
        << "  if (!getDerived().VisitAttr(A))\n"
        << "    return false;\n"
        << "  if (!getDerived().Visit" << R.getName() << "Attr(A))\n"
        << "    return false;\n";
 
     std::vector<Record*> ArgRecords = R.getValueAsListOfDefs("Args");
-    for (const auto *Arg : ArgRecords)
-      createArgument(*Arg, R.getName())->writeASTVisitorTraversal(OS);
+    for (const auto *Arg : ArgRecords) {
+      auto A = createArgument(*Arg, R.getName());
+      assert(A);
+      A->writeASTVisitorTraversal(OS);
+    }
 
     OS << "  return true;\n";
     OS << "}\n\n";
   }
 
   // Write generic Traverse routine
   OS << "template <typename Derived>\n"
      << "bool VISITORCLASS<Derived>::TraverseAttr(Attr *A) {\n"
if (!TDependent) {
       OS << "    }\n";
       continue;
     }
 
     std::vector<Record*> ArgRecords = R.getValueAsListOfDefs("Args");
     std::vector<std::unique_ptr<Argument>> Args;
     Args.reserve(ArgRecords.size());
 
-    for (const auto *ArgRecord : ArgRecords)
+    for (const auto *ArgRecord : ArgRecords) {
       Args.emplace_back(createArgument(*ArgRecord, R.getName()));
+      assert(Args.back());
+    }
 
     for (auto const &ai : Args)
       ai->writeTemplateInstantiation(OS);
 
     OS << "        return new (C) " << R.getName() << "Attr(C, *A";
     for (auto const &ai : Args) {
       OS << ", ";
       ai->writeTemplateInstantiationArgs(OS);
void EmitClangAttrParsedAttrList(RecordKeeper &Records, raw_ostream &OS) {
 
   ParsedAttrMap Names = getParsedAttrList(Records);
   for (const auto &I : Names) {
     OS << "PARSED_ATTR(" << I.first << ")\n";
   }
 }
 
 static bool isArgVariadic(const Record &R, StringRef AttrName) {
-  return createArgument(R, AttrName)->isVariadic();
+  auto A = createArgument(R, AttrName);
+  assert(A);
+  return A->isVariadic();
 }
 
 static void emitArgInfo(const Record &R, raw_ostream &OS) {
   // This function will count the number of arguments specified for the
   // attribute and emit the number of required arguments followed by the
   // number of optional arguments.
   std::vector<Record *> Args = R.getValueAsListOfDefs("Args");
   unsigned ArgCount = 0, OptCount = 0;
for (const auto *Attr : Attrs) {
     std::string FunctionContent;
     llvm::raw_string_ostream SS(FunctionContent);
 
     std::vector<FlattenedSpelling> Spellings = GetFlattenedSpellings(R);
     if (Spellings.size() > 1 && !SpellingNamesAreCommon(Spellings))
       SS << "    OS << \" \" << A->getSpelling();\n";
 
     Args = R.getValueAsListOfDefs("Args");
-    for (const auto *Arg : Args)
-      createArgument(*Arg, R.getName())->writeDump(SS);
+    for (const auto *Arg : Args) {
+      auto A = createArgument(*Arg, R.getName());
+      assert(A);
+      A->writeDump(SS);
+    }
 
     if (SS.tell()) {
       OS << "  void Visit" << R.getName() << "Attr(const " << R.getName()
          << "Attr *A) {\n";
       if (!Args.empty())
         OS << "    const auto *SA = cast<" << R.getName()
            << "Attr>(A); (void)SA;\n";
       OS << SS.str();
for (const auto *Attr : Attrs) {
     const Record &R = *Attr;
     if (!R.getValueAsBit("ASTNode"))
       continue;
 
     std::string FunctionContent;
     llvm::raw_string_ostream SS(FunctionContent);
 
     Args = R.getValueAsListOfDefs("Args");
-    for (const auto *Arg : Args)
-      createArgument(*Arg, R.getName())->writeDumpChildren(SS);
+    for (const auto *Arg : Args) {
+      auto A = createArgument(*Arg, R.getName());
+      assert(A);
+      A->writeDumpChildren(SS);
+    }
     if (SS.tell()) {
       OS << "  void Visit" << R.getName() << "Attr(const " << R.getName()
          << "Attr *A) {\n";
       if (!Args.empty())
         OS << "    const auto *SA = cast<" << R.getName()
            << "Attr>(A); (void)SA;\n";
       OS << SS.str();
       OS << "  }\n";

llvm/lib/DWARFLinker/DWARFLinker.cpp

if (!registerModuleReference(
                          Options.ParseableSwiftInterfaces,
                          [&](const Twine &Warning, const DWARFDie &DIE) {
                            reportWarning(Warning, File, &DIE);
                          });
       // Keep everything.
       Unit->markEverythingAsKept();
     }
   }
+  assert(Unit && "CompileUnit is not set!");
   if (!Unit->getOrigUnit().getUnitDIE().hasChildren())
     return Error::success();
   if (!Quiet && Options.Verbose) {
     outs().indent(Indent);
     outs() << "cloning .debug_info from " << Filename << "\n";
   }
 
   UnitListTy CompileUnits;

llvm/lib/LTO/ThinLTOCodeGenerator.cpp

for (auto &Mod : Modules) {
       }, count++);
     }
 
     return;
   }
 
   // Sequential linking phase
   auto Index = linkCombinedIndex();
+  assert(Index);
 
   // Save temps: index.
   if (!SaveTempsDir.empty()) {
     auto SaveTempPath = SaveTempsDir + "index.bc";
     std::error_code EC;
     raw_fd_ostream OS(SaveTempPath, EC, sys::fs::OF_None);
     if (EC)
       report_fatal_error(Twine("Failed to open ") + SaveTempPath +

llvm/lib/Support/VirtualFileSystem.cpp

while (true) {
     ++I;
     if (!Node) {
       if (I == E) {
         // End of the path.
         std::unique_ptr<detail::InMemoryNode> Child;
         if (HardLinkTarget)
           Child.reset(new detail::InMemoryHardLink(P.str(), *HardLinkTarget));
         else {
+          assert(Buffer && "Non-HardLink without a buffer?");
           // Create a new file or directory.
           Status Stat(P.str(), getNextVirtualUniqueID(),
                       llvm::sys::toTimePoint(ModificationTime), ResolvedUser,
                       ResolvedGroup, Buffer->getBufferSize(), ResolvedType,
                       ResolvedPerms);
           if (ResolvedType == sys::fs::file_type::directory_file) {
             Child.reset(new detail::InMemoryDirectory(std::move(Stat)));
           } else {
if (auto *NewDir = dyn_cast<detail::InMemoryDirectory>(Node)) {
               isa<detail::InMemoryHardLink>(Node)) &&
              "Must be either file, hardlink or directory!");
 
       // Trying to insert a directory in place of a file.
       if (I != E)
         return false;
 
       // Return false only if the new file is different from the existing one.
+      assert(Buffer);
       if (auto Link = dyn_cast<detail::InMemoryHardLink>(Node)) {
         return Link->getResolvedFile().getBuffer()->getBuffer() ==
                Buffer->getBuffer();
       }
       return cast<detail::InMemoryFile>(Node)->getBuffer()->getBuffer() ==
              Buffer->getBuffer();
     }
   }

llvm/tools/llvm-link/llvm-link.cpp

std::unique_ptr<Module> takeModule(const std::string &FileName) {
     return Ret;
   }
 };
 
 // Get a Module for \p FileName from the cache, or load it lazily.
 Module &ModuleLazyLoaderCache::operator()(const char *argv0,
                                           const std::string &Identifier) {
   auto &Module = ModuleMap[Identifier];
-  if (!Module)
+  if (!Module) {
     Module = createLazyModule(argv0, Identifier);
+    assert(Module && "Failed to create lazy module!");
+  }
   return *Module;
 }
 } // anonymous namespace
 
 namespace {
 struct LLVMLinkDiagnosticHandler : public DiagnosticHandler {
   bool handleDiagnostics(const DiagnosticInfo &DI) override {
     unsigned Severity = DI.getSeverity();

llvm/utils/TableGen/OptParserEmitter.cpp

static MarshallingInfo::Ptr createMarshallingInfo(const Record &R) {
   MarshallingInfo::Ptr Ret;
   StringRef KindString = R.getValueAsString("MarshallingInfoKind");
   if (KindString == "Default") {
     Ret = std::make_unique<MarshallingInfo>(R);
   } else if (KindString == "BooleanFlag") {
     Ret = std::make_unique<MarshallingInfoBooleanFlag>(
         R, *R.getValueAsDef("NegOption"));
   }
+  assert(Ret && "Unknown MarshallingInfoKind.");
 
   Ret->ShouldAlwaysEmit = R.getValueAsBit("ShouldAlwaysEmit");
   Ret->KeyPath = R.getValueAsString("KeyPath");
   Ret->DefaultValue = R.getValueAsString("DefaultValue");
   Ret->NormalizedValuesScope = R.getValueAsString("NormalizedValuesScope");
 
   Ret->Normalizer = R.getValueAsString("Normalizer");
   Ret->Denormalizer = R.getValueAsString("Denormalizer");