Page MenuHomePhabricator

[CTU] Add more lit tests and better error handling
ClosedPublic

Authored by martong on Nov 30 2018, 9:25 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.Nov 30 2018, 9:25 AM
Szelethus added inline comments.Nov 30 2018, 9:37 AM
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?

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

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 updated this revision to Diff 176619.Dec 4 2018, 6:30 AM
martong marked 8 inline comments as done.
  • 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?

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 marked an inline comment as done.Dec 5 2018, 8:44 AM
martong added inline comments.
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 added inline comments.Dec 5 2018, 9:21 AM
test/Analysis/ctu-main.c
4 ↗(On Diff #176149)

Sounds great! :)

martong marked 2 inline comments as done.Dec 7 2018, 3:44 AM
martong added inline comments.
test/Analysis/ctu-main.c
4 ↗(On Diff #176149)

Ok, I have renamed the ExternalFnMap files.

martong updated this revision to Diff 177170.Dec 7 2018, 3:45 AM
martong marked an inline comment as done.
  • Rename externalFnMap files
This revision is now accepted and ready to land.Dec 7 2018, 7:38 AM
This revision was automatically updated to reflect the committed changes.