[CTU] Add more lit tests and better error handling

[martong]

Summary

Adding some more CTU list tests. E.g. to check if a construct is unsupported.
We also slightly modify the handling of the return value of the Import
function from ASTImporter.

[Szelethus]

test/Analysis/ctu-main.c

3–5 ↗

(On Diff #176149)

Could you please break these lines?

// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu %S/Inputs/ctu-other.c \
// RUN:    -emit-pch -o %t/ctudir2/ctu-other.c.ast
//
// RUN: cp %S/Inputs/externalFnMap2.txt %t/ctudir2/externalFnMap.txt
//
// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -std=c89 -analyze -verify %s
// RUN:   -analyzer-checker=core \
// RUN:   -analyzer-checker=debug.ExprInspection \
// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
// RUN:   -analyzer-config ctu-dir=%t/ctudir2

4 ↗

(On Diff #176149)

This is a question rather than anything else, why do we have both externalFnMap2.txt and externalFnMap.txt?

[Szelethus]

Also, maybe it'd be worth making a CTU directory under test/Analysis for CTU related test files.

[a_sidorin]

Hi Gabor,
Please find my comments inline.

lib/CrossTU/CrossTranslationUnit.cpp

251 ↗

(On Diff #176149)

Conditionals with a single-line body don't require braces.

test/Analysis/Inputs/ctu-other.c

1 ↗

(On Diff #176149)

Please clang-format the new files.

6 ↗

(On Diff #176149)

Please use a consistent naming style across the file. There are names starting with capital, having underscores and written like this.

[martong]

• Break long RUN lines
• Clang format the test files
• Use consistent naming style
• Remove braces

test/Analysis/Inputs/ctu-other.c

6 ↗

(On Diff #176149)

Ok, I renamed the things, I was trying to be consistent with the LLVM style, one exception is the name of variables still start with small case.
I also restructured the file a bit and added more comments, so it is more cleaner which things belong together to the same test.

test/Analysis/ctu-main.c

3–5 ↗

(On Diff #176149)

Ok, good point.

4 ↗

(On Diff #176149)

externalFnMap.txt goes for ctu-other.cpp.
externalFnMap2.txt goes for ctu-other.c.
Perhaps we should rename them in the Inputs directory to include the indexed file name. E.g. ctu-other.cpp.externalFnMap.txt. What do you think?

[martong]

Also, maybe it'd be worth making a CTU directory under test/Analysis for CTU related test files.

It is a good point, but I'd do that in the future once we have even more ctu related test files. Perhaps together with a new check-clang-analysis-ctu build target.

[martong]

lib/CrossTU/CrossTranslationUnit.cpp

247 ↗

(On Diff #176619)

TODO: add a comment that this function never returns with nullptr on success. And consequently CrossTranslationUnitContext::getCrossTUDefinition neither.

[Szelethus]

test/Analysis/ctu-main.c

4 ↗

(On Diff #176149)

Sounds great! :)

[martong]

test/Analysis/ctu-main.c

4 ↗

(On Diff #176149)

Ok, I have renamed the ExternalFnMap files.

[martong]

• Rename externalFnMap files

[xazax.hun]

LG!

cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp

 }
 
 llvm::Expected<const FunctionDecl *>
 CrossTranslationUnitContext::importDefinition(const FunctionDecl *FD) {
   assert(FD->hasBody() && "Functions to be imported should have body.");
 
   ASTImporter &Importer = getOrCreateASTImporter(FD->getASTContext());
   auto *ToDecl =
-      cast<FunctionDecl>(Importer.Import(const_cast<FunctionDecl *>(FD)));
+      cast_or_null<FunctionDecl>(Importer.Import(const_cast<FunctionDecl *>(FD)));
+  if (!ToDecl)
+    return llvm::make_error<IndexError>(index_error_code::failed_import);
   assert(ToDecl->hasBody());
   assert(FD->hasBody() && "Functions already imported should have body.");
   ++NumGetCTUSuccess;
   return ToDecl;
 }
 
 ASTImporter &
 CrossTranslationUnitContext::getOrCreateASTImporter(ASTContext &From) {

cfe/trunk/test/Analysis/Inputs/ctu-other.c

+// Test typedef and global variable in function.
+typedef struct {
+  int a;
+  int b;
+} FooBar;
+FooBar fb;
+int f(int i) {
+  if (fb.a) {
+    fb.b = i;
+  }
+  return 1;
+}
+
+// Test enums.
+enum B { x = 42,
+         l,
+         s };
+int enumCheck(void) {
+  return x;
+}
+
+// Test reporting an error in macro definition
+#define MYMACRO(ctx) \
+  ctx->a;
+struct S {
+  int a;
+};
+int g(struct S *ctx) {
+  MYMACRO(ctx);
+  return 0;
+}
+
+// Test that asm import does not fail.
+int inlineAsm() {
+  int res;
+  asm("mov $42, %0"
+      : "=r"(res));
+  return res;
+}
+
+// Implicit function.
+int identImplicit(int in) {
+  return in;
+}
+
+// ASTImporter doesn't support this construct.
+int structInProto(struct DataType {int a;int b; } * d) {
+  return 0;
+}

cfe/trunk/test/Analysis/Inputs/ctu-other.c.externalFnMap.txt

+c:@F@inlineAsm ctu-other.c.ast
+c:@F@g ctu-other.c.ast
+c:@F@f ctu-other.c.ast
+c:@F@enumCheck ctu-other.c.ast
+c:@F@identImplicit ctu-other.c.ast
+c:@F@structInProto ctu-other.c.ast

cfe/trunk/test/Analysis/Inputs/ctu-other.cpp.externalFnMap.txt

+c:@N@chns@F@chf1#I# ctu-other.cpp.ast
+c:@N@myns@N@embed_ns@F@fens#I# ctu-other.cpp.ast
+c:@F@g#I# ctu-other.cpp.ast
+c:@S@mycls@F@fscl#I#S ctu-other.cpp.ast
+c:@S@mycls@F@fcl#I# ctu-other.cpp.ast
+c:@N@myns@S@embed_cls@F@fecl#I# ctu-other.cpp.ast
+c:@S@mycls@S@embed_cls2@F@fecl2#I# ctu-other.cpp.ast
+c:@F@f#I# ctu-other.cpp.ast
+c:@N@myns@F@fns#I# ctu-other.cpp.ast
+c:@F@h#I# ctu-other.cpp.ast
+c:@F@h_chain#I# ctu-chain.cpp.ast
+c:@N@chns@S@chcls@F@chf4#I# ctu-chain.cpp.ast
+c:@N@chns@F@chf2#I# ctu-chain.cpp.ast
+c:@F@fun_using_anon_struct#I# ctu-other.cpp.ast
+c:@F@other_macro_diag#I# ctu-other.cpp.ast

cfe/trunk/test/Analysis/Inputs/externalFnMap.txt

-c:@N@chns@F@chf1#I# ctu-other.cpp.ast
-c:@N@myns@N@embed_ns@F@fens#I# ctu-other.cpp.ast
-c:@F@g#I# ctu-other.cpp.ast
-c:@S@mycls@F@fscl#I#S ctu-other.cpp.ast
-c:@S@mycls@F@fcl#I# ctu-other.cpp.ast
-c:@N@myns@S@embed_cls@F@fecl#I# ctu-other.cpp.ast
-c:@S@mycls@S@embed_cls2@F@fecl2#I# ctu-other.cpp.ast
-c:@F@f#I# ctu-other.cpp.ast
-c:@N@myns@F@fns#I# ctu-other.cpp.ast
-c:@F@h#I# ctu-other.cpp.ast
-c:@F@h_chain#I# ctu-chain.cpp.ast
-c:@N@chns@S@chcls@F@chf4#I# ctu-chain.cpp.ast
-c:@N@chns@F@chf2#I# ctu-chain.cpp.ast
-c:@F@fun_using_anon_struct#I# ctu-other.cpp.ast
-c:@F@other_macro_diag#I# ctu-other.cpp.ast

cfe/trunk/test/Analysis/ctu-main.c

+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir2
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu \
+// RUN:   -emit-pch -o %t/ctudir2/ctu-other.c.ast %S/Inputs/ctu-other.c
+// RUN: cp %S/Inputs/ctu-other.c.externalFnMap.txt %t/ctudir2/externalFnMap.txt
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -std=c89 -analyze \
+// RUN:   -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN:   -analyzer-config ctu-dir=%t/ctudir2 \
+// RUN:   -verify %s
+
+void clang_analyzer_eval(int);
+
+// Test typedef and global variable in function.
+typedef struct {
+  int a;
+  int b;
+} FooBar;
+extern FooBar fb;
+int f(int);
+void testGlobalVariable() {
+  clang_analyzer_eval(f(5) == 1);         // expected-warning{{TRUE}}
+}
+
+// Test enums.
+int enumCheck(void);
+enum A { x,
+         y,
+         z };
+void testEnum() {
+  clang_analyzer_eval(x == 0);            // expected-warning{{TRUE}}
+  clang_analyzer_eval(enumCheck() == 42); // expected-warning{{TRUE}}
+}
+
+// Test that asm import does not fail.
+int inlineAsm();
+int testInlineAsm() {
+  return inlineAsm();
+}
+
+// Test reporting error in a macro.
+struct S;
+int g(struct S *);
+void testMacro(void) {
+  g(0); // expected-warning@Inputs/ctu-other.c:29 {{Access to field 'a' results in a dereference of a null pointer (loaded from variable 'ctx')}}
+}
+
+// The external function prototype is incomplete.
+// warning:implicit functions are prohibited by c99
+void testImplicit() {
+  int res = identImplicit(6);   // external implicit functions are not inlined
+  clang_analyzer_eval(res == 6); // expected-warning{{TRUE}}
+}
+
+// Tests the import of functions that have a struct parameter
+// defined in its prototype.
+struct DataType {
+  int a;
+  int b;
+};
+int structInProto(struct DataType *d);
+void testStructDefInArgument() {
+  struct DataType d;
+  d.a = 1;
+  d.b = 0;
+  clang_analyzer_eval(structInProto(&d) == 0); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+}

cfe/trunk/test/Analysis/ctu-main.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: %clang_cc1 -triple x86_64-pc-linux-gnu \
 // RUN:   -emit-pch -o %t/ctudir/ctu-chain.cpp.ast %S/Inputs/ctu-chain.cpp
-// RUN: cp %S/Inputs/externalFnMap.txt %t/ctudir/
+// RUN: cp %S/Inputs/ctu-other.cpp.externalFnMap.txt %t/ctudir/externalFnMap.txt
 // RUN: %clang_analyze_cc1 -triple x86_64-pc-linux-gnu \
 // 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
 // RUN: %clang_analyze_cc1 -triple x86_64-pc-linux-gnu \
 // RUN:   -analyzer-checker=core,debug.ExprInspection \
 // RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \