In D71791#1795093, @xazax.hun wrote:

I decided to fix the unittests. Having CFGs full of dangling pointers to the AST does not look fun at all, so I think this change was long overdue.

I am not sure what would be the best way to test this change here.

For reasons other than being a part of the project, CodeChecker is objectively an amazing tool to use with the analyzer. LGTM!

In D69962#1744679, @NoQ wrote:

In D69962#1742291, @Szelethus wrote:

Try to add a non-sanitizer built tablegen: -DCLANG_TABLEGEN=/path/to/non/sanitized/clang-tblgen -DLLVM_TABLEGEN=/path/to/non/sanitized/llvm-tblgen

Yay nice! The clang that i built this way is still unusable though, i.e. crashes on almost all tests :( I guess i'm doing something wrong.

I'd like to commit the test as is and then keep looking for the solution in the background.

Accepted accidentally. Well, in any case, I trust your judgement on this! I'd prefer not to commit the test file as-is.

In D69962#1741503, @NoQ wrote:

In D69962#1739440, @NoQ wrote:

In D69962#1738618, @Szelethus wrote:

Nice catch! Though, wouldn't the memory sanitizer buildbots break on this reliably?

I kinda hope so; i'll take a look at home.

Memory sanitizer at home: *fails with 1700 uninitialized reads in TableGen*.

Whoo!

LGTM, provided that the inlines are addressed! Thanks!

Nice catch! Though, wouldn't the memory sanitizer buildbots break on this reliably?

In D69813#1734272, @Charusso wrote:

In D69813#1734193, @Szelethus wrote:

Hmm, so this checker is rather a collection of CERT rule checkers, right? Shouldn't the checker name contain the actual rule name (STR31-C)? User interfacewise, I would much prefer smaller, leaner checkers than a big one with a lot of options, which are barely supported for end-users. I would expect a cert package to contain subpackages like cert.str, and checker names cert.str.31StringSize, or similar.

It is the STR rules of CERT, nothing else. Most of the rules are tied together, and that is why the checker needs to be designed as one checker at first. I am not sure which part of the STR I will cover, so may when the checker evolves and some functions does not need the same helper methods we need to create new checkers. STR31 and STR32 are my projects which is like one single project. Also I did not except the users to specify the rule number, but this checker could be something like cert.str.Termination. There is two floating-point CERT checkers inside the insecureAPI that is why I have introduced the cert package, which will have three members, one is that new checker. I think a new package is only necessary if it contains at least two checkers.

Hmm, so this checker is rather a collection of CERT rule checkers, right? Shouldn't the checker name contain the actual rule name (STR31-C)? User interfacewise, I would much prefer smaller, leaner checkers than a big one with a lot of options, which are barely supported for end-users. I would expect a cert package to contain subpackages like cert.str, and checker names cert.str.31StringSize, or similar. Also, shouldn't we move related checkers from security.insecureAPI to cert? Or just mention the rule name in the description, and continue not having a cert package?

YES PLEASE. Debug checkers that only dump from check::EndAnalysis won't rely on the analysis not actually crashing. Which is ironically exactly when we want to use them.

Changes to MallocChecker really highlight the positive effects of this patch. Nice!

In D69662#1731974, @balazske wrote:

I wanted to remove eval::Call because only one checker can do this otherwise it is undefined behavior (according to the not very new "Analyzer Guide"). If it is essentially needed in this checker it will remain.

Let's make it 3! Thank you so much for sticking by, I guess one of the reasons why a patch so obviously great and desired took so long is that we still didn't figure out how we imagine the CallDescriptionMap to be integrated into larger checkers :) In any case, this is a major step in the right direction, MallocChecker and some of the others should take notes.

I have no authority in clang-tidy, but the idea is nice! You may wanna reupload with full context though.

LGTM given that the inlines are fixed.

Its becoming a bit difficult to navigate inlines, could you please mark them as done as you address them?

LGTM, can we remove the open projects entry under the same breath?

In D69318#1718220, @gamesh411 wrote:

Please feel free to add more reviewers.

In D66733#1706521, @sylvestre.ledru wrote:

I added it to the release notes here : https://reviews.llvm.org/rC374593
I am wondering if the option( WarnForDeadNestedAssignments ) to disable it is really necessary?
I haven't seen any false positive while deadstore has some.

This is amazing, thanks!! LGTM.

I forgot to emphasise that the entire point of the patch was to get rid of getAllocationFamily, at least the way it used to work, because it was a mess and prevented me from moving forward with changing things to a CallDescriptionMap.

In D66049#1701730, @dkrupp wrote:

I also analyzed openssl with the baseline and this version, but did not find any new warnings.
See:
http://codechecker-demo.eastus.cloudapp.azure.com/Default/#run=D66049_baseline&newcheck=D66049_improved&review-status=Unreviewed&review-status=Confirmed&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=D66049_baseline_diff_D66049_improved

Rebase.

Rebase, unforget to support 3-arg malloc, for which apparently were no tests at all.

Use regular parameters instead of template parameters.

In D68162#1686810, @NoQ wrote:

Thank you, fantastic finding!

in fact we know it compile time

Yeah, but is it accidental or is there a good reason behind always having this information at compile time? 'Cause i don't want to restrict the code to always provide this information at compile time if we're not sure it'll always be able to provide it in compile time.

The logic of the patch is fine in my eyes, but I dislike the formatting. Could you please make the code obey the LLVM coding style, and after that use clang-format-diff.py?
https://llvm.org/docs/CodingStandards.html
https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

A few nits inline, otherwise the patch is awesome, thank you!!

I'm starting to be really happy with the current direction! I think I'll start splitting this up soon, I'm confident that the current interface (after some polishing) is general enough to develop incrementally.

The patch looks alright, I won't formally accept because if I knew how these worked, it wouldn't have caused so much pain to so many people :)

It seems like this patch is diffed against your latest commit, not the master branch.

I have no objections on my end.

If you could change that, it would be awesome! But since this revision has its own side effect, let's commit as-is. LGTM!

Hmm, so before this patch, we just used LVNode everywhere and ignored InputNode. It may not have made much sense, but its still not as confusing as using both if the creation of LVNode is unnecessary overall. Could we just remove it?

Never knew about this either, so I'm not gonna miss it. Thanks!

I really don't think so, but I'll ask around in the office.

The following words are echoing in my ears as I'm knowingly going completely against them:

Apologies for the intrusion, but these plugins were added by me, and test an important case for our internal plugins. Could you please give me just a day or so the check whether the tests work for us correctly?

Welp, windows builtbots broke again. I'll try to see whats wrong with undef sanitizer, but fear this will end up in a revert.

In retrospect, I would have made this patch a little more fragmented (its almost a year old, does it beat the revival of the symbolreaper patch?), but it would be a painful chore to separate at this point, if you dont mind. I have a couple more cooking in the cauldron that are more digestible, so, lesson learned :)

In D54823#1675604, @NoQ wrote:

Is it just me or phabricator somehow refuses to display the changes since the last diff here? That's probably the commit diff is involved somehow. So i'm confused what exactly has changed >.<

In D54823#1675352, @Szelethus wrote:

Rebase, fix (suspected) error that caused buildbot errors. Hold off commiting in favor checking whether putting CallDescriptionMap in would be too invasive, but really, can't be worse then it already is.

Rebase, fix (suspected) error that caused buildbot errors. Hold off commiting in favor checking whether putting CallDescriptionMap in would be too invasive, but really, can't be worse then it already is.

We have no solid evidence to conclude that such an event should not occur at a BlockEntrance, so fixing this error mustn't be that bad. I certainly should've used llvm::isa_or_nonnull, so the patch overall makes sense, so I'm commiting it as is. With that said, this test case highlights a fundamental flaw in how loop wideining is implemented (hence it being off-by-default), and it should be addressed later separately.

While we're there, could you please see whether the included test case (note how condition tracking is turned off) fails on your platform without the rest of the patch applied (it definitely does on mine, which is why I was surprised to see this bug report pop up only now)? If not, I can just push a small commit with the llvm::isa_and_nonnull change to get some breathing room.

In D66716#1671105, @TWeaver wrote:

Hi there,

just checking in, is this patch still going ahead?

thanks
Tom W

Aha, so every user gets to create their own PathDiagnosticConsumerOptions object, makes sense! There is no interface misconception, because -analyzer-config will only configure what the analyzer would tinket with. I like this patch! If you dont mind, I'd prefer to have one last round with this, but otherwise LGTM.

Yes, thank you! I've been keeping my mailbox open and commiting slowly, it seems like the buildbots have a wrong email address set up for me.

In D67420#1668474, @NoQ wrote:

It's not that AnalyzerOptions is necessary to construct this, it's more like sufficient.

In D67420#1666578, @NoQ wrote:

In D67420#1666141, @Szelethus wrote:

I do!

Hmm, it sounds like you want to force both Clang frontend and Clang-Tidy to use the same set of flags to control these options (?) Much like -analyzer-config, these flags will have different "style" compared to other flags in the tool. Which is probably not too bad for hidden frontend flags that control the Analyzer because they're anyway set by a separate GUI checkbox, but the inconsistency with other flags would be super glaring in case of Clang-Tidy CLI. Do we really want to go in that direction? I'll be much more comfortable with letting each tool deal with its flags the way it prefers - this doesn't look like a good place for code reuse to me.

@Szelethus: I could have stored PathDiagnosticConsumerOptions in AnalyzerOptions by value and pass a const reference around, but it wasn't pleasant to integrate with AnalyzerOptions.def. I.e., i'd have to implement a new kind of option that doesn't allocate its own field but instead re-uses a field within a sub-object. Do you want me to go for it or is this implementation good enough?

Looks great! Are we sure that PathDiagnostic.h is a good header name?

LGTM! I think code readability improved geatly.

I'm open to discuss a better design here. Eg., i thought about making it part of the visitor interface instead, but i don't immediately see how to do this without breaking the logic of "only add the note at the call site in which the event has happened, not every time allocated memory is returned from anywhere".

Sure!