This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
martong marked an inline comment as done.
Closed by commit rG56b9b97c1ef5: [clang][analyzer][ctu] Make CTU a two phase
analysis (authored by martong).
Changed prior
martong marked an inline comment as done.
martong added inline comments.
Comment at: clang/docs/ReleaseNotes.rst:352
+ reports are lost compared to single-TU analysis, the lost reports are highly
+ likely to be false positives.
xazax.hun wrote:
> I wonder if
xazax.hun added inline comments.
Comment at: clang/docs/ReleaseNotes.rst:352
+ reports are lost compared to single-TU analysis, the lost reports are highly
+ likely to be false positives.
I wonder if you also want to mention that this still finds most of the
xazax.hun accepted this revision.
xazax.hun added inline comments.
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:446
+}
+const bool BState = State->contains(D);
+if (!BState) { // This is the first time we see this foreign function.
martong updated this revision to Diff 430047.
martong added a comment.
- Update ReleaseNotes.rst
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123773/new/
https://reviews.llvm.org/D123773
Files:
clang/docs/ReleaseNotes.rst
martong updated this revision to Diff 430023.
martong marked an inline comment as done.
martong added a comment.
- Change ctuBifurcate to use bool in GDM
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123773/new/
https://reviews.llvm.org/D123773
martong marked an inline comment as done.
martong added inline comments.
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:446
+}
+const bool BState = State->contains(D);
+if (!BState) { // This is the first time we see this foreign function.
xazax.hun added inline comments.
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:446
+}
+const bool BState = State->contains(D);
+if (!BState) { // This is the first time we see this foreign function.
martong wrote:
> xazax.hun
martong updated this revision to Diff 429172.
martong marked 7 inline comments as done.
martong added a comment.
- Change -mul to -pct
- Change default values to ctu-max-nodes-pct = 50 and ctu-max-nodes-min = 1
- Rename isCTUInFirtstPhase to isSecondPhaseCTU and invert the condition
- Check
martong marked 15 inline comments as done.
martong added inline comments.
Comment at: clang/include/clang/AST/ASTImporterSharedState.h:83
+
+ void setNewDecl(Decl *ToD) { NewDecls.insert(ToD); }
};
whisperity wrote:
> (The naming of this function feels a bit
whisperity added inline comments.
Comment at: clang/include/clang/AST/ASTImporterSharedState.h:83
+
+ void setNewDecl(Decl *ToD) { NewDecls.insert(ToD); }
};
(The naming of this function feels a bit odd. `markAsNewDecl` or just
`markNewDecl`?)
martong marked 3 inline comments as done.
martong added inline comments.
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:116
+ const bool Foreign = false; // From CTU.
+
xazax.hun wrote:
> martong wrote:
> > martong wrote:
> > >
xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.
Overall looks good to me. I wish some parts would be simpler but it looks like
sometimes it is not easy to extend the current code and we might need to do
some refactoring at some
xazax.hun added inline comments.
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:116
+ const bool Foreign = false; // From CTU.
+
martong wrote:
> martong wrote:
> > xazax.hun wrote:
> > > I feel that we use different terms for
xazax.hun added inline comments.
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:446
+}
+const bool BState = State->contains(D);
+if (!BState) { // This is the first time we see this foreign function.
xazax.hun wrote:
> martong
xazax.hun added inline comments.
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:446
+}
+const bool BState = State->contains(D);
+if (!BState) { // This is the first time we see this foreign function.
martong wrote:
> xazax.hun
martong added inline comments.
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:116
+ const bool Foreign = false; // From CTU.
+
martong wrote:
> xazax.hun wrote:
> > I feel that we use different terms for the imported
martong updated this revision to Diff 428055.
martong marked 8 inline comments as done.
martong added a comment.
Herald added a reviewer: shafik.
- Add explanatory comment to RuntimeDefinition::Foreign field
- Changed the return value from bool to void in ctuBifurcate and in inlineCall
- Add
martong marked 4 inline comments as done.
martong added a comment.
Thanks for the review Gabor, I'll update soon and hope I could answer your
questions.
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:116
+ const bool Foreign = false; // From
xazax.hun added inline comments.
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:116
+ const bool Foreign = false; // From CTU.
+
I feel that we use different terms for the imported declarations. Sometimes we
call them `new`,
martong updated this revision to Diff 426037.
martong added a comment.
- Add more explanatory comments to the values of ctu-phase1-inlining
- Fix typo in comments in ctu-on-demand-parsing tests
- Remove stale comment, config options are desribed elsewhere
Repository:
rG LLVM Github Monorepo
martong added inline comments.
Comment at: clang/test/Analysis/ctu-on-demand-parsing.c:23
+// FIXME On-demand ctu should be tested in the very same file that we have for
+// the PCH version, but with a different a different verify prefix (e.g.
+// -verfiy=on-demanc-ctu)
martong updated this revision to Diff 425784.
martong added a comment.
- Chosing the correct baseline this time
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123773/new/
https://reviews.llvm.org/D123773
Files:
martong updated this revision to Diff 425782.
martong added a comment.
- Add an option to config the inlining mode during the first phase
- Change the `ctu-main.c[pp]` tests to have a RUN line with this new config
option that mimics the old ctu implementation's behavor.
- Change the
xazax.hun added a comment.
> Make CTU a two phase analysis
At this point maybe it will be a three phase, as we might dump the ASTs/create
index in phase 1 :)
> During this phase, if we find a foreign function (that could be inlined from
> another TU) then we don’t inline that immediately, we
martong added inline comments.
Comment at: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp:167
+ // Let CTU run as many steps we had in the single TU run.
+ // However, we need at least some minimal value to pass those lit tests that
+ // report a bug only in the CTU mode.
martong created this revision.
martong added reviewers: steakhal, NoQ.
Herald added subscribers: manas, ASDenysPetrov, gamesh411, dkrupp, donat.nagy,
Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware,
xazax.hun.
Herald added a reviewer: Szelethus.
Herald added a
27 matches
Mail list logo
