In D89638#2341923, @vsavchenko wrote:

In D89638#2341855, @martong wrote:

I was thinking about an alternative approach (5th approach (?), yeah, I think we need an RFC).

What if we had just one attribute with a StringArgument ?
Actually, we already have that with the annotate attribute.
How do you like this?

struct [[clang::annotate("CSA:supress:RefCntblBaseVirtualDtor")]] Derived2
    : RefCntblBase{};

Disadvantages: we must process strings whenever a node has the annotate attr attached, we have to come up with a "DSL".
Advantages: total flexibility.
WDYT?

Honestly, I think that "CSA:supress:RefCntblBaseVirtualDtor" is way too verbose. It is incredibly easy to make mistakes, it seems hard to use and it is hell a lot to put in the actual code. It also doesn't resolve the issue with what unique identifier we should use. Imagine we decide that we should use Bug type as that unique identifier. It means that "CSA:supress:RefCntblBaseVirtualDtor" should get changed, but it is already in the user's code! It is quite important to settle on a solution that will still be available when we land a more general solution.

I was thinking about an alternative approach (5th approach (?), yeah, I think we need an RFC).

Ok, thanks for the context. LG!

Unfortunately, I could not reproduce the mentioned crash on our macOS machine. The mentioned test just passed with the output below. I gave up.

myuser@msmarple ~/llvm3/build/release_assert $ /usr/local/Frameworks/Python.framework/Versions/3.8/bin/python3.8 /Users/myuser/llvm3/git/llvm-project/lldb/test/API/dotest.py -S nm -u CXXFLAGS -u CFLAGS --codesign-identity - --env LLVM_LIBS_DIR=/Users/myuser/llvm3/build/release_assert/./lib --arch x86_64 --build-dir /Users/myuser/llvm3/build/release_assert/lldb-test-build.noindex -s /Users/myuser/llvm3/build/release_assert/lldb-test-traces --lldb-module-cache-dir /Users/myuser/llvm3/build/release_assert/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /Users/myuser/llvm3/build/release_assert/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /Users/myuser/llvm3/build/release_assert/./bin/lldb --compiler /Users/myuser/llvm3/build/release_assert/./bin/clang --dsymutil /Users/myuser/llvm3/build/release_assert/./bin/dsymutil --filecheck /Users/myuser/llvm3/build/release_assert/./bin/FileCheck --yaml2obj /Users/myuser/llvm3/build/release_assert/./bin/yaml2obj --server /Users/myuser/llvm3/build/release_assert/./bin/debugserver --lldb-libs-dir /Users/myuser/llvm3/build/release_assert/./lib /Users/myuser/llvm3/git/llvm-project/lldb/test/API/commands/expression/import_builtin_fileid/ -p TestImportBuiltinFileID.py -t
lldb version 12.0.99 (https://github.com/llvm/llvm-project.git revision d454328ea88562a6ec6260529a040035ab9c4a06)
  clang revision d454328ea88562a6ec6260529a040035ab9c4a06
  llvm revision d454328ea88562a6ec6260529a040035ab9c4a06
libstdcxx tests will not be run because: Don't know how to build with libstdcxx on macosx
Skipping following debug info categories: ['dwo']

Adding @NoQ as a reviewer, he might have some valuable comments, as he is very keen on this feature.

as it is something which is actually in production already
...
At the very least we need it for compatibility - this is already a shipping feature.

What is the context here? Did it cause any crash/bug or were you just reading through the code for a good night sleep? :D

Seems like this patch handles already existing attributes in Clang, so this might not be affected by the concerns brought up here by James:
http://lists.llvm.org/pipermail/cfe-dev/2020-October/066996.html

In D63640#2331779, @Tyker wrote:

In D63640#2331734, @martong wrote:

In D63640#2331410, @rsmith wrote:

Reverse ping: I have a patch implementing class type non-type template parameters

that's blocked on this landing. If you won't have time to address @martong's comments soon, do you mind if I take this over and land it?

nice to see this coming.

It is okay for me to commit this patch in its current state. The changes I suggested could result in a cleaner code, but I can do those changes after we land this.

i couldn't apply martong's suggestion completely because importChecked is part of ASTNodeImporter not ASTImporter, but cleaned up some code.

In D63640#2331410, @rsmith wrote:

Reverse ping: I have a patch implementing class type non-type template parameters that's blocked on this landing. If you won't have time to address @martong's comments soon, do you mind if I take this over and land it?

Ups, sorry for the break and thanks for the notice! I am going to investigate.

Thanks for the reivew!

Thanks for the review!

Currently, we are totally inconsistent about the diagnostics. Either we should emit a diagnostic before all return false or we should not ever emit any diags. The diagnostics in their current form are misleading, because there could be many notes missing. I am not sure how much do you guys value these diags in LLDB, but in CTU we simply don't care about them. I'd rather remove these diags from the equivalency check code, because they are causing only confusion. (Or we should properly implement and test all aspects of the diags.)

Sorry for the late review, I just noticed something which is not a logical error, but we could make the ASTImporter code much cleaner.

• Delegate to BitWidth Expr matching in case of BitFields

In D88665#2307562, @shafik wrote:

So was the bug we were saying there were falsely equivalent or falsely not equivalent?

I think the bug is the crash :)

Yes. More specifically, the call of getBitWidthValue() caused a segfault with release builds and with assertions enabled it caused the mentioned assert on the given test code.

The added test causes the below assertion in the baseline (w/o the fix):

ASTTests: ../../git/llvm-project/clang/lib/AST/ExprConstant.cpp:14543: llvm::APSInt clang::Expr::EvaluateKnownConstInt(const clang::ASTContext&, llvm::SmallVectorImpl<std::pair<clang::SourceLocation, clang::PartialDiagnostic> >*) const: Assertion `!isValueDependent() && "Expression evaluator can't be called on a dependent expression."' failed.

In D88019#2296337, @steakhal wrote:

In D88019#2291953, @steakhal wrote:

What are our options mitigating anything similar happening in the future?

This way any change touching the SymbolicRangeInferrer and any related parts of the analyzer seems to be way too fragile.
Especially, since we might want to add support for comparing SymSyms, just like we try to do in D77792.

What about changing the EXPENSIVE_CHECKS in the assume function in the following way:
Convert all range constraints into a Z3 model and check if that is UNSAT.
In that case, we would have returned a state with contradictions, so we would prevent this particular bug from lurking around to bite us later.

And another possibility could be to create a debug checker, which registers to the assume callback and does the same conversion and check.
This is more appealing to me in some way, like decouples the Z3 dependency from the ConstraintManager header.

Which approach should I prefer? @NoQ @vsavchenko @martong @xazax.hun @Szelethus

In this example, it cast to the unsigned char (which is the type of the stored value of **b, stored at #1) instead of the static type of the access (the type of **b is charat #2).

Beware, Phabricator ruins the visual experience of this nice analysis. E.g //char ***// is visible as an italic char *.

In D88092#2289312, @Szelethus wrote:

A joy of reviewing C++ code is that you get to marvel in all the great things the language has, without having to pull fistfuls of hair out to get get to that point. These patches are always a treat. LGTM!

Thanks for the review! :)

Adding Valeriy as a reviewer. He's been working with the solver recently, so his insights might be really useful here.

• Avoid same pattern when checking whether the assumption is infeasible

Thanks for the quick review!
I updated according to your suggestion, so we avoid the same pattern.

@steakhal, thank you for your time and huge effort in debugging this!

We came up with a quick fix, let us know what you think:
https://reviews.llvm.org/D88019

Hi Valery,

Alright, I refactored the change a bit. A Constraint::assume works similarly to an engine DualAssume. Plus I added isApplicable to check if it is meaningful to call the assume at all.

• apply -> assume, Add isApplicable

In D86660#2278025, @shafik wrote:

We are going to move forward with this approach (after dealing with the multi-dimensional array case) temporarily. We are seeing crash bugs from this from users and we want to fix it while we explore the solution space more.

Okay, I understand the business reasons and I am fine with this patch if this is really a temporary solution. Just please add a FIXME that indicates this and D71378 are temporary, plus that this should be really handled by the preceding import call.

In D87619#2273450, @martong wrote:

Thank you! This is a great simplification of the code and it is much cleaner this way, nice work!
I queued a CTU build, just to verify nothing will be break:
#1​71​9 at http://codechecker-buildbot.eastus.cloudapp.azure.com:8080/job/ctu_pipeline_clang-master-monorepo/

Actually, the test added here could have catched that regression.

In D87683#2274663, @baloghadamsoftware wrote:

In D87683#2274569, @njames93 wrote:

Please fix the test case first, can't call operator new(unsigned long, void*) with an argument of type void*
The other failures the pre merge bot detected can safely be disregarded

Placement new is defined per standard:

void* operator new  ( std::size_t count, void* ptr );

Here, the problem is that size_t is unsigned long on Linux and it seems that it is unsigned long long on Windows. How should I overcome this?

There is a blatant regression we introduced in D87240. Actually, the test added here could have catched that regression.
See https://reviews.llvm.org/D87240#inline-812062
I am putting back the modeling dependency with this patch.

In D87619#2273462, @teemperor wrote:

In D87619#2273450, @martong wrote:

Thank you! This is a great simplification of the code and it is much cleaner this way, nice work!
I queued a CTU build, just to verify nothing will be break:
#1​71​9 at http://codechecker-buildbot.eastus.cloudapp.azure.com:8080/job/ctu_pipeline_clang-master-monorepo/

A related question: You see any problem with doing the same generated Traversal* code we do for the Stmts also for the Decls? This way we could simplify some common checks (e.g., NameDecl's name comparison could cover all subclasses that want it). It seems that for example the Records can't just reuse the name comparison because of the getTypedefNameForAnonDecl() trick, so it might not be as simple as the Stmt code.

Thank you! This is a great simplification of the code and it is much cleaner this way, nice work!
I queued a CTU build, just to verify nothing will be break:
#1​71​9 at http://codechecker-buildbot.eastus.cloudapp.azure.com:8080/job/ctu_pipeline_clang-master-monorepo/

• Add tests to verify compatibility of the two checkers

This problem only exists if we traverse a CFGBlock in order. And Liveness in fact does it reverse order. So a distinct pass is indeed unnecessary, we can note the appearance of the assignment by the time we reach the variable.

Unfortunately template specializations do not catch the descendants of the class for which the template is specialized. Therefore it does not work correcly for the descendants of FunctionDecl, such as CXXMethodDecl, CXXConstructorDecl, CXXDestructorDecl etc. This patch fixes this issue by using a template metaprogram.

In D87444#2268073, @martong wrote:

In D87444#2268001, @balazske wrote:

Probably it should be tested how this change affects CTU analysis (function body is checked now).

I queued a build (#1704) in our CI for CTU: http://codechecker-buildbot.eastus.cloudapp.azure.com:8080/job/ctu_pipeline_clang-master-monorepo/
I don't expect much regression because my gut feeling is that it is quite rare to compare two function definitions. But let's see the results. Raphael would you mind if we wait for the results before committing?

In D87444#2268001, @balazske wrote:

Probably it should be tested how this change affects CTU analysis (function body is checked now).

Thank you for the changes! This looks good to me now!

Changing reviewers:

• Removing 'a.sidorin' with dot. Afaik, Alexei's live user is 'a_sidorin' with an underscore, so I added it.
• Removing 'jdoerfert' because his herald script is ought to match OpenMP related changes, but it matched 'omp' in the word 'compare' in the title.
• Adding 'balazske' as he's been working recently with this class. @balazske, if you have some time, please take a look, your insight could be useful.

Thanks! This patch is really cool, I like it overall.

In D79431#2265155, @Szelethus wrote:

In D79431#2263693, @martong wrote:

In D79431#2263690, @martong wrote:

What if we'd add a toString method to the constraints and we'd add this to Msg? This way we'd know the contents of the constraint, thus we we'd know how the constraint is violated.

I mean we'd know what is not satisfied. But, to know why exactly that is not satisfied we should dump the whole State but that's obviously not an option. Perhaps we could track which symbols and expressions are participating in the assumption related to the constraint and we could dump only those, but this seems to be a very complex approach.

I realize that the how and why phrases in this context a bit too vague :) What do you mean under having to dump the whole State? I didn't mean to compress a bug path into a warning message, only what I mentioned in D79431#2020951. In any case, I think its okay to just move on with this patch. LGTM!

In D79431#2263690, @martong wrote:

In D79431#2215610, @Szelethus wrote:

Ah, okay, silly me. Didn't catch that. Then the message is OK for now.

edit: Describing how the violation happened might be a valuable for development purposes as well.

What if we'd add a toString method to the constraints and we'd add this to Msg? This way we'd know the contents of the constraint, thus we we'd know how the constraint is violated.

I mean we'd know what is not satisfied. But, to know why exactly that is not satisfied we should dump the whole State but that's obviously not an option. Perhaps we could track which symbols and expressions are participating in the assumption related to the constraint and we could dump only those, but this seems to be a very complex approach.

In D79431#2215610, @Szelethus wrote:

Ah, okay, silly me. Didn't catch that. Then the message is OK for now.

edit: Describing how the violation happened might be a valuable for development purposes as well.

@steakhal Ping.

This basically looks good to me.

LGTM! Thanks for the clarification and the example you gave.
(I agree with @steakhal and I wish if we could get rid of the many lines not-descriptive plist stuff, but that is rather unrelated)

But if the string is invalidated (or the new length is completely unknown), do not remove the length from the state; instead, set it to SymbolConjured.

Where exactly do you implement the above?

In D87239#2259345, @steakhal wrote:

I completely agree with you.
I plan to further refactor the CStringChecker, but the related patches are pretty much stuck :D

I think this workaround is fine for now.
You might as well extend the corresponding parts of the CStringChecker to make the modelling more precise.
It shouldn't be much of a hassle.
What do you say about that?

I think the modeling is well done and precise. I mean, it seems like all of the constraints that I am removing here are handled in CStringChecker. It checks the pointer arguments whether they are null. Also, the length is checked in case of strncasecmp, here:

if (IsBounded) {
  // Get the max number of characters to compare.
  const Expr *lenExpr = CE->getArg(2);
  SVal lenVal = state->getSVal(lenExpr, LCtx);

In D87081#2258637, @Szelethus wrote:

The patch looks great, in fact, it demonstrates how well thought out your summary crafting machinery is.

In D87081#2258579, @martong wrote:

However, in a similar case with the CallAndMessage Checker, we decided to list the more specific Checker as a dependency.

We got the answer to D77061#2057063! We should turn it into a weak dependency though (D80905).

In D87081#2256636, @balazske wrote:

This checker will make an additional assumption on fread and fwrite with the ReturnValueCondition. The return value is constrained by StreamChecker too but it splits the error (if returned value is less that arg 3) and non-error cases into separate branches. I think this causes no problem because it will refine the assumption made here (if this assumption is made first) or the assumption here has no effect (if the split happened already).

Be sure to triple check whether the ExplodedGraph looks okay with both checkers enabled.

In D87081#2256636, @balazske wrote:

This checker will make an additional assumption on fread and fwrite with the ReturnValueCondition.

There is nothing new in that. This assumption described by the .Case has been here since the inception of this Checker. This patch does not change it. This patch adds two new argument constraints.