[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.
xazax.hun added a comment. Generally looks good, I only wonder if this works well with inline namespaces. Could you test? Repository: rC Clang https://reviews.llvm.org/D48027 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.
xazax.hun accepted this revision. xazax.hun added a comment. Sorry for the delay, I think this is OK to commit. As a possible improvement, I can imagine making it slightly stricter, e.g. you could only skip QualifiedNames for inline namespaces and the beginning. Maybe add support for staring with `""` or `::` to even disable skipping namespaces at the beginning? But these are just nice to have features, I am perfectly ok with not having them or doing it in a followup patch. https://reviews.llvm.org/D48027 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.
xazax.hun added a comment. Oh, and thanks for working on this, this improvement was long overdue, but everybody was busy with something else :) https://reviews.llvm.org/D48027 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51041: [clang-tidy] Don't run misc-unused-using-decls check in C++17.
xazax.hun added inline comments. Comment at: clang-tidy/misc/UnusedUsingDeclsCheck.cpp:35 + // it is not ture in C++17's template argument deduction. + if (!getLangOpts().CPlusPlus || getLangOpts().CPlusPlus17 || + getLangOpts().CPlusPlus2a) Isn't the check for `getLangOpts().CPlusPlus2a` redundant? Shouldn't it imply the C++17 flag? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51041 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer
xazax.hun added a comment.
The analyzer can only reason about const variables this way, right? Maybe we
should only import the initializers for such variables to have better
performance? What do you think?
Also, I wonder what happens with user-defined classes. Will the analyzer
evaluate the constructor if the variable is imported?
Comment at: include/clang/CrossTU/CrossTranslationUnit.h:114
+ llvm::Expected
+ getCrossTUDefinition(const VarDecl *VD, StringRef CrossTUDir,
+ StringRef IndexName);
I wonder if we need these overloads. Maybe having only the templated version
and having a static assert for the supported types is better? Asking the other
reviewers as well.
Comment at: include/clang/CrossTU/CrossTranslationUnit.h:138
llvm::Expected importDefinition(const FunctionDecl
*FD);
+ llvm::Expected importDefinition(const VarDecl *VD);
Same question as above.
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:114
+bool HasDefinition(const FunctionDecl *D, const FunctionDecl *&DefD) {
+ return D->hasBody(DefD);
Functions should start with lowercase letter. Maybe these could be static?
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:143
+CrossTranslationUnitContext::findDefInDeclContext(const DeclContext *DC,
+ StringRef LookupFnName) {
assert(DC && "Declaration Context must not be null");
Fn in the name refers to a function, maybe that could be changed as well.
Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:354
+if (!VD->hasExternalStorage() ||
+VD->getAnyInitializer() != nullptr)
+ return true;
Will this work for zero initialized globals (i.e. not having an initializer
expression) that are defined in the current translation unit? It would be great
to have a test case for that.
https://reviews.llvm.org/D46421
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer
xazax.hun added inline comments. Comment at: include/clang/CrossTU/CrossTranslationUnit.h:117 - /// This function loads a function definition from an external AST - ///file. + /// \brief This function loads a definition from an external AST file. /// Adding briefs are unnecessary, see https://reviews.llvm.org/rL331834 https://reviews.llvm.org/D46421 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47135: [analyzer] A checker for dangling internal buffer pointers in C++
xazax.hun added a comment.
Looks good so far, some comments inline.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:58
+
+ auto *TypeDecl = TypedR->getValueType().getTypePtr()->getAsCXXRecordDecl();
+ if (TypeDecl->getName() != "basic_string")
QualType should have overloaded `->` operator, I think you can remove the
`getTypePtr`.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:65
+ if (Call.isCalled(CStrFn)) {
+SymbolRef RawPtr = Call.getReturnValue().getAsSymbol();
+State = State->set(TypedR, RawPtr);
I wonder if we can always get a symbol.
I can think of two cases when the call above could fail:
* Non-standard implementation that does not return a pointer
* The analyzer able to inline stuff and the returned value is a constant (a
specific address that is shared between all empty strings in some
implementation?)
Even though I do find any of the above likely. @NoQ what do you think? Does
this worth a check?
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:73
+if (State->contains(TypedR)) {
+ const SymbolRef *StrBufferPtr = State->get(TypedR);
+ const Expr *Origin = Call.getOriginExpr();
What if no symbol is associated with the region? Won't this return null that we
dereference later on?
Comment at: test/Analysis/dangling-internal-buffer.cpp:24
+
+void deref_after_scope_char() {
+ const char *c;
I would like to see test cases that does not trigger warning.
https://reviews.llvm.org/D47135
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47135: [analyzer] A checker for dangling internal buffer pointers in C++
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:73
+if (State->contains(TypedR)) {
+ const SymbolRef *StrBufferPtr = State->get(TypedR);
+ const Expr *Origin = Call.getOriginExpr();
xazax.hun wrote:
> What if no symbol is associated with the region? Won't this return null that
> we dereference later on?
Oh, never mind this one, I did not notice the `contains` call above.
https://reviews.llvm.org/D47135
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47135: [analyzer] A checker for dangling internal buffer pointers in C++
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:65
+ if (Call.isCalled(CStrFn)) {
+SymbolRef RawPtr = Call.getReturnValue().getAsSymbol();
+State = State->set(TypedR, RawPtr);
xazax.hun wrote:
> I wonder if we can always get a symbol.
> I can think of two cases when the call above could fail:
> * Non-standard implementation that does not return a pointer
> * The analyzer able to inline stuff and the returned value is a constant (a
> specific address that is shared between all empty strings in some
> implementation?)
>
> Even though I do find any of the above likely. @NoQ what do you think? Does
> this worth a check?
Sorry for the spam. Unfortunately, it is not possible to edit the comments.
I do *not* find any of the above likely.
https://reviews.llvm.org/D47135
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47135: [analyzer] A checker for dangling internal buffer pointers in C++
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D47135 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47416: [analyzer] Clean up the program state map of DanglingInternalBufferChecker
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LG! Repository: rC Clang https://reviews.llvm.org/D47416 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45517: [analyzer] False positive refutation with Z3
xazax.hun added inline comments. Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:2382 +// Reset the solver +RefutationMgr.reset(); + } george.karpenkov wrote: > george.karpenkov wrote: > > (apologies in advance for nitpicking not on your code). > > > > Currently, this is written in a stateful way: we have a solver, at each > > iteration we add constraints, and at the end we reset it. To me it would > > make considerably more sense to write the code in a functional style: as we > > go, generate a vector of formulas, then once we reach the path end, create > > the solver object, check satisfiability, and then destroy the entire solver. > Elaborating more: we are already forced to have visitor object state, let's > use that. `RefutationMgr` is essentially a wrapper around a Z3 solver object, > let's just create one when visitor is constructed (directly or in unique_ptr) > and then rely on the destructor to destroy it. > Then no `reset` is necessary. Note that while constructing the constraint solver here might make perfect sense now, it also inhibits incremental solving. If we do not plan to experiment with incremental solvers anytime soon I am fine with this direction as well. Comment at: lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp:1264 + +if (OnlyPurged && SuccCR.contains(Sym)) + continue; george.karpenkov wrote: > I would guess that this is an optimization done in order not to re-add the > constraints we already have. > I think we should really not bother doing that, as Z3 will do a much better > job here then we can. Note that we are using lots of domain knowledge here like we have the most info about a symbol just before it dies. Also This optimization is a single lookup on the symbol level. I am not sure if Z3 could deal with this on the symbol level. It might need to do this on the constraint level. My point is, I am perfectly fine removing this optimization but I would like to see some performance numbers first either on a project that exercises refutation quite a bit or on some synthetic test cases. https://reviews.llvm.org/D45517 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38171: Implement clang-tidy check aliases.
xazax.hun added a comment. @leanil : Could you add a test case where the warnings are explicitly disabled in the compilation command and also one where only the aliased warning is explicitly disabled? In https://reviews.llvm.org/D38171#878814, @lebedev.ri wrote: > I feel like the current handling of the clang-tidy check aliases needs > adjusting. > If one enables all the checks (`Checks: '*'`), and then disables some of them > on check-by-check basis, if the disabled check has aliases > (`cppcoreguidelines-pro-type-vararg` vs `hicpp-vararg`, > `hicpp-no-array-decay` > vs `cppcoreguidelines-pro-bounds-array-to-pointer-decay` and so on) > each of the aliases must be explicitly be disabled too. That is somewhat intentional I think. Lets imagined you are interested in cppcoreguidelines but not interested in high integrity cpp. It would be great to be able to use Checks: `'cppcoreguidelines*,-hicpp*'` without having to worry about the aliases. In https://reviews.llvm.org/D38171#878814, @lebedev.ri wrote: > If that is intentional, perhaps there could be a config option to either > disable > creation/registration of the check aliases altogether, or an option to threat > aliases as a hard link, so anything happening to any of the aliases/base > check > would happen to all of them. I think the source of the problems is that conceptually there are two distinct reasons to disable a check. One reason when the user is not interested in the results regardless what is the category or name of the check. In this case, all aliases should be disabled. The second is when a category of checks (e.g.: a guideline) is not applicable to the code. In this case aliases should not be disabled. So I think a global option would not solve this issue. A better solution might be a per glob notation. So `-` could mean do not disable aliases and `--` could mean disable aliases as well but that is just an example. All in all, I think this is not related strictly to this issue and I think we should discuss this separately on the mailing list. In https://reviews.llvm.org/D38171#878814, @lebedev.ri wrote: > (Also, if the check has parameters, and one specifies different parameters > for the base check, and the aliases, what happens?) I think this is a very good point, this needs to be documented somewhere. https://reviews.llvm.org/D38171 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38171: Implement clang-tidy check aliases.
xazax.hun added a comment. In https://reviews.llvm.org/D38171#878808, @alexfh wrote: > András, that's definitely an interesting idea. However, it might be > interesting to explore a more principled approach: > > 1. Make `clang-diagnostic-*` checks first-class citizens and take full > control of all diagnostics, i.e. disable all Clang diagnostics by default, > and enable the ones that correspond to the enabled clang-diagnostic checks. I agree that this is a good idea. I think it could be done in this patch. But to be sure, could you elaborate on what do you mean by first-class citizen? > 2. Make aliases first-class citizens (there was a proposal as well, but we > didn't arrive to a consensus at that time). That would include the ability to > configure an alias name for any check including clang-diagnostic- and > clang-analyzer- checks. Do you have a link to the proposal? What do you mean by the ability to configure? As in having a config file or registering aliases in modules like now? If you mean the former, I think that is better addressed in a follow-up patch. > 3. Use aliases to map clang-diagnostic- checks to check names under cert-, > hicpp-, etc. > > I didn't carefully consider all possible implications and there may be > issues with any or all of the three parts of this, but I think it's worth > exploring. https://reviews.llvm.org/D38171 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38171: Implement clang-tidy check aliases.
xazax.hun added a comment. In https://reviews.llvm.org/D38171#880022, @lebedev.ri wrote: > A mail [0] posted by @JonasToth is about the similar problem, i think we can > continue there. Great! Could you summarize your points there as well? Thanks in advance. https://reviews.llvm.org/D38171 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31538: [analyzer] MisusedMovedObjectChecker: Fix a false positive on state-resetting a base-class sub-object.
xazax.hun accepted this revision. xazax.hun added a comment. I think there was only one comment but that is already addressed in a dependent revision. So I think this one is good as is. https://reviews.llvm.org/D31538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38674: [analyzer] MisusedMovedObjectChecker: More precise warning message
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D38674 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38675: [analyzer] MisusedMovedObjectChecker: Moving the checker out of alpha state
xazax.hun added a comment. In https://reviews.llvm.org/D38675#891750, @danielmarjamaki wrote: > > However, the checker seems to work with a low false positive rate. (<15 on > > the LLVM, 6 effectively different) > > This does not sound like a low false positive rate to me. Could you describe > what the false positives are? Is it possible to fix them? Note that the unique findings are 6. I think there are non-alpha checks with more false positives. https://reviews.llvm.org/D38675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31541: [analyzer] MisusedMovedObjectChecker: Add a printState() method.
xazax.hun accepted this revision. xazax.hun added a comment. LGTM! https://reviews.llvm.org/D31541 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38702: [Analyzer] Do not segfault on unexpected call_once implementation
xazax.hun added a comment. Did you link the correct bug in the description? The one you linked was closed long ago. https://reviews.llvm.org/D38702 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38171: Implement clang-tidy check aliases.
xazax.hun added a comment. Let's also summarize what do we have now and what do we want. > I also think this sounds good, though I'm not quite sure it can be done in > this patch. Anyway, we should definitely specify what we expect from > first-class citizen checks. Please correct & extend the list: > > - can be enabled or disabled through a unique name This is addressed by the current pathc. > - can have config options Do you know warnings that have config options? Does this make sense for this featre? > - can have aliases Does that make sense to be able to add aliases to warning aliases? I > - inherits ClangTidyCheck I think this is just technical and this does not really affect the users. In https://reviews.llvm.org/D38171#878808, @alexfh wrote: > 1. Make `clang-diagnostic-*` checks first-class citizens and take full > control of all diagnostics, i.e. disable all Clang diagnostics by default, > and enable the ones that correspond to the enabled clang-diagnostic checks. I think this might be a good idea in general. @leanil could you address this point in this patch? > 2. Make aliases first-class citizens (there was a proposal as well, but we > didn't arrive to a consensus at that time). That would include the ability to > configure an alias name for any check including clang-diagnostic- and > clang-analyzer- checks. I am not sure that this makes sense, see my points above. > 3. Use aliases to map clang-diagnostic- checks to check names under cert-, > hicpp-, etc. I agree. https://reviews.llvm.org/D38171 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38728: [analyzer] Use the signature of the primary template for issue hash calculation
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Core/IssueHash.cpp:39
+ // primary template.
+ if (const FunctionDecl *InstantiatedFrom =
+ Target->getInstantiatedFromMemberFunction())
martong wrote:
> Could we use here FunctionDecl::getPrimaryTemplate() ? That seems more
> general, it handles both specializations and instantiations.
Unfortunately `getPrimaryTemplate` is not sufficient. The function might be a
member function of a template class. In this case, there is no primary template
for the function (only for the enclosing class) but it still depends on a
template parameter.
Comment at: test/Analysis/bug_hash_test.cpp:61
-// CHECK: diagnostics
+template
+T f(T i) {
martong wrote:
> We could add a few more test cases:
> - a function template in class template
> - specializations vs instantiations
> - the combination of the above two (?)
>
Good point.
Comment at: test/Analysis/bug_hash_test.cpp:1363
// CHECK-NEXT:
+// CHECK-NEXT:
+// CHECK-NEXT: path
martong wrote:
> I am not sure if this is possible, but could we add unit test just for the
> `GetSignature` function? Instead of these huge plist files?
>
> I am thinking something like this:
> https://github.com/martong/friend-stats/blob/ed0c69ea3669c933204c799f59b85cd7b2507c34/ut/FriendFunctionsTest.cpp#L31
I think it is more convenient to use regression test for this purpose than
unittests. But I replaced the long plist checking with something much more
concise.
https://reviews.llvm.org/D38728
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38728: [analyzer] Use the signature of the primary template for issue hash calculation
xazax.hun added a comment. In https://reviews.llvm.org/D38728#895669, @NoQ wrote: > I think it would be great to split them into two different patches, to be > able to easily see how the change in the hashing affects the tests (and maybe > revert easily if something goes wrong). So you would commit the hash change first and the test change on the top of it? Or the other way around? https://reviews.llvm.org/D38728 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34512: Add preliminary Cross Translation Unit support library
xazax.hun added a comment. In https://reviews.llvm.org/D34512#890537, @r.stahl wrote: > In https://reviews.llvm.org/D34512#877643, @xazax.hun wrote: > > > - Unittests now creates temporary files at the correct location > > - Temporary files are also cleaned up when the process is terminated > > - Absolute paths are handled correctly by the library > > > I think there is an issue with this change. > > First, the function map generation writes absolute paths to the map file. Now > when the `parseCrossTUIndex` function parses it, it will no longer prepend > the paths with the CTUDir and therefore expect the precompiled AST files at > the exact path of the source file. This seems like a bad requirement, because > the user would have to overwrite his source files. > > If I understand your intention correctly, a fix would be to replace the > `is_absolute` check with a check for `CTUDir == ""` in the > `parseCrossTUIndex` function. Good catch, that is right! Repository: rL LLVM https://reviews.llvm.org/D34512 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38842: [CrossTU] Fix handling of Cross Translation Unit directory path
xazax.hun created this revision.
The function map generator tool always creates absolute path. The correct logic
to determine whether a path should be handled as absolute depends on the value
of the CrossTU directory. Added a unit test to avoid regressions.
https://reviews.llvm.org/D38842
Files:
lib/CrossTU/CrossTranslationUnit.cpp
unittests/CrossTU/CrossTranslationUnitTest.cpp
Index: unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- unittests/CrossTU/CrossTranslationUnitTest.cpp
+++ unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -109,9 +109,9 @@
TEST(CrossTranslationUnit, IndexFormatCanBeParsed) {
llvm::StringMap Index;
- Index["a"] = "b";
- Index["c"] = "d";
- Index["e"] = "f";
+ Index["a"] = "/b/f1";
+ Index["c"] = "/d/f2";
+ Index["e"] = "/f/f3";
std::string IndexText = createCrossTUIndexString(Index);
int IndexFD;
@@ -134,5 +134,25 @@
EXPECT_TRUE(Index.count(E.getKey()));
}
+TEST(CrossTranslationUnit, CTUDirIsHandledCorrectly) {
+ llvm::StringMap Index;
+ Index["a"] = "/b/c/d";
+ std::string IndexText = createCrossTUIndexString(Index);
+
+ int IndexFD;
+ llvm::SmallString<256> IndexFileName;
+ ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("index", "txt", IndexFD,
+ IndexFileName));
+ llvm::ToolOutputFile IndexFile(IndexFileName, IndexFD);
+ IndexFile.os() << IndexText;
+ IndexFile.os().flush();
+ EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+ llvm::Expected> IndexOrErr =
+ parseCrossTUIndex(IndexFileName, "/ctudir");
+ EXPECT_TRUE((bool)IndexOrErr);
+ llvm::StringMap ParsedIndex = IndexOrErr.get();
+ EXPECT_EQ(ParsedIndex["a"], "/ctudir/b/c/d");
+}
+
} // end namespace cross_tu
} // end namespace clang
Index: lib/CrossTU/CrossTranslationUnit.cpp
===
--- lib/CrossTU/CrossTranslationUnit.cpp
+++ lib/CrossTU/CrossTranslationUnit.cpp
@@ -93,7 +93,7 @@
index_error_code::multiple_definitions, IndexPath.str(), LineNo);
StringRef FileName = LineRef.substr(Pos + 1);
SmallString<256> FilePath = CrossTUDir;
- if (llvm::sys::path::is_absolute(FileName))
+ if (CrossTUDir.empty())
FilePath = FileName;
else
llvm::sys::path::append(FilePath, FileName);
Index: unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- unittests/CrossTU/CrossTranslationUnitTest.cpp
+++ unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -109,9 +109,9 @@
TEST(CrossTranslationUnit, IndexFormatCanBeParsed) {
llvm::StringMap Index;
- Index["a"] = "b";
- Index["c"] = "d";
- Index["e"] = "f";
+ Index["a"] = "/b/f1";
+ Index["c"] = "/d/f2";
+ Index["e"] = "/f/f3";
std::string IndexText = createCrossTUIndexString(Index);
int IndexFD;
@@ -134,5 +134,25 @@
EXPECT_TRUE(Index.count(E.getKey()));
}
+TEST(CrossTranslationUnit, CTUDirIsHandledCorrectly) {
+ llvm::StringMap Index;
+ Index["a"] = "/b/c/d";
+ std::string IndexText = createCrossTUIndexString(Index);
+
+ int IndexFD;
+ llvm::SmallString<256> IndexFileName;
+ ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("index", "txt", IndexFD,
+ IndexFileName));
+ llvm::ToolOutputFile IndexFile(IndexFileName, IndexFD);
+ IndexFile.os() << IndexText;
+ IndexFile.os().flush();
+ EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+ llvm::Expected> IndexOrErr =
+ parseCrossTUIndex(IndexFileName, "/ctudir");
+ EXPECT_TRUE((bool)IndexOrErr);
+ llvm::StringMap ParsedIndex = IndexOrErr.get();
+ EXPECT_EQ(ParsedIndex["a"], "/ctudir/b/c/d");
+}
+
} // end namespace cross_tu
} // end namespace clang
Index: lib/CrossTU/CrossTranslationUnit.cpp
===
--- lib/CrossTU/CrossTranslationUnit.cpp
+++ lib/CrossTU/CrossTranslationUnit.cpp
@@ -93,7 +93,7 @@
index_error_code::multiple_definitions, IndexPath.str(), LineNo);
StringRef FileName = LineRef.substr(Pos + 1);
SmallString<256> FilePath = CrossTUDir;
- if (llvm::sys::path::is_absolute(FileName))
+ if (CrossTUDir.empty())
FilePath = FileName;
else
llvm::sys::path::append(FilePath, FileName);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38728: [analyzer] Use the signature of the primary template for issue hash calculation
xazax.hun updated this revision to Diff 118788.
xazax.hun added a comment.
- Rebase based on the dependent revision and minor cleanups
https://reviews.llvm.org/D38728
Files:
lib/StaticAnalyzer/Core/IssueHash.cpp
test/Analysis/bug_hash_test.cpp
test/Analysis/edges-new.mm
Index: test/Analysis/edges-new.mm
===
--- test/Analysis/edges-new.mm
+++ test/Analysis/edges-new.mm
@@ -20288,7 +20288,7 @@
// CHECK-NEXT:typeBad deallocator
// CHECK-NEXT:
check_nameunix.MismatchedDeallocator
// CHECK-NEXT:
-// CHECK-NEXT:
issue_hash_content_of_line_in_contextd9dbbf68db41ab74e2158f4b131abe34
+// CHECK-NEXT:
issue_hash_content_of_line_in_context046c88d1c91ff46d6506dff5ff880756
// CHECK-NEXT: issue_hash_function_offset0
// CHECK-NEXT: location
// CHECK-NEXT:
Index: test/Analysis/bug_hash_test.cpp
===
--- test/Analysis/bug_hash_test.cpp
+++ test/Analysis/bug_hash_test.cpp
@@ -71,15 +71,13 @@
template
void f(T) {
- clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void
f(double)$27$clang_analyzer_hashDump(5);$Category}}
- //
expected-warning@-1{{debug.ExprInspection$void
f(int)$27$clang_analyzer_hashDump(5);$Category}}
+ clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void
f(T)$27$clang_analyzer_hashDump(5);$Category}}
}
template
struct TX {
void f(T) {
-clang_analyzer_hashDump(5); // expected-warning
{{debug.ExprInspection$void
TX::f(double)$29$clang_analyzer_hashDump(5);$Category}}
- //
expected-warning@-1{{debug.ExprInspection$void
TX::f(int)$29$clang_analyzer_hashDump(5);$Category}}
+clang_analyzer_hashDump(5); // expected-warning
{{debug.ExprInspection$void TX::f(T)$29$clang_analyzer_hashDump(5);$Category}}
}
};
@@ -99,11 +97,17 @@
struct TTX {
template
void f(T, S) {
-clang_analyzer_hashDump(5); // expected-warning
{{debug.ExprInspection$void TTX::f(int,
int)$29$clang_analyzer_hashDump(5);$Category}}
+clang_analyzer_hashDump(5); // expected-warning
{{debug.ExprInspection$void TTX::f(T,
S)$29$clang_analyzer_hashDump(5);$Category}}
}
};
void g() {
+ // TX and TX is instantiated from the same code with the same
+ // source locations. The same error happining in both of the instantiations
+ // should share the common hash. This means we should not include the
+ // template argument for these types in the function signature.
+ // Note that, we still want the hash to be different for explicit
+ // specializations.
TX x;
TX y;
TX xl;
Index: lib/StaticAnalyzer/Core/IssueHash.cpp
===
--- lib/StaticAnalyzer/Core/IssueHash.cpp
+++ lib/StaticAnalyzer/Core/IssueHash.cpp
@@ -33,6 +33,13 @@
return "";
std::string Signature;
+ // When a flow sensitive bug happens in templated code we should not generate
+ // distinct hash value for every instantiation. Use the signature from the
+ // primary template.
+ if (const FunctionDecl *InstantiatedFrom =
+ Target->getTemplateInstantiationPattern())
+Target = InstantiatedFrom;
+
if (!isa(Target) && !isa(Target) &&
!isa(Target))
Signature.append(Target->getReturnType().getAsString()).append(" ");
Index: test/Analysis/edges-new.mm
===
--- test/Analysis/edges-new.mm
+++ test/Analysis/edges-new.mm
@@ -20288,7 +20288,7 @@
// CHECK-NEXT:typeBad deallocator
// CHECK-NEXT:check_nameunix.MismatchedDeallocator
// CHECK-NEXT:
-// CHECK-NEXT:issue_hash_content_of_line_in_contextd9dbbf68db41ab74e2158f4b131abe34
+// CHECK-NEXT:issue_hash_content_of_line_in_context046c88d1c91ff46d6506dff5ff880756
// CHECK-NEXT: issue_hash_function_offset0
// CHECK-NEXT: location
// CHECK-NEXT:
Index: test/Analysis/bug_hash_test.cpp
===
--- test/Analysis/bug_hash_test.cpp
+++ test/Analysis/bug_hash_test.cpp
@@ -71,15 +71,13 @@
template
void f(T) {
- clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void f(double)$27$clang_analyzer_hashDump(5);$Category}}
- // expected-warning@-1{{debug.ExprInspection$void f(int)$27$clang_analyzer_hashDump(5);$Category}}
+ clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void f(T)$27$clang_analyzer_hashDump(5);$Category}}
}
template
struct TX {
void f(T) {
-clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void TX::f(double)$29$clang_analyzer_hashDump(5);$Category}}
- // expected-warning@-1{{debug.ExprInspection$void TX::f(int)$29$clang_analyzer_hashDump(5);$Category}}
+clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInsp
[PATCH] D38688: [clang-tidy] Misc redundant expressions checker updated for macros
xazax.hun added a comment.
Thanks for working on this, these additions look very useful to me.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:124
+ case Stmt::CXXFunctionalCastExprClass:
+return cast(Left)->getTypeAsWritten() ==
You could merge this case with the above case if you cast Left and Right to
ExplicitCastExpr. So both label could end up executing the same code.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:330
+
+AST_MATCHER_P(Expr, expandedByMacro, std::set, Names) {
+ const SourceManager &SM = Finder->getASTContext().getSourceManager();
`llvm::StringSet` might be a more efficient choice.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:336
+std::string MacroName = Lexer::getImmediateMacroName(Loc, SM, LO);
+if (Names.find(MacroName) != Names.end())
+ return true;
You could use `count` here.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:357
+ ConstExpr = Result.Nodes.getNodeAs(CstId);
+ return ConstExpr && ConstExpr->isIntegerConstantExpr(Value, *Result.Context);
+}
I think you could just return the pointer and return a null pointer in case it
is not an integerConstantExpr. This way no compatibility overload needed.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:510
+// E.g.: from (X == 5) && (X == 5) retrieves 5 and 5
+static void retrieveConstExprFromBothSides(const BinaryOperator *&BinOp,
+ BinaryOperatorKind &MainOpcode,
I would prefer to return a pair of pointer to be returned to output parameters.
Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:579
-AST_MATCHER_P(Expr, expandedByMacro, std::set, Names) {
- const SourceManager &SM = Finder->getASTContext().getSourceManager();
- const LangOptions &LO = Finder->getASTContext().getLangOpts();
- SourceLocation Loc = Node.getExprLoc();
- while (Loc.isMacroID()) {
-std::string MacroName = Lexer::getImmediateMacroName(Loc, SM, LO);
-if (Names.find(MacroName) != Names.end())
+static bool isOperatorSetMeaningful(BinaryOperatorKind &Opcode,
+BinaryOperatorKind &LhsOpcode,
I think a comment might be good here what do you mean by meaningful.
Comment at: test/clang-tidy/misc-redundant-expression.cpp:387
// Should not match.
- if (X == 10 && Y == 10) return 1;
- if (X != 10 && X != 12) return 1;
Why did you remove these test cases?
Comment at: test/clang-tidy/misc-redundant-expression.cpp:404
if (X <= 10 && X >= 10) return 1;
- if (!X && !Y) return 1;
- if (!X && Y) return 1;
Same comment as above.
Repository:
rL LLVM
https://reviews.llvm.org/D38688
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38943: [ASTImporter] import SubStmt of CaseStmt
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM with a nit. Comment at: unittests/AST/ASTImporterTest.cpp:100 + // This might catch other cases. + Imported->dump(ToNothing); I would elaborate a bit more on the purpose of the code below. https://reviews.llvm.org/D38943 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37437: [analyzer] Fix some checker's output plist not containing the checker name
xazax.hun updated this revision to Diff 119141.
xazax.hun marked 2 inline comments as done.
xazax.hun added a comment.
Herald added a subscriber: szepet.
- Address review comments.
https://reviews.llvm.org/D37437
Files:
include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
lib/StaticAnalyzer/Checkers/CStringChecker.cpp
lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h
lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.h
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp
lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
lib/StaticAnalyzer/Checkers/ValistChecker.cpp
test/Analysis/malloc.c
Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -1720,13 +1720,6 @@
}
}
-char *dupstrWarn(const char *s) {
- const int len = strlen(s);
- char *p = (char*) smallocWarn(len + 1);
- strcpy(p, s); // expected-warning{{String copy function overflows destination buffer}}
- return p;
-}
-
int *radar15580979() {
int *data = (int *)malloc(32);
int *p = data ?: (int*)malloc(32); // no warning
Index: lib/StaticAnalyzer/Checkers/ValistChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ValistChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ValistChecker.cpp
@@ -64,7 +64,7 @@
CheckerContext &C) const;
void reportLeakedVALists(const RegionVector &LeakedVALists, StringRef Msg1,
StringRef Msg2, CheckerContext &C, ExplodedNode *N,
- bool ForceReport = false) const;
+ bool ReportUninit = false) const;
void checkVAListStartCall(const CallEvent &Call, CheckerContext &C,
bool IsCopy) const;
@@ -267,15 +267,15 @@
void ValistChecker::reportLeakedVALists(const RegionVector &LeakedVALists,
StringRef Msg1, StringRef Msg2,
CheckerContext &C, ExplodedNode *N,
-bool ForceReport) const {
+bool ReportUninit) const {
if (!(ChecksEnabled[CK_Unterminated] ||
-(ChecksEnabled[CK_Uninitialized] && ForceReport)))
+(ChecksEnabled[CK_Uninitialized] && ReportUninit)))
return;
for (auto Reg : LeakedVALists) {
if (!BT_leakedvalist) {
- BT_leakedvalist.reset(new BugType(CheckNames[CK_Unterminated],
-"Leaked va_list",
-categories::MemoryError));
+ BT_leakedvalist.reset(new BugType(
+ CheckNames[ReportUninit ? CK_Uninitialized : CK_Unterminated],
+ "Leaked va_list", categories::MemoryError));
BT_leakedvalist->setSuppressOnSink(true);
}
@@ -375,7 +375,7 @@
std::shared_ptr ValistChecker::ValistBugVisitor::VisitNode(
const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC,
-BugReport &BR) {
+BugReport &) {
ProgramStateRef State = N->getState();
ProgramStateRef StatePrev = PrevN->getState();
Index: lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
+++ lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
@@ -54,8 +54,8 @@
check::PointerEscape> {
CallDescription OpenFn, CloseFn;
- std::unique_ptr DoubleCloseBugType;
- std::unique_ptr LeakBugType;
+ mutable std::unique_ptr DoubleCloseBugType;
+ mutable std::unique_ptr LeakBugType;
void reportDoubleClose(SymbolRef FileDescSym,
const CallEvent &Call,
@@ -104,16 +104,7 @@
} // end anonymous namespace
SimpleStreamChecker::SimpleStreamChecker()
-: OpenFn("fopen"), CloseFn("fclose", 1) {
- // Initialize the bug types.
- DoubleCloseBugType.reset(
- new BugType(this, "Double fclose", "Unix Stream API Error"));
-
- LeakBugType.reset(
- new BugType(this, "Resource Leak", "Unix Stream API Error"));
- // Sinks are higher importance bugs as well as calls to assert() or exit(0).
- LeakBugType->setSuppressOnSink(true);
-}
+: OpenFn("fopen"), CloseFn("fclose", 1) {}
void SimpleStreamChecker::checkPostCall(const CallEvent &Call,
CheckerContext &C) const {
@@ -206,6 +197,10 @@
if (!ErrNode)
return;
+ if (!DoubleCloseBugType)
+DoubleCloseBugType.reset(
+new
[PATCH] D37437: [analyzer] Fix some checker's output plist not containing the checker name
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:308
if (StOutBound && !StInBound) {
+if (!Filter.CheckCStringOutOfBounds)
+ return state;
zaks.anna wrote:
> This seems to be related to the change in the test, correct? In the future,
> please, split changes like this one into their own separate commits.
Yes. I will do so in the future, thanks.
https://reviews.llvm.org/D37437
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38943: [ASTImporter] import SubStmt of CaseStmt
xazax.hun added inline comments. Comment at: unittests/AST/ASTImporterTest.cpp:100 + // This might catch other cases. + Imported->dump(ToNothing); r.stahl wrote: > xazax.hun wrote: > > I would elaborate a bit more on the purpose of the code below. > I will need help for that, since I do not have a better rationale myself. My > guess would be that print has different assumptions and error checking than > dump, so executing both increases coverage. So the reason you need this to traverse the AST and crash on the null pointers (when the patch is not applied right?) I think you could comment something like traverse the AST to catch certain bugs like poorly (not) imported subtrees. If we do not want to actually print the whole tree (because it might be noisy) you can also try using a no-op recursive AST visitor. https://reviews.llvm.org/D38943 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis
xazax.hun updated this revision to Diff 119458.
xazax.hun marked an inline comment as done.
xazax.hun added a comment.
- Update the scan-build part to work correctly with the accepted version of
libCrossTU
https://reviews.llvm.org/D30691
Files:
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
lib/StaticAnalyzer/Core/CMakeLists.txt
lib/StaticAnalyzer/Core/CallEvent.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/PathDiagnostic.cpp
lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
lib/StaticAnalyzer/Frontend/CMakeLists.txt
test/Analysis/Inputs/ctu-chain.cpp
test/Analysis/Inputs/ctu-other.cpp
test/Analysis/Inputs/externalFnMap.txt
test/Analysis/ctu-main.cpp
tools/scan-build-py/libscanbuild/__init__.py
tools/scan-build-py/libscanbuild/analyze.py
tools/scan-build-py/libscanbuild/arguments.py
tools/scan-build-py/libscanbuild/clang.py
tools/scan-build-py/libscanbuild/report.py
tools/scan-build-py/tests/unit/test_analyze.py
tools/scan-build-py/tests/unit/test_clang.py
Index: tools/scan-build-py/tests/unit/test_clang.py
===
--- tools/scan-build-py/tests/unit/test_clang.py
+++ tools/scan-build-py/tests/unit/test_clang.py
@@ -92,3 +92,16 @@
self.assertEqual('Checker One description', result.get('checker.one'))
self.assertTrue('checker.two' in result)
self.assertEqual('Checker Two description', result.get('checker.two'))
+
+
+class ClangIsCtuCapableTest(unittest.TestCase):
+def test_ctu_not_found(self):
+is_ctu = sut.is_ctu_capable('not-found-clang',
+'not-found-clang-func-mapping')
+self.assertFalse(is_ctu)
+
+
+class ClangGetTripleArchTest(unittest.TestCase):
+def test_arch_is_not_empty(self):
+arch = sut.get_triple_arch(['clang', '-E', '-'], '.')
+self.assertTrue(len(arch) > 0)
Index: tools/scan-build-py/tests/unit/test_analyze.py
===
--- tools/scan-build-py/tests/unit/test_analyze.py
+++ tools/scan-build-py/tests/unit/test_analyze.py
@@ -4,12 +4,12 @@
# This file is distributed under the University of Illinois Open Source
# License. See LICENSE.TXT for details.
-import libear
-import libscanbuild.analyze as sut
import unittest
import re
import os
import os.path
+import libear
+import libscanbuild.analyze as sut
class ReportDirectoryTest(unittest.TestCase):
@@ -333,3 +333,83 @@
def test_method_exception_not_caught(self):
self.assertRaises(Exception, method_exception_from_inside, dict())
+
+
+class PrefixWithTest(unittest.TestCase):
+
+def test_gives_empty_on_empty(self):
+res = sut.prefix_with(0, [])
+self.assertFalse(res)
+
+def test_interleaves_prefix(self):
+res = sut.prefix_with(0, [1, 2, 3])
+self.assertListEqual([0, 1, 0, 2, 0, 3], res)
+
+
+class MergeCtuMapTest(unittest.TestCase):
+
+def test_no_map_gives_empty(self):
+pairs = sut.create_global_ctu_function_map([])
+self.assertFalse(pairs)
+
+def test_multiple_maps_merged(self):
+concat_map = ['c:@F@fun1#I# ast/fun1.c.ast',
+ 'c:@F@fun2#I# ast/fun2.c.ast',
+ 'c:@F@fun3#I# ast/fun3.c.ast']
+pairs = sut.create_global_ctu_function_map(concat_map)
+self.assertTrue(('c:@F@fun1#I#', 'ast/fun1.c.ast') in pairs)
+self.assertTrue(('c:@F@fun2#I#', 'ast/fun2.c.ast') in pairs)
+self.assertTrue(('c:@F@fun3#I#', 'ast/fun3.c.ast') in pairs)
+self.assertEqual(3, len(pairs))
+
+def test_not_unique_func_left_out(self):
+concat_map = ['c:@F@fun1#I# ast/fun1.c.ast',
+ 'c:@F@fun2#I# ast/fun2.c.ast',
+ 'c:@F@fun1#I# ast/fun7.c.ast']
+pairs = sut.create_global_ctu_function_map(concat_map)
+self.assertFalse(('c:@F@fun1#I#', 'ast/fun1.c.ast') in pairs)
+self.assertFalse(('c:@F@fun1#I#', 'ast/fun7.c.ast') in pairs)
+self.assertTrue(('c:@F@fun2#I#', 'ast/fun2.c.ast') in pairs)
+self.assertEqual(1, len(pairs))
+
+def test_duplicates_are_kept(self):
+concat_map = ['c:@F@fun1#I# ast/fun1.c.ast',
+ 'c:@F@fun2#I# ast/fun2.c.ast',
+ 'c:@F@fun1#I# ast/fun1.c.ast']
+pairs = sut.create_global_ctu_function_map(concat_map)
+self.assertTrue(('c:@F@fun1#I#', 'ast/fun1.c.ast') in pairs)
+self.assertTrue(('c:@F@fun2#I#', 'ast/fun2.c.ast') in pairs)
+self.assertEqual(2, len(pairs))
+
+def test_space_handled_in_source(self):
+concat_map = ['c:@F@fun1#I# ast/f un.c.ast']
+pairs = sut.create_global_ctu_function_map(concat_map)
+self.assertTrue(('c:@F@fun1#I#', 'ast/f un.c.ast') in pairs)
+self.assertEqual(1, len(pai
[PATCH] D38922: [analyzer] LoopUnrolling: check the bitwidth of the used numbers (pr34943)
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D38922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39048: Dump signed integers in SymIntExpr and IntSymExpr correctly
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D39048 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38921: [analyzer] LoopUnrolling: update the matched assignment operators
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Core/LoopUnrolling.cpp:100
declRefExpr(to(varDecl(VarNodeMatcher)),
binaryOperator(anyOf(hasOperatorName("="), hasOperatorName("+="),
hasOperatorName("/="), hasOperatorName("*="),
Maybe instead of enumerating all the assignment operators here it would be
better to have a matcher that checks `isAssignmentOp` for
CXXOperatorCalls/BinaryOperators? This would be less error prone and more
future proof.
https://reviews.llvm.org/D38921
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38845: [ASTImporter] Support importing UnresolvedMemberExpr, DependentNameType, DependentScopeDeclRefExpr
xazax.hun added inline comments. Comment at: lib/AST/ASTImporter.cpp:5500 + + TemplateArgumentListInfo ToTAInfo; + TemplateArgumentListInfo *ResInfo = nullptr; According to phabricator this code is very similar to a snippet starting from line 4524 and some code bellow. Maybe it would be worth to have a function instead of duplicating? https://reviews.llvm.org/D38845 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38843: [ASTImporter] Support importing CXXPseudoDestructorExpr
xazax.hun added inline comments. Comment at: lib/AST/ASTImporter.cpp:5549 + Expr *BaseE = Importer.Import(E->getBase()); + if (!BaseE) +return nullptr; Does `E->getBase()` guaranteed to return non-null? What happens when this node was constructed using EmptyShell? Shouldn't we check for that somehow? When can that happen? https://reviews.llvm.org/D38843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38694: [ASTImporter] Support importing CXXUnresolvedConstructExpr and UnresolvedLookupExpr
xazax.hun added inline comments.
Comment at: lib/AST/ASTImporter.cpp:5476
+
+ for (unsigned ai = 0, ae = NumArgs; ai != ae; ++ai) {
+Expr *FromArg = CE->getArg(ai);
Use uppercase variable names.
Comment at: lib/AST/ASTImporter.cpp:5477
+ for (unsigned ai = 0, ae = NumArgs; ai != ae; ++ai) {
+Expr *FromArg = CE->getArg(ai);
+Expr *ToArg = Importer.Import(FromArg);
I would eliminate this local variable.
https://reviews.llvm.org/D38694
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38171: Implement clang-tidy check aliases.
xazax.hun added a comment. One problem to think about when we add all clang-diagnostic as "first or second" class citizen, `checkes=*` might now enable all the warnings which make no sense and might be surprising to the users. What do you think? https://reviews.llvm.org/D38171 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39049: [analyzer] Fix wrong calculation of offset in ArrayBoundsV2
xazax.hun added a comment. I checked what happens: The checker would like to solve the following (I inspect the branch when x == 0 ): `((reg_$1) + 1) * 4 <= 0` The `getSimplifiedOffsets` function kicks in and simplifies the expression above to the following: `(reg_$1) <= -1` The analyzer also know that the value of `y` is within `[1,98]`. The source of the problem is that the simplified expression is evaluated after the right-hand side is converted to an unsigned value which will be greater than the max value of `y`. I think we did not regress after omitting some of the computation because the analyzer's default constraint manager handles the case when there is a constant addition/subtraction next to the symbol. So if we lose something with this modification, we could probably observe that using multidimensional arrays. Do you mind writing some tests with multidimensional arrays to check what do we lose if we remove that code? Also, how hard would it be to correct the calculation for unsigned values? Repository: rL LLVM https://reviews.llvm.org/D39049 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37187: [Analyzer] Fix Bug 25609 - Assertion UNREACHABLE: 'Unexpected ProgramPoint' with widen-loops=true
xazax.hun added a comment. LGTM! https://reviews.llvm.org/D37187 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36471: [StaticAnalyzer] Try to calculate arithmetic result when operand has a range of possible values
xazax.hun added a subscriber: NoQ. xazax.hun added a comment. I think this change is very useful but it is also important to get these changes right. I think one of the main reason you did not get review comments yet is that it is not easy to verify that these changes are sound. In general, there are false positives in the analyzer due to limits in the constraint manager (or missing parts in modeling the language). But in general, we try to avoid having false positives due to unsound assumptions (apart from some cases like assuming const methods will not change the fields of a class). While the change you introduced is indeed very useful the soundness probably depends on the details of how promotions, conversions, and other corner cases are handled. In order to introduce a change like this, we need to have those cases covered to ensure that we have the soundness we want and this needs to be verified with test cases. Also once the solution is sound it would be great to measure the performance to ensure that we did not regress too much. I understand that you do not want to work on something that might not get accepted but also with the available information it might be hard to decide whether this is a good approach to the problem or not. But of course, I am just guessing here, @dcoughlin, @zaks.anna, @NoQ might have a different opinion. A bit more technical comment: did you consider using `SValBuilder`'s `evalBinOpNN`? I believe it already handles at least some of the conversions you did not cover here. Repository: rL LLVM https://reviews.llvm.org/D36471 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38845: [ASTImporter] Support importing UnresolvedMemberExpr, DependentNameType, DependentScopeDeclRefExpr
xazax.hun added inline comments. Comment at: lib/AST/ASTImporter.cpp:5500 + + TemplateArgumentListInfo ToTAInfo; + TemplateArgumentListInfo *ResInfo = nullptr; szepet wrote: > xazax.hun wrote: > > According to phabricator this code is very similar to a snippet starting > > from line 4524 and some code bellow. Maybe it would be worth to have a > > function instead of duplicating? > Good point, I would do it in a separate patch and add it as a dependency if > it is OK. Sure, that would be awesome. :) https://reviews.llvm.org/D38845 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37187: [Analyzer] Fix Bug 25609 - Assertion UNREACHABLE: 'Unexpected ProgramPoint' with widen-loops=true
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:651-652
+ } else if (StoreSite->getLocation().getAs()) {
+os << "Reach the max loop limit.";
+os << " Assigning a conjured symbol";
+if (R->canPrintPretty()) {
zaks.anna wrote:
> MTC wrote:
> > NoQ wrote:
> > > This is user-facing text, and users shouldn't know about conjured
> > > symbols, and "max" shouldn't be shortened, and i'm not sure what else.
> > > I'd probably suggest something along the lines of "Contents of <...> are
> > > wiped", but this is still not good enough.
> > >
> > > Also could you add a test that displays this note? I.e. with
> > > `-analyzer-output=text`.
> > Thanks for your review.
> >
> > You are right, whether this information should be displayed to the user is
> > a question worth discussing.
> I am not convinced that we need to print this information to the user. The
> problem here is that it leaks internal implementation details about the
> analyzer. The users should not know about "loop limits" and "invalidation"
> and most of the users would not even know what this means. I can see how this
> is useful to the analyzer developers for debugging the analyzer, but not to
> the end user.
>
While we might not want to expose this to the user this can be really useful to
understand what the analyzer is doing when we debugging a false positive
finding. Maybe it would be worth to introduce a developer mode or verbose mode
for those purposes. What do you think?
https://reviews.llvm.org/D37187
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39121: [clang-tidy] Misplaced Operator in Strlen in Alloc
xazax.hun added inline comments.
Comment at: clang-tidy/misc/MisplacedOperatorInStrlenInAllocCheck.cpp:30
+ Finder->addMatcher(
+ callExpr(callee(functionDecl(hasName("malloc"))),
+ hasArgument(0, anyOf(hasDescendant(BadUse), BadUse)))
Maybe it is worth to have a configurable list of allocation functions?
Maybe it would be worth to support`alloca` as well?
Comment at: clang-tidy/misc/MisplacedOperatorInStrlenInAllocCheck.cpp:44
+const MatchFinder::MatchResult &Result) {
+ // FIXME: Add callback implementation.
+ const auto *Alloc = Result.Nodes.getNodeAs("Alloc");
What is this fixme?
Comment at: clang-tidy/misc/MisplacedOperatorInStrlenInAllocCheck.h:19
+
+/// FIXME: Write a short description.
+///
There is a missing description.
Comment at:
docs/clang-tidy/checks/misc-misplaced-operator-in-strlen-in-alloc.rst:16
+void bad_malloc(char *str) {
+ char *c = (char*) malloc(strlen(str + 1));
+}
What if this code is intentional for some reason?
I think in thase case it could be rewritten like the following (which is much
cleaner):
```
char *c = (char*) malloc(strlen(str)-1);
```
I think it might be a good idea to mention this possibility as a way to
suppress this warning in the documentation.
https://reviews.llvm.org/D39121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33537: [clang-tidy] Exception Escape Checker
xazax.hun added a comment. I agree that we should not spend too much effort on making warnings from the compiler and tidy disjunct. https://reviews.llvm.org/D33537 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26105: Allow CaseStmt to be initialized with a SubStmt
xazax.hun abandoned this revision. xazax.hun added a comment. Since r316069 this is no longer relevant. https://reviews.llvm.org/D26105 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37187: [Analyzer] Fix Bug 25609 - Assertion UNREACHABLE: 'Unexpected ProgramPoint' with widen-loops=true
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:651-652
+ } else if (StoreSite->getLocation().getAs()) {
+os << "Reach the max loop limit.";
+os << " Assigning a conjured symbol";
+if (R->canPrintPretty()) {
zaks.anna wrote:
> xazax.hun wrote:
> > zaks.anna wrote:
> > > MTC wrote:
> > > > NoQ wrote:
> > > > > This is user-facing text, and users shouldn't know about conjured
> > > > > symbols, and "max" shouldn't be shortened, and i'm not sure what
> > > > > else. I'd probably suggest something along the lines of "Contents of
> > > > > <...> are wiped", but this is still not good enough.
> > > > >
> > > > > Also could you add a test that displays this note? I.e. with
> > > > > `-analyzer-output=text`.
> > > > Thanks for your review.
> > > >
> > > > You are right, whether this information should be displayed to the user
> > > > is a question worth discussing.
> > > I am not convinced that we need to print this information to the user.
> > > The problem here is that it leaks internal implementation details about
> > > the analyzer. The users should not know about "loop limits" and
> > > "invalidation" and most of the users would not even know what this means.
> > > I can see how this is useful to the analyzer developers for debugging the
> > > analyzer, but not to the end user.
> > >
> > While we might not want to expose this to the user this can be really
> > useful to understand what the analyzer is doing when we debugging a false
> > positive finding. Maybe it would be worth to introduce a developer mode or
> > verbose mode for those purposes. What do you think?
> I'd be fine with that in theory, though the downside is that it would pollute
> the code a bit. One trick that's often used to better understand a report
> when debugging is to remove the path note pruning (by passing a flag). I am
> not sure if that approach can be extended for this case. Ex: maybe we could
> have special markers on the notes saying that they are for debug purposes
> only and have the pruning remove them.
>
> By the way, is this change related to the other change from this patch?
I agree that we should just commit this without the message to fix the assert.
The "debug message" could be addressed in a separate patch.
https://reviews.llvm.org/D37187
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37470: [analyzer] Handle ObjC messages conservatively in CallDescription
xazax.hun updated this revision to Diff 120233.
xazax.hun added a comment.
Herald added a subscriber: szepet.
- Modify a test to trigger the assertion fail before the patch is applied.
https://reviews.llvm.org/D37470
Files:
lib/StaticAnalyzer/Core/CallEvent.cpp
test/Analysis/objc-message.m
Index: test/Analysis/objc-message.m
===
--- test/Analysis/objc-message.m
+++ test/Analysis/objc-message.m
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection
-analyzer-store=region -verify -Wno-objc-root-class %s
+// RUN: %clang_analyze_cc1
-analyzer-checker=core,alpha.unix.BlockInCriticalSection,debug.ExprInspection
-analyzer-store=region -verify -Wno-objc-root-class %s
extern void clang_analyzer_warnIfReached();
void clang_analyzer_eval(int);
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -211,7 +211,9 @@
}
bool CallEvent::isCalled(const CallDescription &CD) const {
- assert(getKind() != CE_ObjCMessage && "Obj-C methods are not supported");
+ // FIXME: Add ObjC Message support.
+ if (getKind() == CE_ObjCMessage)
+return false;
if (!CD.IsLookupDone) {
CD.IsLookupDone = true;
CD.II =
&getState()->getStateManager().getContext().Idents.get(CD.FuncName);
Index: test/Analysis/objc-message.m
===
--- test/Analysis/objc-message.m
+++ test/Analysis/objc-message.m
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-store=region -verify -Wno-objc-root-class %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.unix.BlockInCriticalSection,debug.ExprInspection -analyzer-store=region -verify -Wno-objc-root-class %s
extern void clang_analyzer_warnIfReached();
void clang_analyzer_eval(int);
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -211,7 +211,9 @@
}
bool CallEvent::isCalled(const CallDescription &CD) const {
- assert(getKind() != CE_ObjCMessage && "Obj-C methods are not supported");
+ // FIXME: Add ObjC Message support.
+ if (getKind() == CE_ObjCMessage)
+return false;
if (!CD.IsLookupDone) {
CD.IsLookupDone = true;
CD.II = &getState()->getStateManager().getContext().Idents.get(CD.FuncName);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38688: [clang-tidy] Misc redundant expressions checker updated for macros
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM! But wait for @aaron.ballman, @alexfh, or @hokein for the final word. https://reviews.llvm.org/D38688 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49568: [analyzer][WIP] Scan the program state map in the visitor only once in DanglingInternalBufferChecker
xazax.hun added a comment. Yeah, I would rather have the cleanups and do extra work in the visitor. But lets wait what @NoQ thinks. Repository: rC Clang https://reviews.llvm.org/D49568 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
xazax.hun requested changes to this revision.
xazax.hun added a comment.
This revision now requires changes to proceed.
Some comments, mostly nits inline.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:149
+C.addTransition(State);
return;
+ }
Nit: This return is redundant.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:202
+ markPtrSymbolsReleased(Call, State, ObjRegion, C);
}
}
Nit: no need for braces here.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:204
}
+ return;
+}
Nit: redundant return.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:212
+ // Check [string.require] / first point.
+ if (const auto *FC = dyn_cast(&Call)) {
+const FunctionDecl *FD = FC->getDecl();
Shouldn't we also check if the function is a standard library function? Or do
we assume that user functions also invalidate the strings?
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:213
+ if (const auto *FC = dyn_cast(&Call)) {
+const FunctionDecl *FD = FC->getDecl();
+for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
I am not sure if we always have a `Decl` here, I am afraid this might return
null sometimes. Please add a test case with a function pointer (received as an
argument in a top level function).
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:227
}
+ return;
}
Nit: redundant return.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2934
+} else if (const auto *CallE = dyn_cast(S)) {
+ OS << CallE->getDirectCallee()->getNameAsString();
}
I think `getDirectCallee` might fail and return `nullptr`. One more reason to
test function pointers :)
Repository:
rC Clang
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:207
- if (mayInvalidateBuffer(Call)) {
-if (const PtrSet *PS = State->get(ObjRegion)) {
- // Mark all pointer symbols associated with the deleted object released.
- const Expr *Origin = Call.getOriginExpr();
- for (const auto Symbol : *PS) {
-// NOTE: `Origin` may be null, and will be stored so in the symbol's
-// `RefState` in MallocChecker's `RegionState` program state map.
-State = allocation_state::markReleased(State, Symbol, Origin);
- }
- State = State->remove(ObjRegion);
- C.addTransition(State);
- return;
+void InnerPointerChecker::checkPreCall(const CallEvent &Call,
+ CheckerContext &C) const {
NoQ wrote:
> I believe that this should also go into `PostCall`. Symbols aren't released
> until some point //within// the call.
Oh, I see. But is it guaranteed that the symbols are not garbage collected
until post call? Also, the environment will always contain all the bindings for
the arguments?
Repository:
rC Clang
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
xazax.hun added a comment.
Small comments inline.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:181
- auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl();
- if (TypeDecl->getName() != "basic_string")
-return;
+for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
+ QualType ParamTy = FD->getParamDecl(I)->getType();
Nit: maybe using `FunctionDecl::parameters` and range based for loop is sligtly
cleaner.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:189
+ // argument but not as a parameter.
+ const auto *MemberOpCall = dyn_cast(FC);
+ unsigned ArgI = MemberOpCall ? I+1 : I;
Nit: maybe using isa + bool is cleaner here?
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:192
- if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
-SVal RawPtr = Call.getReturnValue();
-if (SymbolRef Sym = RawPtr.getAsSymbol(/*IncludeBaseRegions=*/true)) {
- // Start tracking this raw pointer by adding it to the set of symbols
- // associated with this container object in the program state map.
- PtrSet::Factory &F = State->getStateManager().get_context();
- const PtrSet *SetPtr = State->get(ObjRegion);
- PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet();
- assert(C.wasInlined || !Set.contains(Sym));
- Set = F.add(Set, Sym);
- State = State->set(ObjRegion, Set);
- C.addTransition(State);
+ SVal Arg = FC->getArgSVal(ArgI);
+ const auto *ArgRegion =
While I cannot recall examples in the STL the number of arguments and
parameters might differ. We might have ellipsis in the parameters and this way
we may pass more arguments. Since we do not have the parameter type for this
case, I think it is ok to not handle it. But it might be worth to have a
comment. The other case, when we have default arguments. I do not really know
how the analyzer behaves with default arguments but maybe it would be worth to
add a test case to ensure we do not crash on that?
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:181
- auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl();
- if (TypeDecl->getName() != "basic_string")
-return;
+for (unsigned I = 0, E = FD->getNumParams(); I != E; ++I) {
+ QualType ParamTy = FD->getParamDecl(I)->getType();
rnkovacs wrote:
> xazax.hun wrote:
> > Nit: maybe using `FunctionDecl::parameters` and range based for loop is
> > sligtly cleaner.
> Given that I am manipulating indices below, I feel that actually the plain
> for loop is a bit simpler here, what do you think?
Indeed, I am fine with the old style loop too.
https://reviews.llvm.org/D49656
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49656: [analyzer] Add support for more pointer invalidating functions in InnerPointerChecker
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D49656 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49811: [analyzer] Obtain a ReturnStmt from a CFGAutomaticObjDtor
xazax.hun accepted this revision. xazax.hun added a comment. We could also print something about the ReturnStmt, like source location or pretty print of its expressions so we can check that we picked the right one in case we have multiple. But consider this as an optional task if you have nothing better to do. I am ok with committing this as is. https://reviews.llvm.org/D49811 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D5767: Template Instantiation Observer + a few other templight-related changes
xazax.hun added a comment.
Overall looks good. Was this tested on large software? I would also be grateful
if you could run the regression tests with templight always being enabled to
see if they uncover any assertions/crashes.
Comment at: include/clang/Driver/CC1Options.td:537
+def templight_dump : Flag<["-"], "templight-dump">,
+ HelpText<"Dump templight information to stdout">;
def ast_dump_lookups : Flag<["-"], "ast-dump-lookups">,
Do we want to keep the templight name? I am ok with it, but I wonder if
something like dump template instantiation information, or dump template
profile information would be more descriptive to the newcomers.
Comment at: include/clang/Sema/Sema.h:7117
+
+ /// Added for Template instantiation observation
+ /// Memoization means we are _not_ instantiating a template because
Terminate the first sentence with period.
Comment at: include/clang/Sema/TemplateInstCallback.h:26
+public:
+ virtual ~TemplateInstantiationCallback() {}
+
Prefer `= default;`
Comment at: lib/Frontend/FrontendActions.cpp:29
+#include "llvm/Support/YAMLTraits.h"
+#include
#include
Is this header used? If not (after solving my other comments below), please
remove it.
Comment at: lib/Frontend/FrontendActions.cpp:319
+ const CodeSynthesisContext &Inst) override {
+DisplayTemplightEntry(std::cout, TheSema, Inst);
+ }
`std::cout` is rarely used in LLVM. Did you consider `llvm::outs`?
Also, do we want to output to the standard output or standard error? Is there
other dump functionality that is being printed to the standard output?
Comment at: lib/Frontend/FrontendActions.cpp:357
+ template
+ static void DisplayTemplightEntry(std::ostream &Out, const Sema &TheSema,
+const CodeSynthesisContext &Inst) {
Some methods still starts with uppercase letters.
Comment at: lib/Frontend/FrontendActions.cpp:377
+Entry.Event = BeginInstantiation ? "Begin" : "End";
+if (NamedDecl *NamedTemplate = dyn_cast_or_null(Inst.Entity)) {
+ llvm::raw_string_ostream OS(Entry.Name);
Use `auto *` to avoid repeating the type name.
Comment at: lib/FrontendTool/ExecuteCompilerInvocation.cpp:261
+
+}
Add namespace closing comment.
Comment at: lib/Sema/Sema.cpp:40
#include "clang/Sema/TemplateDeduction.h"
+#include "clang/Sema/TemplateInstCallback.h"
#include "llvm/ADT/DenseMap.h"
Do you need to add this include?
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:204
+
+ case Memoization:
+break;
This function should never be called with `Memoization`? Maybe this worth a
comment?
https://reviews.llvm.org/D5767
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D5767: Template Instantiation Observer + a few other templight-related changes
xazax.hun added a comment. In https://reviews.llvm.org/D5767#999143, @sabel83 wrote: > 2. What do you mean by regression tests? We have run the clang-test target > successfully on the patched code (which has the hook). Note that the hook > this pull request provides is implemented as a ProgramAction. It is not > intended to be used together with other program actions, I don't see how we > could turn it on for other tests (if that is what you referred to). I would bet the sema tests are full of tricky edge cases. So running templight on those tests would be a helpful exercise. I am only interested in assertion fails, so we do not need to check the output. One option to do so would be to add the -templight-dump option to the %clang_cc1 variable when running the tests. Note that the tests are likely to fail because the output will change, but if there are no crashes, it is fine. https://reviews.llvm.org/D5767 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D5767: Template Instantiation Observer + a few other templight-related changes
xazax.hun added inline comments. Comment at: lib/FrontendTool/ExecuteCompilerInvocation.cpp:261 + +} //namespace Nit: this should be `// namespace clang` https://reviews.llvm.org/D5767 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D5767: Template Instantiation Observer + a few other templight-related changes
xazax.hun added inline comments. Comment at: lib/Sema/SemaTemplateInstantiate.cpp:646 } +} + Use either `LLVM_FALLTHROUGH;` here or break to avoid compiler warnings. https://reviews.llvm.org/D5767 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43012: [ASTImporter] Fix lexical DC for templated decls; support VarTemplatePartialSpecDecl
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. Herald added a subscriber: martong. Looks good to me. Only found a few nits. Comment at: lib/AST/ASTImporter.cpp:4296 // Create the declaration that is being templated. - SourceLocation StartLoc = Importer.Import(DTemplated->getLocStart()); - SourceLocation IdLoc = Importer.Import(DTemplated->getLocation()); - TypeSourceInfo *TInfo = Importer.Import(DTemplated->getTypeSourceInfo()); - VarDecl *D2Templated = VarDecl::Create(Importer.getToContext(), DC, StartLoc, - IdLoc, Name.getAsIdentifierInfo(), T, - TInfo, DTemplated->getStorageClass()); - D2Templated->setAccess(DTemplated->getAccess()); - D2Templated->setQualifierInfo(Importer.Import(DTemplated->getQualifierLoc())); - D2Templated->setLexicalDeclContext(LexicalDC); - - // Importer.Imported(DTemplated, D2Templated); - // LexicalDC->addDeclInternal(D2Templated); - - // Merge the initializer. - if (ImportDefinition(DTemplated, D2Templated)) + VarDecl *ToTemplated = dyn_cast_or_null(Importer.Import(DTemplated)); + if (!ToTemplated) `auto *` to not repeat type. Comment at: lib/AST/ASTImporter.cpp:4399 +if (ImportTemplateArgumentListInfo(D->getTemplateArgsInfo(), + ToTAInfo)) + return nullptr; The formatting might be a bit off here. Repository: rC Clang https://reviews.llvm.org/D43012 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D5767: Template Instantiation Observer + a few other templight-related changes
xazax.hun accepted this revision. xazax.hun added a comment. Thanks, this looks good to me! I will try this out soon and commit after that. https://reviews.llvm.org/D5767 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38171: [clang-tidy] Implement clang-tidy check aliases
xazax.hun added subscribers: dkrupp, whisperity. xazax.hun added a comment. @alexfh did you have any chance to think about this change? https://reviews.llvm.org/D38171 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis
xazax.hun added a comment. In https://reviews.llvm.org/D30691#1003514, @george.karpenkov wrote: > Python code looks OK to me, I have one last request: could we have a small > documentation how the whole thing is supposed work in integration, preferably > on an available open-source project any reader could check out? > I am asking because I have actually tried and failed to launch this CTU > patch on a project I was analyzing. We added the documentation. Could you recheck? Thanks in advance! Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:395 +return XFE && !YFE; + return XFE->getName() < YFE->getName(); +} nikhgupt wrote: > getName could yield incorrect results if two files in the project have the > same name. This might break the assert for PathDiagnostics 'total ordering' > and 'uniqueness'. > Maybe replacing FileEntry's getName with FullSourceLoc's getFileID could > resolve this. Thank you, this is a known problem that we plan to address in a follow-up patch. https://reviews.llvm.org/D30691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis
xazax.hun updated this revision to Diff 134431.
xazax.hun added a comment.
- Rebased to current ToT
- Fixed a problem that the scan-build-py used an old version of the ctu
configuration option
- Added a short guide how to use CTU
https://reviews.llvm.org/D30691
Files:
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h
lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
lib/StaticAnalyzer/Core/CMakeLists.txt
lib/StaticAnalyzer/Core/CallEvent.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/PathDiagnostic.cpp
lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
lib/StaticAnalyzer/Frontend/CMakeLists.txt
test/Analysis/Inputs/ctu-chain.cpp
test/Analysis/Inputs/ctu-other.cpp
test/Analysis/Inputs/externalFnMap.txt
test/Analysis/ctu-main.cpp
tools/scan-build-py/README.md
tools/scan-build-py/libscanbuild/__init__.py
tools/scan-build-py/libscanbuild/analyze.py
tools/scan-build-py/libscanbuild/arguments.py
tools/scan-build-py/libscanbuild/clang.py
tools/scan-build-py/libscanbuild/report.py
tools/scan-build-py/tests/unit/test_analyze.py
tools/scan-build-py/tests/unit/test_clang.py
Index: tools/scan-build-py/tests/unit/test_clang.py
===
--- tools/scan-build-py/tests/unit/test_clang.py
+++ tools/scan-build-py/tests/unit/test_clang.py
@@ -92,3 +92,15 @@
self.assertEqual('Checker One description', result.get('checker.one'))
self.assertTrue('checker.two' in result)
self.assertEqual('Checker Two description', result.get('checker.two'))
+
+
+class ClangIsCtuCapableTest(unittest.TestCase):
+def test_ctu_not_found(self):
+is_ctu = sut.is_ctu_capable('not-found-clang-func-mapping')
+self.assertFalse(is_ctu)
+
+
+class ClangGetTripleArchTest(unittest.TestCase):
+def test_arch_is_not_empty(self):
+arch = sut.get_triple_arch(['clang', '-E', '-'], '.')
+self.assertTrue(len(arch) > 0)
Index: tools/scan-build-py/tests/unit/test_analyze.py
===
--- tools/scan-build-py/tests/unit/test_analyze.py
+++ tools/scan-build-py/tests/unit/test_analyze.py
@@ -4,12 +4,12 @@
# This file is distributed under the University of Illinois Open Source
# License. See LICENSE.TXT for details.
-import libear
-import libscanbuild.analyze as sut
import unittest
import re
import os
import os.path
+import libear
+import libscanbuild.analyze as sut
class ReportDirectoryTest(unittest.TestCase):
@@ -333,3 +333,83 @@
def test_method_exception_not_caught(self):
self.assertRaises(Exception, method_exception_from_inside, dict())
+
+
+class PrefixWithTest(unittest.TestCase):
+
+def test_gives_empty_on_empty(self):
+res = sut.prefix_with(0, [])
+self.assertFalse(res)
+
+def test_interleaves_prefix(self):
+res = sut.prefix_with(0, [1, 2, 3])
+self.assertListEqual([0, 1, 0, 2, 0, 3], res)
+
+
+class MergeCtuMapTest(unittest.TestCase):
+
+def test_no_map_gives_empty(self):
+pairs = sut.create_global_ctu_function_map([])
+self.assertFalse(pairs)
+
+def test_multiple_maps_merged(self):
+concat_map = ['c:@F@fun1#I# ast/fun1.c.ast',
+ 'c:@F@fun2#I# ast/fun2.c.ast',
+ 'c:@F@fun3#I# ast/fun3.c.ast']
+pairs = sut.create_global_ctu_function_map(concat_map)
+self.assertTrue(('c:@F@fun1#I#', 'ast/fun1.c.ast') in pairs)
+self.assertTrue(('c:@F@fun2#I#', 'ast/fun2.c.ast') in pairs)
+self.assertTrue(('c:@F@fun3#I#', 'ast/fun3.c.ast') in pairs)
+self.assertEqual(3, len(pairs))
+
+def test_not_unique_func_left_out(self):
+concat_map = ['c:@F@fun1#I# ast/fun1.c.ast',
+ 'c:@F@fun2#I# ast/fun2.c.ast',
+ 'c:@F@fun1#I# ast/fun7.c.ast']
+pairs = sut.create_global_ctu_function_map(concat_map)
+self.assertFalse(('c:@F@fun1#I#', 'ast/fun1.c.ast') in pairs)
+self.assertFalse(('c:@F@fun1#I#', 'ast/fun7.c.ast') in pairs)
+self.assertTrue(('c:@F@fun2#I#', 'ast/fun2.c.ast') in pairs)
+self.assertEqual(1, len(pairs))
+
+def test_duplicates_are_kept(self):
+concat_map = ['c:@F@fun1#I# ast/fun1.c.ast',
+ 'c:@F@fun2#I# ast/fun2.c.ast',
+ 'c:@F@fun1#I# ast/fun1.c.ast']
+pairs = sut.create_global_ctu_function_map(concat_map)
+self.assertTrue(('c:@F@fun1#I#', 'ast/fun1.c.ast') in pairs)
+self.assertTrue(('c:@F@fun2#I#', 'ast/fun2.c.ast') in pairs)
+self.assertEqual(2, len(pairs))
+
+def test_space_handled_in_source(self):
+concat_map = ['c:@F@fun1#I# ast/f un.c.ast']
+pairs = sut.create_global_ctu_function_map(concat_map)
+self.assertTrue(('c:@F@fun1#I#', 'a
[PATCH] D30489: [analyzer] catch out of bounds for VLA
xazax.hun added a comment.
In https://reviews.llvm.org/D30489#769916, @NoQ wrote:
> An idea. I believe the safest way to find the bugs you mentioned would be to
> replace extent-as-a-symbol with extent-as-a-trait.
>
> I.e., currently we construct `extent_$3{SymRegion{conj_$1{char *}}}`, assume
> that it is equal to `reg_$0` (which produces a `SymSymExpr`) and then
> catch more `SymSymExpr`s along the path and compare them to the first one.
>
> Instead, i believe that from the start we should have done something like
>
> REGISTER_MAP_WITH_PROGRAMSTATE(RegionExtent, const SubRegion *, NonLoc);
>
>
> Then when the VLA is constructed (or memory is malloc'ed or something like
> that), we just set the `RegionExtent` trait directly to `reg_$0` and
> later easily compare it to anything. The region's `getExtent()` method would
> be modified to consult this trait instead of (or, at least, before)
> constructing a new symbol.
>
> Ideologically it is the same thing. Technically it produces simpler symbolic
> expressions, and i believe that both RangeConstraintManager and Z3 would
> benefit from simpler symbolic expressions.
+1, I like this approach!
Repository:
rL LLVM
https://reviews.llvm.org/D30489
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis
xazax.hun updated this revision to Diff 101695.
xazax.hun edited the summary of this revision.
xazax.hun added a comment.
- Migrate to use USR instead of mangled names. Name mangling related changes
are reverted.
- Better error handling in some cases.
- File paths containing spaces are now handled correctly.
- Fixes to support scripts.
https://reviews.llvm.org/D30691
Files:
include/clang/AST/ASTContext.h
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/AST/ASTContext.cpp
lib/AST/ASTImporter.cpp
lib/Basic/SourceManager.cpp
lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
lib/StaticAnalyzer/Core/CMakeLists.txt
lib/StaticAnalyzer/Core/CallEvent.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
test/Analysis/Inputs/ctu-chain.cpp
test/Analysis/Inputs/ctu-other.cpp
test/Analysis/Inputs/externalFnMap.txt
test/Analysis/ctu-main.cpp
test/Analysis/func-mapping-test.cpp
test/CMakeLists.txt
test/lit.cfg
tools/CMakeLists.txt
tools/clang-func-mapping/CMakeLists.txt
tools/clang-func-mapping/ClangFnMapGen.cpp
tools/ctu-analysis/ctu-analyze.py
tools/ctu-analysis/ctu-build.py
tools/scan-build-py/libscanbuild/analyze.py
Index: tools/scan-build-py/libscanbuild/analyze.py
===
--- tools/scan-build-py/libscanbuild/analyze.py
+++ tools/scan-build-py/libscanbuild/analyze.py
@@ -383,7 +383,8 @@
def target():
""" Creates output file name for reports. """
-if opts['output_format'] in {'plist', 'plist-html'}:
+if opts['output_format'] in {'plist', 'plist-html',
+ 'plist-multi-file'}:
(handle, name) = tempfile.mkstemp(prefix='report-',
suffix='.plist',
dir=opts['output_dir'])
Index: tools/ctu-analysis/ctu-build.py
===
--- /dev/null
+++ tools/ctu-analysis/ctu-build.py
@@ -0,0 +1,243 @@
+#!/usr/bin/env python
+
+import argparse
+import json
+import glob
+import logging
+import multiprocessing
+import os
+import re
+import signal
+import subprocess
+import shlex
+import shutil
+import tempfile
+
+SOURCE_PATTERN = re.compile('.*\.(C|c|cc|cpp|cxx|ii|m|mm)$', re.IGNORECASE)
+TIMEOUT = 86400
+EXTERNAL_FUNCTION_MAP_FILENAME = 'externalFnMap.txt'
+TEMP_EXTERNAL_FNMAP_FOLDER = 'tmpExternalFnMaps'
+
+
+def get_args():
+parser = argparse.ArgumentParser(
+description='Executes 1st pass of CTU analysis where we preprocess '
+'all files in the compilation database and generate '
+'AST dumps and other necessary information from those '
+'to be used later by the 2nd pass of '
+'Cross Translation Unit analysis',
+formatter_class=argparse.ArgumentDefaultsHelpFormatter)
+parser.add_argument('-b', required=True, dest='buildlog',
+metavar='build.json',
+help='JSON Compilation Database to be used')
+parser.add_argument('-p', metavar='preanalyze-dir', dest='ctuindir',
+help='Target directory for preanalyzation data',
+default='.ctu')
+parser.add_argument('-j', metavar='threads', dest='threads', type=int,
+help='Number of threads to be used',
+default=int(multiprocessing.cpu_count() * 1.0))
+parser.add_argument('-v', dest='verbose', action='store_true',
+help='Verbose output')
+parser.add_argument('--clang-path', metavar='clang-path',
+dest='clang_path',
+help='Set path to directory of clang binaries used '
+ '(default taken from CLANG_PATH envvar)',
+default=os.environ.get('CLANG_PATH'))
+mainargs = parser.parse_args()
+
+if mainargs.verbose:
+logging.getLogger().setLevel(logging.INFO)
+
+if mainargs.clang_path is None:
+clang_path = ''
+else:
+clang_path = os.path.abspath(mainargs.clang_path)
+logging.info('CTU uses clang dir: ' +
+ (clang_path if clang_path != '' else ''))
+
+return mainargs, clang_path
+
+
+def process_buildlog(buildlog_filename, src_2_dir, src_2_cmd, src_order,
+ cmd_2_src, cmd_order):
+with open(buildlog_filename, 'r') as buildlog_file:
+buildlog = json.load(buildlog_file)
+for step in buildlog:
+if SOURCE_PATTERN.match(step['file']):
+if step['file'] not in src_2_dir:
+src_2_dir[step['file']] = step['directory']
+src_2_cmd[step['file']] = step['command']
+src_order.append(step[
[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis
xazax.hun marked 2 inline comments as done. xazax.hun added a comment. In https://reviews.llvm.org/D30691#731617, @zaks.anna wrote: > I agree that scan-build or scan-build-py integration is an important issue to > resolve here. What I envision is that users will just call scan-build and > pass -whole-project as an option to it. Everything else will happen > automagically:) We contacted Laszlo and we have a pull request into scan-build that is under review. He is very helpful and supports the idea of scan-build-py supporting CTU analysis. > I do not quite understand why AST serialization is needed at all. Can we > instead recompile the translation units on demand into a separate ASTContext > and then ASTImport? We did a prototype implementation of on-demand reparsing. On the C projects we tested, the runtime is increased by 10-30% compared to dumping the ASTs. Note that, it is relatively fast to parse C, I would expect a much bigger delta in case of C++ projects. Unfortunately, we weren't able to test that setting due to the ASTImporter limitations. Comment at: include/clang/AST/Mangle.h:59 + // the static analyzer. + bool ShouldForceMangleProto; xazax.hun wrote: > dcoughlin wrote: > > I'm pretty worried about using C++ mangling for C functions. It doesn't > > ever seem appropriate to do so and it sounds like it is papering over > > problems with the design. > > > > Some questions: > > - How do you handle when two translation units have different C functions > > with the same type signatures? Is there a collision? This can arise because > > of two-level namespacing or when building multiple targets with the same > > CTU directory. > > - How do you handle when a C function has the same signature as a C++ > > function. Is there a collision when you mangle the C function? > I agree that using C++ mangling for C+ is not the nicest solution, and I had > to circumvent a problem in the name mangler for C prototypes. > > In case a mangled name is found in multiple source files, it will not be > imported. This is the way how collisions handled regardless of being C or C++ > functions. > The target arch is appended to the mangled name to support the cross > compilation scenario. Currently we do not add the full triple, but this could > be done. > > An alternative solution would be to use USRs instead of mangled names but we > did not explore this option in depth yet. Note that the newest version of this patch does not use name mangling, it uses USRs instead. This turned out to be a perfectly viable alternative and we did not see any behavioral changes on the project we tested after the transition. https://reviews.llvm.org/D30691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34277: [analyzer] Bump default performance thresholds?
xazax.hun added a comment. While I have no objections, I am wondering whether this is the good way to spend the performance budget. In particular, there are patches to generate more symbolic expressions, that could also degrade the performance (but fix some fixmes along the way). https://reviews.llvm.org/D34277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34275: [analyzer] Re-implemente current virtual calls checker in a path-sensitive way
xazax.hun added a comment.
I do not see any test cases for this patch. Could you add them as well?
Are you sure that this representation is ok?
How do you handle the following case?
struct A {
A() {
X x;
x.virtualMethod(); // this virtual call is ok
}
}
int main() {
A a;
}
Comment at: VirtualCallChecker.cpp:44
+ private:
+const unsigned Flag;
+bool Found;
The name of this flag is not descriptive enough. Please choose a name that
refers to what are you using this value for.
Comment at: VirtualCallChecker.cpp:79
+ ProgramStateRef state = N->getState();
+ const unsigned ctorflag = state->get();
+ const unsigned dtorflag = state->get();
Variable names should start with uppercase letter.
Comment at: VirtualCallChecker.cpp:83
+ const CXXConstructorDecl *CD =
+dyn_cast(LCtx->getDecl());
+ const CXXDestructorDecl *DD =
Are you sure that the LCtx->getDecl can not return null?
Comment at: VirtualCallChecker.cpp:114
+ CheckerContext &C) const {
+ const Decl *D = dyn_cast_or_null(Call.getDecl());
+ if (!D)
Do you need this cast here?
Comment at: VirtualCallChecker.cpp:119
+ ProgramStateRef state = C.getState();
+ const unsigned ctorflag = state->get();
+ const unsigned dtorflag = state->get();
Querying the state is not free, I think you should also query something from
the state once you are sure that you will need that.
Comment at: VirtualCallChecker.cpp:158
+ // GDM of constructor and destructor.
+ if (isVirtualCall(CE) && ctorflag > 0 && state->get() == 0) {
+if (!BT_CT) {
I don't think this is the right approach. Once you increased the ObjectFlag on
a path, you will never report anything on that path anymore. I think it might
be better to have a map from Symbols (representing this) to ctor/dtors or some
other approach.
Comment at: VirtualCallChecker.cpp:260
+ if (SFC->inTopFrame()) {
+ const FunctionDecl *FD = SFC->getDecl()->getAsFunction();
+if (!FD)
The formatting seems to be off here I recommend using clang format.
https://reviews.llvm.org/D34275
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34275: [analyzer] Re-implemente current virtual calls checker in a path-sensitive way
xazax.hun added a comment.
Note that when you update the differential revision you need to upload the
whole diff. Your diff now only contains the tests but not the code.
In https://reviews.llvm.org/D34275#785189, @wangxindsb wrote:
> > How do you handle the following case?
> >
> > struct A {
> > A() {
> > X x;
> > x.virtualMethod(); // this virtual call is ok
> > }
> > }
> > int main() {
> > A a;
> > }
>
> I use the checker to check the code above, the checker works as expect and
> doesn't throw the warning.
What about:
struct A {
A() {
X x;
x.virtualMethod(); // this virtual call is ok
foo(); // should warn here
}
virtual foo();
}
int main() {
A a;
}
Does the checker warn on the second call?
Comment at: virtualcall.cpp:1
-// RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall
-analyzer-store region -verify -std=c++11 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall
-analyzer-store region -analyzer-config
optin.cplusplus.VirtualCall:Interprocedural=true -DINTERPROCEDURAL=1 -verify
-std=c++11 %s
Please add the appropriate run lines so you can run the tests using `make
check`.
https://reviews.llvm.org/D34275
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34275: [analyzer] Re-implemente current virtual calls checker in a path-sensitive way
xazax.hun added a comment.
In https://reviews.llvm.org/D34275#785294, @wangxindsb wrote:
> > What about:
> >
> > struct A {
> > A() {
> > X x;
> > x.virtualMethod(); // this virtual call is ok
> > foo(); // should warn here
> > }
> > virtual foo();
> > }
> > int main() {
> > A a;
> > }
> >
> >
> > Does the checker warn on the second call?
>
> Yes, the checker warn on the second call.
Oh, I think I see how it works now. How about:
struct A;
struct X {
void callFooOfA(A*);
};
struct A {
A() {
X x;
x.virtualMethod(); // this virtual call is ok
x.callFooOfA(this)
}
virtual foo();
};
void X::callFooOfA(A* a) {
a->foo(); // Would be good to warn here.
}
int main() {
A a;
}
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:1
-//===- VirtualCallChecker.cpp *- C++
-*-==//
-//
Please add the license back.
https://reviews.llvm.org/D34275
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34449: [Clang-tidy] Enable constexpr definitions in headers.
xazax.hun created this revision.
xazax.hun added a project: clang-tools-extra.
Herald added a subscriber: whisperity.
Constexpr variable definitions should be ok in headers.
https://stackoverflow.com/questions/34445336/constexpr-global-constants-in-a-header-file-and-odr
Repository:
rL LLVM
https://reviews.llvm.org/D34449
Files:
clang-tidy/misc/DefinitionsInHeadersCheck.cpp
test/clang-tidy/misc-definitions-in-headers.hpp
Index: test/clang-tidy/misc-definitions-in-headers.hpp
===
--- test/clang-tidy/misc-definitions-in-headers.hpp
+++ test/clang-tidy/misc-definitions-in-headers.hpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s misc-definitions-in-headers %t
+// RUN: %check_clang_tidy %s misc-definitions-in-headers %t -- -- -std=c++1z
int f() {
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f' defined in a header
file; function definitions in header files can lead to ODR violations
[misc-definitions-in-headers]
@@ -175,3 +175,7 @@
int CD::f() { // OK: partial template specialization.
return 0;
}
+
+class CE {
+ constexpr static int i = 5; // OK: constexpr definition.
+};
Index: clang-tidy/misc/DefinitionsInHeadersCheck.cpp
===
--- clang-tidy/misc/DefinitionsInHeadersCheck.cpp
+++ clang-tidy/misc/DefinitionsInHeadersCheck.cpp
@@ -54,7 +54,7 @@
return;
auto DefinitionMatcher =
anyOf(functionDecl(isDefinition(), unless(isDeleted())),
-varDecl(isDefinition()));
+varDecl(isDefinition(), unless(isConstexpr(;
if (UseHeaderFileExtension) {
Finder->addMatcher(namedDecl(DefinitionMatcher,
usesHeaderFileExtension(HeaderFileExtensions))
Index: test/clang-tidy/misc-definitions-in-headers.hpp
===
--- test/clang-tidy/misc-definitions-in-headers.hpp
+++ test/clang-tidy/misc-definitions-in-headers.hpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s misc-definitions-in-headers %t
+// RUN: %check_clang_tidy %s misc-definitions-in-headers %t -- -- -std=c++1z
int f() {
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f' defined in a header file; function definitions in header files can lead to ODR violations [misc-definitions-in-headers]
@@ -175,3 +175,7 @@
int CD::f() { // OK: partial template specialization.
return 0;
}
+
+class CE {
+ constexpr static int i = 5; // OK: constexpr definition.
+};
Index: clang-tidy/misc/DefinitionsInHeadersCheck.cpp
===
--- clang-tidy/misc/DefinitionsInHeadersCheck.cpp
+++ clang-tidy/misc/DefinitionsInHeadersCheck.cpp
@@ -54,7 +54,7 @@
return;
auto DefinitionMatcher =
anyOf(functionDecl(isDefinition(), unless(isDeleted())),
-varDecl(isDefinition()));
+varDecl(isDefinition(), unless(isConstexpr(;
if (UseHeaderFileExtension) {
Finder->addMatcher(namedDecl(DefinitionMatcher,
usesHeaderFileExtension(HeaderFileExtensions))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34502: [analyzer] Do not continue to analyze a path if the constraints contradict with builtin assume
xazax.hun created this revision.
Herald added a subscriber: whisperity.
This is how asserts are working right now. This way the semantics of
__builtin_assume will be identical to asserts.
I also moved the tests to another file.
Repository:
rL LLVM
https://reviews.llvm.org/D34502
Files:
lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
test/Analysis/builtin-assume.c
test/Analysis/builtin-functions.cpp
Index: test/Analysis/builtin-functions.cpp
===
--- test/Analysis/builtin-functions.cpp
+++ test/Analysis/builtin-functions.cpp
@@ -1,6 +1,7 @@
// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10
-analyzer-checker=core,debug.ExprInspection %s -std=c++11 -verify
void clang_analyzer_eval(bool);
+void clang_analyzer_warnIfReached();
void testAddressof(int x) {
clang_analyzer_eval(&x == __builtin_addressof(x)); //
expected-warning{{TRUE}}
@@ -50,3 +51,16 @@
q = (char*) __builtin_assume_aligned(p + 1, 16);
clang_analyzer_eval(p == q); // expected-warning{{FALSE}}
}
+
+void f(int i) {
+ __builtin_assume(i < 10);
+ clang_analyzer_eval(i < 15); // expected-warning {{TRUE}}
+}
+
+void g(int i) {
+ if (i > 5) {
+__builtin_assume(i < 5);
+clang_analyzer_warnIfReached(); // Assumtion contradicts constraints.
+// We give up the analysis on this path.
+ }
+}
Index: test/Analysis/builtin-assume.c
===
--- test/Analysis/builtin-assume.c
+++ /dev/null
@@ -1,8 +0,0 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify
%s
-
-void clang_analyzer_eval(int);
-
-void f(int i) {
- __builtin_assume(i < 10);
- clang_analyzer_eval(i < 15); // expected-warning {{TRUE}}
-}
Index: lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -50,8 +50,10 @@
state = state->assume(ArgSVal.castAs(), true);
// FIXME: do we want to warn here? Not right now. The most reports might
// come from infeasible paths, thus being false positives.
-if (!state)
+if (!state) {
+ C.generateSink(C.getState(), C.getPredecessor());
return true;
+}
C.addTransition(state);
return true;
Index: test/Analysis/builtin-functions.cpp
===
--- test/Analysis/builtin-functions.cpp
+++ test/Analysis/builtin-functions.cpp
@@ -1,6 +1,7 @@
// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,debug.ExprInspection %s -std=c++11 -verify
void clang_analyzer_eval(bool);
+void clang_analyzer_warnIfReached();
void testAddressof(int x) {
clang_analyzer_eval(&x == __builtin_addressof(x)); // expected-warning{{TRUE}}
@@ -50,3 +51,16 @@
q = (char*) __builtin_assume_aligned(p + 1, 16);
clang_analyzer_eval(p == q); // expected-warning{{FALSE}}
}
+
+void f(int i) {
+ __builtin_assume(i < 10);
+ clang_analyzer_eval(i < 15); // expected-warning {{TRUE}}
+}
+
+void g(int i) {
+ if (i > 5) {
+__builtin_assume(i < 5);
+clang_analyzer_warnIfReached(); // Assumtion contradicts constraints.
+// We give up the analysis on this path.
+ }
+}
Index: test/Analysis/builtin-assume.c
===
--- test/Analysis/builtin-assume.c
+++ /dev/null
@@ -1,8 +0,0 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
-
-void clang_analyzer_eval(int);
-
-void f(int i) {
- __builtin_assume(i < 10);
- clang_analyzer_eval(i < 15); // expected-warning {{TRUE}}
-}
Index: lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -50,8 +50,10 @@
state = state->assume(ArgSVal.castAs(), true);
// FIXME: do we want to warn here? Not right now. The most reports might
// come from infeasible paths, thus being false positives.
-if (!state)
+if (!state) {
+ C.generateSink(C.getState(), C.getPredecessor());
return true;
+}
C.addTransition(state);
return true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34506: Relax an assert in the comparison of source locations
xazax.hun created this revision.
Right now source locations from different translation units can not be
compared.
This is a problem for an upcoming feature in the Static Analyzer, the cross
translation unit support (https://reviews.llvm.org/D30691).
It would be great to be able to sort the source locations, even if there are no
guarantee to have
a meaningful order between source locations from different translation units.
Repository:
rL LLVM
https://reviews.llvm.org/D34506
Files:
lib/Basic/SourceManager.cpp
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -2034,6 +2034,9 @@
}
/// \brief Determines the order of 2 source locations in the translation unit.
+///It also works when two locations are from different translation
+///units. In that case it will return *some* order, that is
+///deterministic for that invocation of the compiler.
///
/// \returns true if LHS source location comes before RHS, false otherwise.
bool SourceManager::isBeforeInTranslationUnit(SourceLocation LHS,
@@ -2130,7 +2133,8 @@
return LIsScratch;
return LOffs.second < ROffs.second;
}
- llvm_unreachable("Unsortable locations found");
+ // Source locations from different translation units.
+ return LOffs.first < ROffs.first;
}
void SourceManager::PrintStats() const {
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -2034,6 +2034,9 @@
}
/// \brief Determines the order of 2 source locations in the translation unit.
+///It also works when two locations are from different translation
+///units. In that case it will return *some* order, that is
+///deterministic for that invocation of the compiler.
///
/// \returns true if LHS source location comes before RHS, false otherwise.
bool SourceManager::isBeforeInTranslationUnit(SourceLocation LHS,
@@ -2130,7 +2133,8 @@
return LIsScratch;
return LOffs.second < ROffs.second;
}
- llvm_unreachable("Unsortable locations found");
+ // Source locations from different translation units.
+ return LOffs.first < ROffs.first;
}
void SourceManager::PrintStats() const {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33841: [clang-tidy] redundant keyword check
xazax.hun added inline comments. Comment at: docs/clang-tidy/checks/readability-redundant-keyword.rst:8 + +`extern` is redundant in function declarations + alexfh wrote: > Could you explain, why you think `extern` is redundant in function > declarations? Just to be clear here, do you think there is a case where extern is not redundant or you just want the documentation to be extended? Repository: rL LLVM https://reviews.llvm.org/D33841 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34506: Relax an assert in the comparison of source locations
xazax.hun added a comment. Note that, it is not easy to add a test case for this patch without the https://reviews.llvm.org/D30691 being accepted. Repository: rL LLVM https://reviews.llvm.org/D34506 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34512: [libTooling] Add preliminary Cross Translation Unit support for libTooling
xazax.hun created this revision.
Herald added a subscriber: mgorny.
This patch introduces a class that can help to build tools that require cross
translation unit facilities.
This class allows function definitions to be loaded from external AST files
based on an index.
In order to use this functionality an index is required. USRs are used as names
to look up the functions.
This class also does caching to avoid redundant loading of AST files.
Right now only function defnitions can be loaded using this API, because this
is what the Static Analyzer requires.
In to future this could be extended to classes, types etc.
Note that, there is no tests right now for this functionality, but this is
temporary.
Tets will come together with the first user once it is accepted:
https://reviews.llvm.org/D30691
Repository:
rL LLVM
https://reviews.llvm.org/D34512
Files:
include/clang/Tooling/CrossTranslationUnit.h
lib/Tooling/CMakeLists.txt
lib/Tooling/CrossTranslationUnit.cpp
Index: lib/Tooling/CrossTranslationUnit.cpp
===
--- /dev/null
+++ lib/Tooling/CrossTranslationUnit.cpp
@@ -0,0 +1,162 @@
+//===--- CrossTranslationUnit.cpp - -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// This file provides an interface to load binary AST dumps on demand. This
+// feature can be utilized for tools that require cross translation unit
+// support.
+//
+//===--===//
+#include "clang/Tooling/CrossTranslationUnit.h"
+#include "clang/AST/ASTImporter.h"
+#include "clang/AST/Decl.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "clang/Index/USRGeneration.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
+#include
+
+namespace clang {
+namespace tooling {
+
+CrossTranslationUnit::CrossTranslationUnit(CompilerInstance &CI)
+: CI(CI), Context(CI.getASTContext()) {}
+
+CrossTranslationUnit::~CrossTranslationUnit() {}
+
+std::string CrossTranslationUnit::getLookupName(const NamedDecl *ND) {
+ SmallString<128> DeclUSR;
+ bool Ret = index::generateUSRForDecl(ND, DeclUSR);
+ assert(!Ret);
+ llvm::raw_svector_ostream OS(DeclUSR);
+ // To support cross compilation.
+ llvm::Triple::ArchType T = Context.getTargetInfo().getTriple().getArch();
+ if (T == llvm::Triple::thumb)
+T = llvm::Triple::arm;
+ OS << "@" << Context.getTargetInfo().getTriple().getArchTypeName(T);
+ return OS.str();
+}
+
+/// Recursively visit the funtion decls of a DeclContext, and looks up a
+/// function based on mangled name.
+const FunctionDecl *
+CrossTranslationUnit::findFunctionInDeclContext(const DeclContext *DC,
+StringRef LookupFnName) {
+ if (!DC)
+return nullptr;
+ for (const Decl *D : DC->decls()) {
+const auto *SubDC = dyn_cast(D);
+if (const auto *FD = findFunctionInDeclContext(SubDC, LookupFnName))
+ return FD;
+
+const auto *ND = dyn_cast(D);
+const FunctionDecl *ResultDecl;
+if (!ND || !ND->hasBody(ResultDecl))
+ continue;
+// We are already sure that the triple is correct here.
+if (getLookupName(ResultDecl) != LookupFnName)
+ continue;
+return ResultDecl;
+ }
+ return nullptr;
+}
+
+const FunctionDecl *
+CrossTranslationUnit::getCTUDefinition(const FunctionDecl *FD, StringRef CTUDir,
+ StringRef IndexName) {
+ assert(!FD->hasBody() && "FD has a definition in current translation unit!");
+
+ std::string LookupFnName = getLookupName(FD);
+ if (LookupFnName.empty())
+return nullptr;
+ ASTUnit *Unit = nullptr;
+ auto FnUnitCacheEntry = FunctionAstUnitMap.find(LookupFnName);
+ if (FnUnitCacheEntry == FunctionAstUnitMap.end()) {
+if (FunctionFileMap.empty()) {
+ SmallString<256> ExternalFunctionMap = CTUDir;
+ llvm::sys::path::append(ExternalFunctionMap, IndexName);
+ std::ifstream ExternalFnMapFile(ExternalFunctionMap.c_str());
+ if (!ExternalFnMapFile) {
+llvm::errs() << "error: '" << ExternalFunctionMap
+ << "' cannot be opened: falling back to non-CTU mode\n";
+return nullptr;
+ }
+
+ std::string FunctionName, FileName;
+ std::string line;
+ while (std::getline(ExternalFnMapFile, line)) {
+size_t pos = line.find(" ");
+FunctionName = line.substr(0, pos);
+FileName = line.substr(pos + 1);
+SmallString<256> FilePath = CTUDir;
+llvm::sys::path::append(FilePath, FileName);
+
[PATCH] D34512: [libTooling] Add preliminary Cross Translation Unit support for libTooling
xazax.hun updated this revision to Diff 103571.
xazax.hun added a comment.
- Add a tool to dump USRs for function definitions. It can be used to create
index files.
https://reviews.llvm.org/D34512
Files:
include/clang/Tooling/CrossTranslationUnit.h
lib/Tooling/CMakeLists.txt
lib/Tooling/CrossTranslationUnit.cpp
tools/clang-func-mapping/CMakeLists.txt
tools/clang-func-mapping/ClangFnMapGen.cpp
Index: tools/clang-func-mapping/ClangFnMapGen.cpp
===
--- /dev/null
+++ tools/clang-func-mapping/ClangFnMapGen.cpp
@@ -0,0 +1,127 @@
+//===- ClangFnMapGen.cpp ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//======//
+//
+// Clang tool which creates a list of defined functions and the files in which
+// they are defined.
+//
+//======//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/GlobalDecl.h"
+#include "clang/AST/Mangle.h"
+#include "clang/AST/StmtVisitor.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Index/USRGeneration.h"
+#include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/Signals.h"
+#include
+#include
+#include
+
+using namespace llvm;
+using namespace clang;
+using namespace clang::tooling;
+
+static cl::OptionCategory ClangFnMapGenCategory("clang-fnmapgen options");
+
+class MapFunctionNamesConsumer : public ASTConsumer {
+public:
+ MapFunctionNamesConsumer(ASTContext &Context) : Ctx(Context) {}
+
+ ~MapFunctionNamesConsumer() {
+// Flush results to standard output.
+llvm::outs() << DefinedFuncsStr.str();
+ }
+
+ virtual void HandleTranslationUnit(ASTContext &Ctx) {
+handleDecl(Ctx.getTranslationUnitDecl());
+ }
+
+private:
+ std::string getLookupName(const FunctionDecl *FD);
+ void handleDecl(const Decl *D);
+
+ ASTContext &Ctx;
+ std::stringstream DefinedFuncsStr;
+ std::string CurrentFileName;
+};
+
+void MapFunctionNamesConsumer::handleDecl(const Decl *D) {
+ if (!D)
+return;
+
+ if (const auto *FD = dyn_cast(D)) {
+if (FD->isThisDeclarationADefinition()) {
+ if (const Stmt *Body = FD->getBody()) {
+SmallString<128> LookupName;
+bool Res = index::generateUSRForDecl(D, LookupName);
+assert(!Res);
+const SourceManager &SM = Ctx.getSourceManager();
+if (CurrentFileName.empty()) {
+ StringRef SMgrName =
+ SM.getFileEntryForID(SM.getMainFileID())->getName();
+ char *Path = realpath(SMgrName.str().c_str(), nullptr);
+ CurrentFileName = Path;
+ free(Path);
+}
+
+switch (FD->getLinkageInternal()) {
+case ExternalLinkage:
+case VisibleNoLinkage:
+case UniqueExternalLinkage:
+ if (SM.isInMainFile(Body->getLocStart()))
+DefinedFuncsStr << LookupName.str().str() << " " << CurrentFileName
+<< "\n";
+default:
+ break;
+}
+ }
+}
+ }
+
+ if (const auto *DC = dyn_cast(D))
+for (const Decl *D : DC->decls())
+ handleDecl(D);
+}
+
+class MapFunctionNamesAction : public ASTFrontendAction {
+protected:
+ std::unique_ptr CreateASTConsumer(CompilerInstance &CI,
+ llvm::StringRef) {
+std::unique_ptr PFC(
+new MapFunctionNamesConsumer(CI.getASTContext()));
+return PFC;
+ }
+};
+
+static cl::extrahelp CommonHelp(CommonOptionsParser::HelpMessage);
+
+int main(int argc, const char **argv) {
+ // Print a stack trace if we signal out.
+ sys::PrintStackTraceOnErrorSignal(argv[0], false);
+ PrettyStackTraceProgram X(argc, argv);
+
+ const char *Overview = "\nThis tool collects the USR name and location "
+ "of all functions definitions in the source files "
+ "(excluding headers).\n";
+ CommonOptionsParser OptionsParser(argc, argv, ClangFnMapGenCategory,
+cl::ZeroOrMore, Overview);
+
+ ClangTool Tool(OptionsParser.getCompilations(),
+ OptionsParser.getSourcePathList());
+ Tool.run(newFrontendActionFactory().get());
+ return 0;
+}
Index: tools/clang-func-mapping/CMakeLists.txt
===
--- /dev/null
+++ tools/clang-func-mapping/CMakeLists.txt
@@ -0,0 +1,21 @@
+set(LLVM_LINK_COMPONENTS
+ ${LLVM_TARGETS_TO_BUILD}
+ asmparser
+ support
+ mc
[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis
xazax.hun added a comment. Some of the CTU related analyzer independent parts are being factored out. The review is ongoing here: https://reviews.llvm.org/D34512 Another small and independent part is under review here: https://reviews.llvm.org/D34506 https://reviews.llvm.org/D30691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34512: [libTooling] Add preliminary Cross Translation Unit support for libTooling
xazax.hun updated this revision to Diff 103707.
xazax.hun marked an inline comment as done.
xazax.hun edited the summary of this revision.
xazax.hun added a comment.
- Added test for the USR dumping tool.
- Added unit test for the CrossTranslationUnit class
- Added documentation for the API entry point
- Renamed some functions and variables
https://reviews.llvm.org/D34512
Files:
include/clang/Tooling/CrossTranslationUnit.h
lib/AST/ASTImporter.cpp
lib/Tooling/CMakeLists.txt
lib/Tooling/CrossTranslationUnit.cpp
test/Analysis/func-mapping-test.cpp
test/CMakeLists.txt
test/lit.cfg
tools/CMakeLists.txt
tools/clang-func-mapping/CMakeLists.txt
tools/clang-func-mapping/ClangFnMapGen.cpp
unittests/Tooling/CMakeLists.txt
unittests/Tooling/CrossTranslationUnitTest.cpp
Index: unittests/Tooling/CrossTranslationUnitTest.cpp
===
--- /dev/null
+++ unittests/Tooling/CrossTranslationUnitTest.cpp
@@ -0,0 +1,95 @@
+//===- unittest/Tooling/CrossTranslationUnitTest.cpp - Tooling unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/CrossTranslationUnit.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include
+
+namespace clang {
+namespace tooling {
+
+namespace {
+StringRef IndexFileName = "index.txt";
+StringRef ASTFileName = "f.ast";
+
+class CTUASTConsumer : public clang::ASTConsumer {
+public:
+ explicit CTUASTConsumer(clang::CompilerInstance &CI, bool *Success)
+ : CTU(CI), Success(Success) {}
+
+ void HandleTranslationUnit(ASTContext &Ctx) {
+const TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl();
+const FunctionDecl *FD = nullptr;
+for (const Decl *D : TU->decls()) {
+ FD = dyn_cast(D);
+ if (FD && FD->getName() == "f")
+break;
+}
+assert(FD);
+bool OrigFDHasBody = FD->hasBody();
+
+// Prepare the index file and the AST file.
+std::error_code EC;
+llvm::raw_fd_ostream OS(IndexFileName, EC, llvm::sys::fs::F_Text);
+OS << "c:@F@f#I# " << ASTFileName << "\n";
+OS.flush();
+StringRef SourceText = "int f(int) { return 0; }\n";
+// This file must exist since the saved ASTFile will reference it.
+llvm::raw_fd_ostream OS2("input.cc", EC, llvm::sys::fs::F_Text);
+OS2 << SourceText;
+OS2.flush();
+std::unique_ptr ASTWithDefinition =
+buildASTFromCode(SourceText);
+ASTWithDefinition->Save(ASTFileName);
+
+// Load the definition from the AST file.
+const FunctionDecl *NewFD =
+CTU.getCrossTUDefinition(FD, ".", IndexFileName);
+
+*Success = NewFD && NewFD->hasBody() && !OrigFDHasBody;
+ }
+
+private:
+ CrossTranslationUnit CTU;
+ bool *Success;
+};
+
+class CTUAction : public clang::ASTFrontendAction {
+public:
+ CTUAction(bool *Success) : Success(Success) {}
+
+protected:
+ std::unique_ptr
+ CreateASTConsumer(clang::CompilerInstance &CI, StringRef) override {
+return llvm::make_unique(CI, Success);
+ }
+
+private:
+ bool *Success;
+};
+
+} // end namespace
+
+TEST(CrossTranslationUnit, CanLoadFunctionDefinition) {
+ bool Success = false;
+ EXPECT_TRUE(runToolOnCode(new CTUAction(&Success), "int f(int);"));
+ EXPECT_TRUE(Success);
+ EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+ EXPECT_FALSE((bool)llvm::sys::fs::remove(IndexFileName));
+ EXPECT_TRUE(llvm::sys::fs::exists(ASTFileName));
+ EXPECT_FALSE((bool)llvm::sys::fs::remove(ASTFileName));
+}
+
+} // end namespace tooling
+} // end namespace clang
Index: unittests/Tooling/CMakeLists.txt
===
--- unittests/Tooling/CMakeLists.txt
+++ unittests/Tooling/CMakeLists.txt
@@ -13,6 +13,7 @@
add_clang_unittest(ToolingTests
CommentHandlerTest.cpp
CompilationDatabaseTest.cpp
+ CrossTranslationUnitTest.cpp
FixItTest.cpp
LookupTest.cpp
QualTypeNamesTest.cpp
Index: tools/clang-func-mapping/ClangFnMapGen.cpp
===
--- /dev/null
+++ tools/clang-func-mapping/ClangFnMapGen.cpp
@@ -0,0 +1,127 @@
+//===- ClangFnMapGen.cpp ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//======//
+//
+// Clang tool which creates a list of defined functions and the files in which
+// they are defined.
+//
+//===---
[PATCH] D34512: [libTooling] Add preliminary Cross Translation Unit support for libTooling
xazax.hun added a comment. Unfortunately, in order to make the Unit Test pass, I also had to modify the AST Importer. Are you ok with having that change as part of this patch (Aleksei might be a suitable person to review that part) or should I make a separate differential for that? https://reviews.llvm.org/D34512 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34512: [libTooling] Add preliminary Cross Translation Unit support for libTooling
xazax.hun added inline comments. Comment at: include/clang/Tooling/CrossTranslationUnit.h:53-58 + /// \p CrossTUDir directory, called \p IndexName. In case the declaration is + /// found in the index the corresponding AST file will be loaded and the + /// definition of the function will be merged into the original AST using + /// the AST Importer. The declaration with the definition will be returned. + /// + /// Note that the AST files should also be in the \p CrossTUDir. klimek wrote: > In the future we'll want to create an index interface around this (which will > probably serve also what the refactoring integration would be based on), > instead of piping files and directories into all classes. > > Perhaps we can start this by already pulling out a class ProjectIndex or > somesuch, with methods like loadASTDefining(...)? > While I do agree to have an interface for that would be really good, but maybe it would be better to first review and accept this patch and after that design the interface in a follow-up patch (so https://reviews.llvm.org/D30691 is not blocked). What do you think? https://reviews.llvm.org/D34512 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34512: [libTooling] Add preliminary Cross Translation Unit support for libTooling
xazax.hun updated this revision to Diff 103710.
xazax.hun added a comment.
- Removed an unrelated change
- Made the unit test more strict
https://reviews.llvm.org/D34512
Files:
include/clang/Tooling/CrossTranslationUnit.h
lib/AST/ASTImporter.cpp
lib/Tooling/CMakeLists.txt
lib/Tooling/CrossTranslationUnit.cpp
test/Analysis/func-mapping-test.cpp
test/CMakeLists.txt
test/lit.cfg
tools/CMakeLists.txt
tools/clang-func-mapping/CMakeLists.txt
tools/clang-func-mapping/ClangFnMapGen.cpp
unittests/Tooling/CMakeLists.txt
unittests/Tooling/CrossTranslationUnitTest.cpp
Index: unittests/Tooling/CrossTranslationUnitTest.cpp
===
--- /dev/null
+++ unittests/Tooling/CrossTranslationUnitTest.cpp
@@ -0,0 +1,98 @@
+//===- unittest/Tooling/CrossTranslationUnitTest.cpp - Tooling unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/CrossTranslationUnit.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include
+
+namespace clang {
+namespace tooling {
+
+namespace {
+StringRef IndexFileName = "index.txt";
+StringRef ASTFileName = "f.ast";
+StringRef DefinitionFileName = "input.cc";
+
+class CTUASTConsumer : public clang::ASTConsumer {
+public:
+ explicit CTUASTConsumer(clang::CompilerInstance &CI, bool *Success)
+ : CTU(CI), Success(Success) {}
+
+ void HandleTranslationUnit(ASTContext &Ctx) {
+const TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl();
+const FunctionDecl *FD = nullptr;
+for (const Decl *D : TU->decls()) {
+ FD = dyn_cast(D);
+ if (FD && FD->getName() == "f")
+break;
+}
+assert(FD);
+bool OrigFDHasBody = FD->hasBody();
+
+// Prepare the index file and the AST file.
+std::error_code EC;
+llvm::raw_fd_ostream OS(IndexFileName, EC, llvm::sys::fs::F_Text);
+OS << "c:@F@f#I# " << ASTFileName << "\n";
+OS.flush();
+StringRef SourceText = "int f(int) { return 0; }\n";
+// This file must exist since the saved ASTFile will reference it.
+llvm::raw_fd_ostream OS2(DefinitionFileName, EC, llvm::sys::fs::F_Text);
+OS2 << SourceText;
+OS2.flush();
+std::unique_ptr ASTWithDefinition =
+buildASTFromCode(SourceText);
+ASTWithDefinition->Save(ASTFileName);
+
+// Load the definition from the AST file.
+const FunctionDecl *NewFD =
+CTU.getCrossTUDefinition(FD, ".", IndexFileName);
+
+*Success = NewFD && NewFD->hasBody() && !OrigFDHasBody;
+ }
+
+private:
+ CrossTranslationUnit CTU;
+ bool *Success;
+};
+
+class CTUAction : public clang::ASTFrontendAction {
+public:
+ CTUAction(bool *Success) : Success(Success) {}
+
+protected:
+ std::unique_ptr
+ CreateASTConsumer(clang::CompilerInstance &CI, StringRef) override {
+return llvm::make_unique(CI, Success);
+ }
+
+private:
+ bool *Success;
+};
+
+} // end namespace
+
+TEST(CrossTranslationUnit, CanLoadFunctionDefinition) {
+ bool Success = false;
+ EXPECT_TRUE(runToolOnCode(new CTUAction(&Success), "int f(int);"));
+ EXPECT_TRUE(Success);
+ EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+ EXPECT_FALSE((bool)llvm::sys::fs::remove(IndexFileName));
+ EXPECT_TRUE(llvm::sys::fs::exists(ASTFileName));
+ EXPECT_FALSE((bool)llvm::sys::fs::remove(ASTFileName));
+ EXPECT_TRUE(llvm::sys::fs::exists(DefinitionFileName));
+ EXPECT_FALSE((bool)llvm::sys::fs::remove(DefinitionFileName));
+}
+
+} // end namespace tooling
+} // end namespace clang
Index: unittests/Tooling/CMakeLists.txt
===
--- unittests/Tooling/CMakeLists.txt
+++ unittests/Tooling/CMakeLists.txt
@@ -13,6 +13,7 @@
add_clang_unittest(ToolingTests
CommentHandlerTest.cpp
CompilationDatabaseTest.cpp
+ CrossTranslationUnitTest.cpp
FixItTest.cpp
LookupTest.cpp
QualTypeNamesTest.cpp
Index: tools/clang-func-mapping/ClangFnMapGen.cpp
===
--- /dev/null
+++ tools/clang-func-mapping/ClangFnMapGen.cpp
@@ -0,0 +1,127 @@
+//===- ClangFnMapGen.cpp ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//======//
+//
+// Clang tool which creates a list of defined functions and the files in which
+// they are defined.
+//
+//======//
+
+#i
[PATCH] D34506: Relax an assert in the comparison of source locations
xazax.hun added a comment. In https://reviews.llvm.org/D34506#787971, @joerg wrote: > I don't think it is a good idea to make this function non-transitive. I think this is a good point. However, I am not entirely sure that it is transitive right now. Check the "Both are in built-in buffers, but from different files" case, which is similar to the separate TU case. But I will look if I can come up with an alternative solution. Repository: rL LLVM https://reviews.llvm.org/D34506 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41266: [analyzer] With c++-allocator-inlining, fix memory space for operator new pointers.
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. In the future, we might want to model the standard placement new and return a symbol with the correct SpaceRegion (i.e.: the space region of the argument). Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:491 + // Placement forms are considered non-standard. + return (FD->getNumParams() == 1); +} The parens are redundant here. Repository: rC Clang https://reviews.llvm.org/D41266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41250: [analyzer] Model implied cast around operator new().
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. The code looks good to me. But the best way to verify these kinds of changes to see how the results change on large projects after applying the patch. https://reviews.llvm.org/D41250 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40560: [analyzer] WIP: Get construction into `operator new` running in simple cases.
xazax.hun added a comment. In https://reviews.llvm.org/D40560#947514, @NoQ wrote: > Replaced the live expression hack with a slightly better approach. It doesn't > update the live variables analysis to take `CFGNewAllocator` into account, > but at least tests now pass. > > In order to keep the return value produced by the `operator new()` call > around until `CXXNewExpr` is evaluated, i added a program state trait, > `CXXNewAllocatorValueStack`: > > 1. Upon evaluating `CFGNewAllocator`, the return `SVal` of the evaluated > allocator call is put here; > 2. Upon evaluating `CXXConstructExpr`, that return value is retrieved from > here; > 3. Upon evaluating `CXXNewExpr`, the return value is retrieved from here > again and then wiped. > > In order to support nested allocator calls, this state trait is organized > as a stack/FIFO, with push in `CFGNewAllocator` and pop in `CXXNewExpr`. The > `llvm::ImmutableList` thing offers some asserts for warning us when we popped > more times than we pushed; we might want to add more asserts here to detect > other mismatches, but i don't see a need for implementing this as an > environment-like map from (`Expr`, `LocationContext`) to SVal. I think it would be great to have tests for such cases to make it apparent it is not trivial to do this only based on the cfg. Maybe something like: A *a = new A(new B, coin ? new C : new D, inlinedCallWithMoreAllocs()); Or in case it turns out to be an easy problem to match these from CFG, I prefer avoiding the state. Also having a new expression within an overloaded operator new might be interesting. > Why `SVal` and not `MemRegion`? Because i believe that ideally we want to > produce constructor calls with null or undefined `this`-arguments. > Constructors into null pointers should be skipped - this is how `operator > new(std::nothrow)` works, for instance. Constructors into garbage pointers > should be immediately caught by the checkers (to throw a warning and generate > a sink), but we still need to produce them so that the checkers could catch > them. But for now `CXXConstructorCall` has room only for one pointer, which > is currently `const MemRegion *`, so this still remains to be tackled. Are you sure we need to produce undef vals? Couldn't a checker subscribe to the post allocator call and check for the undefined value and generate a sink before the constructor call? I am not opposed to using SVals there, just curious. > Also we need to make sure that not only the expression lives, but also its > value lives, with all the various traits attached to it. Hence the new > facility in `ExprEngine::removeDead()` to mark the values in > `CXXNewAllocatorValueStack` as live. Test included in `new-ctor-inlined.cpp` > (the one with `s != 0`) covers this situation. > > Some doxygen comments updated. https://reviews.llvm.org/D40560 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41151: [analyzer] Adding LoopContext and improve loop modeling
xazax.hun added a comment. Just to be sure, this is just a refactoring to make this cleaner or you expect this to have other effects as well, like better performance? https://reviews.llvm.org/D41151 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40937: [clang-tidy] Infinite loop checker
xazax.hun added a comment. I think, while the analyzer is more suitable for certain kinds of checks that require deep analysis, it is still useful to have quicker syntactic checks that can easily identify problems that are the results of typos or incorrectly modified copy and pasted code. I think this check is in that category. Also, the original warning Peter mentioned does something similar but has some shortcomings. The current implementation is not path sensitive. It uses flow sensitivity to check for escaping values. If we would try to port this check to the static analyzer, the questions we would ask from the analyzer are universally quantified (e.g. for all path this variable does not escape and does not change). Unfortunately, it is not that easy with the current analyzer to answer such questions. The static analyzer is better with existential questions (e.g. there is a path such that the condition variables are not escaped and are unchanged in the loop). Using the latter formulation we might have a larger number of false positives because the analyzer sometimes hit infeasible paths. In the first approach, the infeasible paths are less of a problem (they might cause false negatives but not false positives), but we need to be careful with all the peculiarities of the analyzer because it does not guarantee to discover all possible paths. Hopefully, Devin will correct me if I'm wrong :) https://reviews.llvm.org/D40937 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy
xazax.hun added reviewers: NoQ, george.karpenkov. xazax.hun added a comment. In the tests there are multiple variants of the strcpy function guarded by macros. Maybe we should run the tests multiple times to test all variants with different defines? https://reviews.llvm.org/D41384 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41406: [analyzer] Add a new checker callback, check::NewAllocator.
xazax.hun added a comment. Maybe `debug.AnalysisOrder` could be used to test the callback order explicitly. This way the test could also serve as a documentation for the callback order. https://reviews.llvm.org/D41406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41444: [ASTImporterTest] Make testing under '-fdelayed-template-parsing' mandatory
xazax.hun added a comment. Is it possible that this will hide other problems? Wouldn't it be better to run the tests twice once with this argument and once without it? Repository: rC Clang https://reviews.llvm.org/D41444 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41444: [ASTImporterTest] Make testing under '-fdelayed-template-parsing' mandatory
xazax.hun added a comment.
In https://reviews.llvm.org/D41444#960848, @a.sidorin wrote:
> In https://reviews.llvm.org/D41444#960841, @xazax.hun wrote:
>
> > Is it possible that this will hide other problems? Wouldn't it be better to
> > run the tests twice once with this argument and once without it?
>
>
> I don't think so. In fact, without instantiation, we are not even able to
> check semantic code correctness inside templates. So, we are solving this
> problem as well.
E.g. the following code only compiles with `-fdelayed-template-parsing` flag
added:
template
struct Base {
int x;
};
template
struct Derived : Base {
int f() {
return x;
}
};
But yeah, maybe it is not very likely that we hit such issues.
Repository:
rC Clang
https://reviews.llvm.org/D41444
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41451: Make DiagnosticIDs::getAllDiagnostics use std::vector
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D41451 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41444: [ASTImporterTest] Make testing under '-fdelayed-template-parsing' mandatory
xazax.hun added a comment. In https://reviews.llvm.org/D41444#960999, @a.sidorin wrote: > Also, I still think we should rather not apply template-related patches until > this testing is implemented. Gabor, Peter, do you agree? Sure, I am fine with that. Repository: rC Clang https://reviews.llvm.org/D41444 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis
xazax.hun marked 6 inline comments as done.
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:372
+
+ cross_tu::CrossTranslationUnitContext &CTUCtx =
+ Engine->getCrossTranslationUnitContext();
dcoughlin wrote:
> Can this logic be moved to AnalysisDeclContext->getBody()?
>
> CallEvent::getRuntimeDefinition() is really all about modeling function
> dispatch at run time. It seems odd to have the cross-translation-unit loading
> (which is about compiler book-keeping rather than semantics) here.
I just tried that and unfortunately, that introduces cyclic dependencies.
CrossTU depends on Frontend. Frontend depends on Sema. Sema depends on
Analysis. Making Analysis depending on CrossTU introduces the cycle.
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:382
+[&](const cross_tu::IndexError &IE) {
+ CTUCtx.emitCrossTUDiagnostics(IE);
+});
dcoughlin wrote:
> I don't think it makes sense to diagnose index errors here.
>
> Doing it when during analysis means that, for example, the parse error could
> be emitted or not emitted depending on whether the analyzer thinks a
> particular call site is reached.
>
> It would be better to validate/parse the index before starting analysis
> rather than during analysis itself.
>
>
While I agree, right now this validation is not the part of the analyzer but
the responsibility of the "driver" script for example CodeChecker. It is useful
to have this diagnostics to catch bugs in the driver.
https://reviews.llvm.org/D30691
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis
xazax.hun updated this revision to Diff 127525.
xazax.hun marked an inline comment as not done.
xazax.hun added a comment.
- Address some review comments
- Rebased on ToT
https://reviews.llvm.org/D30691
Files:
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
lib/StaticAnalyzer/Core/CMakeLists.txt
lib/StaticAnalyzer/Core/CallEvent.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/PathDiagnostic.cpp
lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
lib/StaticAnalyzer/Frontend/CMakeLists.txt
test/Analysis/Inputs/ctu-chain.cpp
test/Analysis/Inputs/ctu-other.cpp
test/Analysis/Inputs/externalFnMap.txt
test/Analysis/ctu-main.cpp
tools/scan-build-py/libscanbuild/__init__.py
tools/scan-build-py/libscanbuild/analyze.py
tools/scan-build-py/libscanbuild/arguments.py
tools/scan-build-py/libscanbuild/clang.py
tools/scan-build-py/libscanbuild/report.py
tools/scan-build-py/tests/unit/test_analyze.py
tools/scan-build-py/tests/unit/test_clang.py
Index: tools/scan-build-py/tests/unit/test_clang.py
===
--- tools/scan-build-py/tests/unit/test_clang.py
+++ tools/scan-build-py/tests/unit/test_clang.py
@@ -92,3 +92,15 @@
self.assertEqual('Checker One description', result.get('checker.one'))
self.assertTrue('checker.two' in result)
self.assertEqual('Checker Two description', result.get('checker.two'))
+
+
+class ClangIsCtuCapableTest(unittest.TestCase):
+def test_ctu_not_found(self):
+is_ctu = sut.is_ctu_capable('not-found-clang-func-mapping')
+self.assertFalse(is_ctu)
+
+
+class ClangGetTripleArchTest(unittest.TestCase):
+def test_arch_is_not_empty(self):
+arch = sut.get_triple_arch(['clang', '-E', '-'], '.')
+self.assertTrue(len(arch) > 0)
Index: tools/scan-build-py/tests/unit/test_analyze.py
===
--- tools/scan-build-py/tests/unit/test_analyze.py
+++ tools/scan-build-py/tests/unit/test_analyze.py
@@ -4,12 +4,12 @@
# This file is distributed under the University of Illinois Open Source
# License. See LICENSE.TXT for details.
-import libear
-import libscanbuild.analyze as sut
import unittest
import re
import os
import os.path
+import libear
+import libscanbuild.analyze as sut
class ReportDirectoryTest(unittest.TestCase):
@@ -333,3 +333,83 @@
def test_method_exception_not_caught(self):
self.assertRaises(Exception, method_exception_from_inside, dict())
+
+
+class PrefixWithTest(unittest.TestCase):
+
+def test_gives_empty_on_empty(self):
+res = sut.prefix_with(0, [])
+self.assertFalse(res)
+
+def test_interleaves_prefix(self):
+res = sut.prefix_with(0, [1, 2, 3])
+self.assertListEqual([0, 1, 0, 2, 0, 3], res)
+
+
+class MergeCtuMapTest(unittest.TestCase):
+
+def test_no_map_gives_empty(self):
+pairs = sut.create_global_ctu_function_map([])
+self.assertFalse(pairs)
+
+def test_multiple_maps_merged(self):
+concat_map = ['c:@F@fun1#I# ast/fun1.c.ast',
+ 'c:@F@fun2#I# ast/fun2.c.ast',
+ 'c:@F@fun3#I# ast/fun3.c.ast']
+pairs = sut.create_global_ctu_function_map(concat_map)
+self.assertTrue(('c:@F@fun1#I#', 'ast/fun1.c.ast') in pairs)
+self.assertTrue(('c:@F@fun2#I#', 'ast/fun2.c.ast') in pairs)
+self.assertTrue(('c:@F@fun3#I#', 'ast/fun3.c.ast') in pairs)
+self.assertEqual(3, len(pairs))
+
+def test_not_unique_func_left_out(self):
+concat_map = ['c:@F@fun1#I# ast/fun1.c.ast',
+ 'c:@F@fun2#I# ast/fun2.c.ast',
+ 'c:@F@fun1#I# ast/fun7.c.ast']
+pairs = sut.create_global_ctu_function_map(concat_map)
+self.assertFalse(('c:@F@fun1#I#', 'ast/fun1.c.ast') in pairs)
+self.assertFalse(('c:@F@fun1#I#', 'ast/fun7.c.ast') in pairs)
+self.assertTrue(('c:@F@fun2#I#', 'ast/fun2.c.ast') in pairs)
+self.assertEqual(1, len(pairs))
+
+def test_duplicates_are_kept(self):
+concat_map = ['c:@F@fun1#I# ast/fun1.c.ast',
+ 'c:@F@fun2#I# ast/fun2.c.ast',
+ 'c:@F@fun1#I# ast/fun1.c.ast']
+pairs = sut.create_global_ctu_function_map(concat_map)
+self.assertTrue(('c:@F@fun1#I#', 'ast/fun1.c.ast') in pairs)
+self.assertTrue(('c:@F@fun2#I#', 'ast/fun2.c.ast') in pairs)
+self.assertEqual(2, len(pairs))
+
+def test_space_handled_in_source(self):
+concat_map = ['c:@F@fun1#I# ast/f un.c.ast']
+pairs = sut.create_global_ctu_function_map(concat_map)
+self.assertTrue(('c:@F@fun1#I#', 'ast/f un.c.ast') in pairs)
+self.assertEqual(1, len(pairs))
+
+
+class FuncMapSrcToAstTest(unittest.TestCase):
+
+def test_empty_gives_empty(se
[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Core/AnalyzerOptions.cpp:407
+ if (!NaiveCTU.hasValue()) {
+NaiveCTU = getBooleanOption("experimental-enable-naive-ctu-analysis",
+/*Default=*/false);
This option might not be the most readable but this way experimental is a
prefix. Do you prefer this way or something like
`enable-experimental-naive-ctu-analysis`?
https://reviews.llvm.org/D30691
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
