[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added inline comments.


================
Comment at: include/clang/AST/ASTContext.h:42
 #include "clang/Basic/Specifiers.h"
+#include "clang/Basic/VersionTuple.h"
 #include "llvm/ADT/APSInt.h"
----------------
I don't see why this is included here.


================
Comment at: include/clang/AST/Mangle.h:55
   const ManglerKind Kind;
+  // Used for cross tranlsation unit analysis.
+  // To reduce the risk of function name collision in C projects, we force
----------------
tranlsation => translation


================
Comment at: lib/AST/ASTContext.cpp:1457
+    return nullptr;
+  for (Decl *D : DC->decls()) {
+    const auto *SubDC = dyn_cast<DeclContext>(D);
----------------
could add a "const" perhaps


================
Comment at: lib/AST/ASTContext.cpp:1491
+  std::string MangledFnName = getMangledName(FD, MangleCtx.get());
+  std::string ExternalFunctionMap = (XTUDir + "/externalFnMap.txt").str();
+  ASTUnit *Unit = nullptr;
----------------
as far as I see ExternalFunctionMap can be moved into the subscope.


================
Comment at: lib/AST/ASTContext.cpp:1500
+      std::string FunctionName, FileName;
+      while (ExternalFnMapFile >> FunctionName >> FileName)
+        FunctionFileMap[FunctionName] = (XTUDir + "/" + FileName).str();
----------------
I would recommend that a parser is added that make sure the file data is correct/sane. If there is some load/save mismatch now or in the future or if the user choose to tweak the file for some reason, the behaviour could be wrong.



================
Comment at: lib/AST/ASTContext.cpp:1502
+        FunctionFileMap[FunctionName] = (XTUDir + "/" + FileName).str();
+      ExternalFnMapFile.close();
+    }
----------------
well.. I believe it's redundant to close this explicitly since the std::ifstream destructor should do that. But it doesn't hurt doing it.


================
Comment at: lib/Basic/SourceManager.cpp:2028
 /// \brief Determines the order of 2 source locations in the translation unit.
+/// FIXME: It also works when two locations are from different translation unit.
+///        In that case it will return *some* order.
----------------
I am not good at english. But I believe unit=>units.


================
Comment at: lib/Basic/SourceManager.cpp:2126
   }
-  llvm_unreachable("Unsortable locations found");
+  // FIXME: Source locations from different translation unit.
+  return LOffs.first < ROffs.first;
----------------
I am not good at english. But I believe unit=>units.


================
Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:48
+#include <sys/file.h>
+#include <unistd.h>
+#include <fstream>
----------------
I believe sys/file.h and unistd.h are posix includes.


================
Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:433
+    return;
+  int fd = open(fileName.c_str(), O_CREAT|O_WRONLY|O_APPEND, 0666);
+  flock(fd, LOCK_EX);
----------------
this is posix file-I/O


================
Comment at: tools/clang-cmdline-arch-extractor/ClangCmdlineArchExtractor.cpp:56
+  if (Sources.empty())
+    return 1;
+
----------------
Maybe use the EXIT_FAILURE instead


================
Comment at: tools/clang-cmdline-arch-extractor/ClangCmdlineArchExtractor.cpp:66
+  for (StringRef SourceFile : Sources) {
+    char *Path = realpath(SourceFile.data(), nullptr);
+    if (Path)
----------------
This is posix function as far as I know.


================
Comment at: tools/clang-cmdline-arch-extractor/ClangCmdlineArchExtractor.cpp:72
+
+  return 0;
+}
----------------
EXIT_SUCCESS is also possible however I guess that is 0 on all implementations.


================
Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:32
+#include <string>
+#include <sys/file.h>
+#include <unistd.h>
----------------
posix includes


================
Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:153
+
+      if (!FileName.empty())
+        switch (FD->getLinkageInternal()) {
----------------
I do not see how FileName could be empty. It always starts with "/ast/" right?


================
Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:193
+  lockedWrite(BuildDir + "/definedFns.txt", DefinedFuncsStr.str());
+  std::stringstream CFGStr;
+  for (auto &Entry : CG) {
----------------
if a STL stream will be used I recommend the std::ostringstream instead


================
Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:244
+    errs() << "Exactly one XTU dir should be provided\n";
+    return 1;
+  }
----------------
maybe use EXIT_FAILURE


================
Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:257
+  Tool.run(newFrontendActionFactory<MapFunctionNamesAction>().get());
+}
----------------
no return.


================
Comment at: tools/xtu-analysis/xtu-analyze.py:29
+
+threading_factor = int(multiprocessing.cpu_count() * 1.5)
+analyser_output_formats = ['plist-multi-file', 'plist', 'plist-html',
----------------
does this mean that if there are 4 cores this script will by default use 6 threads? isn't that too aggressive?


Repository:
  rL LLVM

https://reviews.llvm.org/D30691