[CTU] Add triple/lang mismatch handling

[martong]

Summary

We introduce a strict policy for C++ CTU. It can work across TUs only if
the C++ dialects are the same. We neither allow C vs C++ CTU. We do this
because the same constructs might be represented with different properties in
the corresponding AST nodes or even the nodes might be completely different (a
struct will be RecordDecl in C, but it will be a CXXRectordDecl in C++, thus it
may cause certain assertions during cast operations).

[martong]

• Add lit tests

[Szelethus]

Hah. Do we support CTU for other languages, like ObjC and ObjC++? Can this be an issue there?

lib/CrossTU/CrossTranslationUnit.cpp

59

// end of namespace llvm

210

TODO:

214

Use punctuation.

[a_sidorin]

Hi Gabor,
Please find my comments inline.

Hah. Do we support CTU for other languages, like ObjC and ObjC++? Can this be an issue there?

That's a good question. In Samsung, CTU hasn't been tested on ObjC code. Upstream CTU supports only FunctionDecls as well so its ObjC status is questionable..

lib/CrossTU/CrossTranslationUnit.cpp

31

Why don't we place it into the anon namespace just below?

35

This has to be split, probably with early returns. Example:

if (Lhs.getArch() != Triple::UnknownArch && Rhs.getArch() != Triple::UnknownArch &&
              Lhs.getArch() != Rhs.getArch()
  return false;
...

47

We can use Triple::isOSUnknown() instead.

59

// end namespace llvm is much more common.

203

Reference char should stick to the variable, not to the type.

211

Comments should end with a dot. Same below.

test/Analysis/ctu-unknown-parts-in-triples.cpp

9

Two spaces after ExprInspection.

[martong]

• Address review comments

[martong]

Thanks for the review! I have updated the patch according to your comments.

lib/CrossTU/CrossTranslationUnit.cpp

31

Ok, I moved it into the unnamed namespace below.

35

Ok, I split that up with early returns.

59

Moved to the unnamed namespace.

[martong]

Hah. Do we support CTU for other languages, like ObjC and ObjC++? Can this be an issue there?

I really don't know. We never tried it, our focus is on C and C++ for now. Unfortunately, there is nobody with ObjC/C++ knowledge and (more importantly) with interest to try CTU and report any errors.

[a_sidorin]

LGTM, thanks!

[martong]

• Break long RUN lines

[xazax.hun]

include/clang/Basic/DiagnosticCrossTUKinds.td

19

I am not sure if we want this to be an error. The analysis could continue without any problems if we do not actually merge the two ASTs. So maybe having a warning only is better. My concern is that once we have this error, there would be no way to analyze mixed language (C/C++) projects cleanly using CTU.

lib/CrossTU/CrossTranslationUnit.cpp

212

I think we should not emit an error here. It should be up to the caller (the user of the library) to decide if she wants to handle this as an error, warnings, or just suppress these kinds of problems. I would rather extend emitCrossTUDiagnostics as a shorthand for the user if emitting an error is the desired behavior.

[martong]

• Diagnose a warning (which may be upgradable to an error)

[martong]

include/clang/Basic/DiagnosticCrossTUKinds.td

19

Okay, I agree, I changed it to be a warning by default.

lib/CrossTU/CrossTranslationUnit.cpp

212

I would prefer to exploit the capabilities of the DiagEngine, instead of extending the interface of CrossTranslationUnitContext.
By using a DiagGroup we can upgrade the warning to be an error, so I just changed it to be like that.

[martong]

• Forgot to rename err_ctu_incompat_triple -> warn_ctu_incompat_triple

[xazax.hun]

lib/CrossTU/CrossTranslationUnit.cpp

212

You do not need the extend the interface. There is already a function for that below. Also, the user might not want to display a warning either but do something else, like generate a log message.

[Szelethus]

We should probably prefer %clang_analyze_cc1 to %clang_cc1 -analyze here too.

[martong]

lib/CrossTU/CrossTranslationUnit.cpp

212

Ah, okay, I missed that. Now I added a case to that function. Still, I think having a DiagGroup is useful anyway.

[martong]

• Add a case to emitCrossTUDiagnostics

[martong]

• Use clang_analyze_cc1

[martong]

We should probably prefer %clang_analyze_cc1 to %clang_cc1 -analyze here too.

Ok, changed that.

[xazax.hun]

LG!

[ychen]

FWIW, ASTUnit seems to save an effective triple while the current AST uses the nominal triple. (for example, armv7a-xx-xx-eabihf vs armv7a-xx-xx-eabi). I'm not sure if there is a good solution other than using heuristics to allow some differences.

[martong]

In D55134#3224339, @ychen wrote:

FWIW, ASTUnit seems to save an effective triple while the current AST uses the nominal triple. (for example, armv7a-xx-xx-eabihf vs armv7a-xx-xx-eabi). I'm not sure if there is a good solution other than using heuristics to allow some differences.

@ychen, thanks for the notice! I hope we'll have time to address the issue soon.

include/clang/Basic/DiagnosticCrossTUKinds.td

 let Component = "CrossTU" in {
 
 def err_fnmap_parsing : Error<
   "error parsing index file: '%0' line: %1 'UniqueID filename' format "
   "expected">;
 
 def err_multiple_def_index : Error<
   "multiple definitions are found for the same key in index ">;
+
+def err_ctu_incompat_triple : Error<
+  "imported AST from '%0' had been generated for a different target, current: %1, imported: %2">;
 }

include/clang/CrossTU/CrossTranslationUnit.h

 enum class index_error_code {
   unspecified = 1,
   missing_index_file,
   invalid_index_format,
   multiple_definitions,
   missing_definition,
   failed_import,
   failed_to_get_external_ast,
-  failed_to_generate_usr
+  failed_to_generate_usr,
+  triple_mismatch,
+  lang_mismatch
 };
 
 class IndexError : public llvm::ErrorInfo<IndexError> {
 public:
   static char ID;
   IndexError(index_error_code C) : Code(C), LineNo(0) {}
   IndexError(index_error_code C, std::string FileName, int LineNo = 0)
       : Code(C), FileName(std::move(FileName)), LineNo(LineNo) {}

lib/CrossTU/CrossTranslationUnit.cpp

 #include "llvm/ADT/Triple.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include <fstream>
 #include <sstream>
 
 namespace clang {
 namespace cross_tu {
 
 namespace {
+
+// Same as Triple's equality operator, but we check a field only if that is
+// known in both instances.
+bool hasEqualKnownFields(const llvm::Triple &Lhs, const llvm::Triple &Rhs) {
+  using llvm::Triple;
+  if (Lhs.getArch() != Triple::UnknownArch &&
+      Rhs.getArch() != Triple::UnknownArch && Lhs.getArch() != Rhs.getArch())
+    return false;
+  if (Lhs.getSubArch() != Triple::NoSubArch &&
+      Rhs.getSubArch() != Triple::NoSubArch &&
+      Lhs.getSubArch() != Rhs.getSubArch())
+    return false;
+  if (Lhs.getVendor() != Triple::UnknownVendor &&
+      Rhs.getVendor() != Triple::UnknownVendor &&
+      Lhs.getVendor() != Rhs.getVendor())
+    return false;
+  if (!Lhs.isOSUnknown() && !Rhs.isOSUnknown() &&
+      Lhs.getOS() != Rhs.getOS())
+    return false;
+  if (Lhs.getEnvironment() != Triple::UnknownEnvironment &&
+      Rhs.getEnvironment() != Triple::UnknownEnvironment &&
+      Lhs.getEnvironment() != Rhs.getEnvironment())
+    return false;
+  if (Lhs.getObjectFormat() != Triple::UnknownObjectFormat &&
+      Rhs.getObjectFormat() != Triple::UnknownObjectFormat &&
+      Lhs.getObjectFormat() != Rhs.getObjectFormat())
+    return false;
+  return true;
+}
+
 // FIXME: This class is will be removed after the transition to llvm::Error.
 class IndexErrorCategory : public std::error_category {
 public:
   const char *name() const noexcept override { return "clang.index"; }
 
   std::string message(int Condition) const override {
     switch (static_cast<index_error_code>(Condition)) {
     case index_error_code::unspecified:
       return "An unknown error has occurred.";
     case index_error_code::missing_index_file:
       return "The index file is missing.";
     case index_error_code::invalid_index_format:
       return "Invalid index file format.";
     case index_error_code::multiple_definitions:
       return "Multiple definitions in the index file.";
     case index_error_code::missing_definition:
       return "Missing definition from the index file.";
     case index_error_code::failed_import:
       return "Failed to import the definition.";
     case index_error_code::failed_to_get_external_ast:
       return "Failed to load external AST source.";
     case index_error_code::failed_to_generate_usr:
       return "Failed to generate USR.";
+    case index_error_code::triple_mismatch:
+      return "Triple mismatch";
+    case index_error_code::lang_mismatch:
+      return "Language mismatch";
     }
     llvm_unreachable("Unrecognized index_error_code.");
   }
 };
 
 static llvm::ManagedStatic<IndexErrorCategory> Category;
 } // end anonymous namespace
 
if (!ASTUnitOrError)
     return ASTUnitOrError.takeError();
   ASTUnit *Unit = *ASTUnitOrError;
   if (!Unit)
     return llvm::make_error<IndexError>(
         index_error_code::failed_to_get_external_ast);
   assert(&Unit->getFileManager() ==
          &Unit->getASTContext().getSourceManager().getFileManager());
 
+  const auto &TripleTo = Context.getTargetInfo().getTriple();
+  const auto &TripleFrom = Unit->getASTContext().getTargetInfo().getTriple();
+  // The imported AST had been generated for a different target.
+  // Some parts of the triple in the loaded ASTContext can be unknown while the
+  // very same parts in the target ASTContext are known. Thus we check for the
+  // known parts only.
+  if (!hasEqualKnownFields(TripleTo, TripleFrom)) {
+    // TODO: Pass the SourceLocation of the CallExpression for more precise
+    // diagnostics.
+    Context.getDiagnostics().Report(diag::err_ctu_incompat_triple)
+        << Unit->getMainFileName() << TripleTo.str() << TripleFrom.str();
+    // TODO: Add statistics here.
+    return llvm::make_error<IndexError>(index_error_code::triple_mismatch);
+  }
+
+  const auto &LangTo = Context.getLangOpts();
+  const auto &LangFrom = Unit->getASTContext().getLangOpts();
+  // FIXME: Currenty we do not support CTU across C++ and C and across
+  // different dialects of C++.
+  if (LangTo.CPlusPlus != LangFrom.CPlusPlus) {
+    // TODO: Add statistics here.
+    return llvm::make_error<IndexError>(index_error_code::lang_mismatch);
+  }
+
   TranslationUnitDecl *TU = Unit->getASTContext().getTranslationUnitDecl();
   if (const FunctionDecl *ResultDecl =
           findFunctionInDeclContext(TU, LookupFnName))
     return importDefinition(ResultDecl);
   return llvm::make_error<IndexError>(index_error_code::failed_import);
 }
 
 void CrossTranslationUnitContext::emitCrossTUDiagnostics(const IndexError &IE) {

test/Analysis/ctu-different-triples.cpp

+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu \
+// RUN:   -emit-pch -o %t/ctudir/ctu-other.cpp.ast %S/Inputs/ctu-other.cpp
+// RUN: cp %S/Inputs/externalFnMap.txt %t/ctudir/externalFnMap.txt
+// RUN: %clang_cc1 -triple powerpc64-montavista-linux-gnu -fsyntax-only \
+// RUN:   -analyze -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN:   -analyzer-config ctu-dir=%t/ctudir \
+// RUN:   -verify %s
+
+// We expect an error in this file, but without a location.
+// expected-error-re@./ctu-different-triples.cpp:*{{imported AST from {{.*}} had been generated for a different target, current: powerpc64-montavista-linux-gnu, imported: x86_64-pc-linux-gnu}}
+
+int f(int);
+
+int main() {
+  return f(5);
+}

test/Analysis/ctu-unknown-parts-in-triples.cpp

+// We do not expect any error when one part of the triple is unknown, but other
+// known parts are equal.
+
+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu \
+// RUN:   -emit-pch -o %t/ctudir/ctu-other.cpp.ast %S/Inputs/ctu-other.cpp
+// RUN: cp %S/Inputs/externalFnMap.txt %t/ctudir/externalFnMap.txt
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -analyze \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN:   -analyzer-config ctu-dir=%t/ctudir \
+// RUN:   -verify %s
+
+// expected-no-diagnostics
+
+int f(int);
+
+int main() {
+  return f(5);
+}