[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

xiaobai added inline comments. Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:74 +TaintConfiguration() = default; +TaintConfiguration(const TaintConfiguration &) = delete; +TaintConfiguration(TaintConfiguration &&) = default; ---

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

This revision was automatically updated to reflect the committed changes. Closed by commit rL367190: [analyzer] Add yaml parser to GenericTaintChecker (authored by boga95, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://rev

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

boga95 updated this revision to Diff 212065. boga95 marked 16 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59555/new/ https://reviews.llvm.org/D59555 Files: include/clang/StaticAnalyzer/Checkers/Checkers.td lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

Szelethus accepted this revision. Szelethus added a comment. LGTM, thanks! Please mark inlines as done as you fix them. If you like the file description I proposed, I'm happy to commit this on your behalf (or you might as well apply for a commit access, since you have a track record of high qua

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

NoQ added inline comments. Comment at: test/Analysis/taint-generic.c:24 +// CHECK-INVALID-FILE-SAME: 'alpha.security.taint.TaintPropagation:Config', +// CHECK-INVALID-FILE-SAME:that expects a valid filename Szelethus wrote: > NoQ wrote: > > Szel

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

boga95 updated this revision to Diff 210540. boga95 marked 7 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59555/new/ https://reviews.llvm.org/D59555 Files: include/clang/StaticAnalyzer/Checkers/Checkers.td lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

Szelethus added inline comments. Comment at: lib/StaticAnalyzer/Checkers/Yaml.h:48 + return Config; +} Szelethus wrote: > ```lang=c++ > } // end of namespace clang > } // end of namespace ento > ``` I mean, the other way around >.< Comment at:

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Looks great to me once @Szelethus's comments are addressed. Thanks!! Comment at: lib/StaticAnalyzer/Checkers/Yaml.h:34 +std::string(Conf

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

Szelethus added a comment. Hmm, okay, so we convert `-1` from the config file to `UINT_MAX` in the code, I like it! I wrote a couple nits but they really are just that. In general, for each different error message, a different test case would be great. Comment at: include/cl

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

boga95 updated this revision to Diff 210260. boga95 marked 2 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59555/new/ https://reviews.llvm.org/D59555 Files: include/clang/StaticAnalyzer/Checkers/Checkers.td lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

boga95 updated this revision to Diff 210258. boga95 marked an inline comment as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59555/new/ https://reviews.llvm.org/D59555 Files: include/clang/StaticAnalyzer/Checkers/Checkers.td lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

Szelethus added a comment. Starting to look real good! Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:807 + "", + Released>, + ]>, We mark options that are not yet ready for used with `InAlpha`, rather then `Re

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

boga95 updated this revision to Diff 210073. boga95 marked 11 inline comments as done. boga95 added a comment. Report diagnostic error in case of an invalid filename or an ill-formed yaml file. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59555/new/ https://reviews.llvm.org/D59555 Fi

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

NoQ added a comment. In D59555#1514602 , @NoQ wrote: > I'm still in doubts on how to connect your work with the `CallDescription` > effort. I'll think more about that. I guess i'll just make a yaml reader for `CallDescription`s as soon as the interface

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

boga95 updated this revision to Diff 203339. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59555/new/ https://reviews.llvm.org/D59555 Files: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp lib/StaticAnalyzer/Checkers/Yaml.h test/Analysis/Inputs/taint-gen

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

xazax.hun added a comment. Generally looks good, some nits inline. Please mark comments you already solved as done. Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:73-75 static const unsigned InvalidArgIndex = UINT_MAX; /// Denotes the return vale. sta

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

boga95 marked 3 inline comments as done. boga95 added a comment. I already thought about it. It would make the code much cleaner, but it would have a little performance impact (Does it matter?). It's straightforward to read the supported functions from another yaml file. Besides that, it can sup

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

NoQ added a comment. Ok! This looks like there isn't that much code, so it should be fine to duplicate. Do you have plans to eliminate the existing switch and instead use yaml for *all* supported functions while shipping a default .yaml file with the analyzer? I'm still in doubts on how to con

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

boga95 updated this revision to Diff 10. boga95 edited the summary of this revision. boga95 added a comment. I changed the parsing, therefore the return value index is represented by -1. I added a test configuration file and parse it when testing to prove the configuration doesn't break the c

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

boga95 added a comment. Sorry for the late answer, I was working on my thesis which is about taint analysis. During that, I implemented several cool features (namespaces, std::cin, std::string, etc.) for the checker. I will share them soon. I thought about the API notes and I think it will fit

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

NoQ added a comment. This new approach is clearly useful to other checkers as well, not only the Taint checker. I believe we should strongly consider generalizing it somehow, it's just too awesome to restrict to a single checker. There's also a closely related technology called "API Notes" that

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

Szelethus added a comment. Sorry, I rushed my earlier comment -- I definitely think that we should get rid of the `UINT_MAX` thing before landing this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59555/new/ https://reviews.llvm.org/D59555 __

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

Szelethus added a comment. We agreed to disagree on the static function stuff -- it doesn't really matter, and I don't insist. I have no objections against this patch, great job! I won't formally accept to make it stand out a little more. Thanks! CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

boga95 updated this revision to Diff 193175. boga95 marked an inline comment as done. boga95 added a comment. Rebase after https://reviews.llvm.org/D59861. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59555/new/ https://reviews.llvm.org/D59555 Files: lib/StaticAnalyzer/Checkers/Gene

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

Szelethus added inline comments. Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:73-75 static const unsigned InvalidArgIndex = UINT_MAX; /// Denotes the return vale. static const unsigned ReturnValueIndex = UINT_MAX - 1; boga95 wrote: > S

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

Szelethus added a comment. In D59555#1442331 , @boga95 wrote: > Why is it better not to use `static` functions/variables? Has it any > performance impact? I don't think so, I just think that it's easier to follow what's happening with a non-static fiel

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

boga95 marked an inline comment as done. boga95 added a comment. Why is it better not to use `static` functions/variables? Has it any performance impact? Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:73-75 static const unsigned InvalidArgIndex = UINT_MAX;

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

Szelethus added a comment. I think the idea is awesome, thanks! We really need to not use `UINT_MAX`, and I think `static` fields and member functions are a bit overused in this patch -- can't we just make `getConfiguration` and `parseConfiguration` static non-member functions, and make them t

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

boga95 created this revision. boga95 added reviewers: Szelethus, xazax.hun, dkrupp, NoQ. Herald added subscribers: cfe-commits, Charusso, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, whisperity. Herald added a project: clang. Parse the yaml configuration file and