Page MenuHomePhabricator

[CTU] Add triple/lang mismatch handling
ClosedPublic

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

Details

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).

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.Nov 30 2018, 9:54 AM
martong updated this revision to Diff 176172.Nov 30 2018, 11:30 AM
  • Add lit tests

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

lib/CrossTU/CrossTranslationUnit.cpp
59 ↗(On Diff #176172)

// end of namespace llvm

210 ↗(On Diff #176172)

TODO:

214 ↗(On Diff #176172)

Use punctuation.

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 ↗(On Diff #176172)

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

35 ↗(On Diff #176172)

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 ↗(On Diff #176172)

We can use Triple::isOSUnknown() instead.

59 ↗(On Diff #176172)

// end namespace llvm is much more common.

203 ↗(On Diff #176172)

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

211 ↗(On Diff #176172)

Comments should end with a dot. Same below.

test/Analysis/ctu-unknown-parts-in-triples.cpp
8 ↗(On Diff #176172)

Two spaces after ExprInspection.

martong updated this revision to Diff 176417.Dec 3 2018, 9:22 AM
martong marked 13 inline comments as done.
  • Address review comments

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

lib/CrossTU/CrossTranslationUnit.cpp
31 ↗(On Diff #176172)

Ok, I moved it into the unnamed namespace below.

35 ↗(On Diff #176172)

Ok, I split that up with early returns.

59 ↗(On Diff #176172)

Moved to the unnamed namespace.

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 accepted this revision.Dec 3 2018, 12:30 PM

LGTM, thanks!

This revision is now accepted and ready to land.Dec 3 2018, 12:30 PM
martong updated this revision to Diff 176584.Dec 4 2018, 3:06 AM
  • Break long RUN lines
xazax.hun requested changes to this revision.Dec 4 2018, 7:05 AM
xazax.hun added inline comments.
include/clang/Basic/DiagnosticCrossTUKinds.td
19 ↗(On Diff #176584)

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 ↗(On Diff #176584)

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.

This revision now requires changes to proceed.Dec 4 2018, 7:05 AM
martong updated this revision to Diff 176671.Dec 4 2018, 10:30 AM
martong marked 4 inline comments as done.
  • Diagnose a warning (which may be upgradable to an error)
martong added inline comments.Dec 4 2018, 10:31 AM
include/clang/Basic/DiagnosticCrossTUKinds.td
19 ↗(On Diff #176584)

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

lib/CrossTU/CrossTranslationUnit.cpp
212 ↗(On Diff #176584)

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 updated this revision to Diff 176672.Dec 4 2018, 10:33 AM
  • Forgot to rename err_ctu_incompat_triple -> warn_ctu_incompat_triple
xazax.hun added inline comments.Dec 4 2018, 10:34 AM
lib/CrossTU/CrossTranslationUnit.cpp
212 ↗(On Diff #176584)

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.

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

martong marked 2 inline comments as done.Dec 4 2018, 12:10 PM
martong added inline comments.
lib/CrossTU/CrossTranslationUnit.cpp
212 ↗(On Diff #176584)

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

martong updated this revision to Diff 176686.Dec 4 2018, 12:11 PM
martong marked an inline comment as done.
  • Add a case to emitCrossTUDiagnostics
martong updated this revision to Diff 176687.Dec 4 2018, 12:13 PM
  • Use clang_analyze_cc1

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

Ok, changed that.

This revision is now accepted and ready to land.Dec 7 2018, 7:27 AM
This revision was automatically updated to reflect the committed changes.