In D97183#2701441, @steakhal wrote:

I think we can conclude that P must be non-null (since it was unconditionally dereferenced), thus the previous check on the inner pointer and the branch it guards must be dead!

I'm catching up and these changes look great.

Which is fairly shocking and mind blowing that we've organically developed two independent implementations of casting, one for RegionStore and one for everything else.

Looks great now, thanks!

Looks great now!

This looks amazing, thanks a lot.

Here's a reduced repro - a file that has different behavior before and after the patch (sorry, not perfectly reduced, my creduce is broken again):

// RUN: clang --analyze %s
typedef Oid;
typedef Pointer;
typedef text;
errstart(int elevel, filename, lineno, funcname, domain);
typedef Datum;
typedef struct {
  Oid elemtype;
} ArrayType;

Great, thanks!~

Looks great and I'm also curious about nested initializers.

Looks great now!

I mean, the extent of an ElementRegion is the size of a single element. The reason why our intrinsic isn't doing what you expect is because we represent the pointer with offset as ElementRegion regardless of whether operator [] was used. On the other hand, an SVal doesn't ever represent a region at all, it only points to its first byte, regardless of the structure of MemRegion inside it; for that reason clang_analyzer_getExtent() is impossible to implement correctly in our current model.

More tests are great!

In D98726#2630947, @RedDocMD wrote:

@NoQ, regarding https://godbolt.org/z/1EczEW, I don't think there should be a warning since the original pointer may be needed in the caller.

It does make sense to split these in two, but I'm not so sure about evalCall.

What about clang_analyzer_getExtent(&x[2]) then?

This will be obsoleted by D69726 because they are going to be the same symbol to begin with.

I don't see any further bugs so I think this is good to go!

In D97183#2650172, @steakhal wrote:

In D97183#2640559, @RedDocMD wrote:

@NoQ, why does the following trigger a null-dereference warning? (https://godbolt.org/z/Kxox8qd16)

void g(std::unique_ptr<A> a) {
  A *aptr = a.get();
  if (!aptr) {}
  a->foo();
}

When a->foo() is called, the constraint !aptr is no longer valid and so InnerPointerVal corresponding to a is no longer constrained to be null.
Am I missing something?

When the if's condition is evaluated, it probably triggered a state split. On one path the aptr (aka. the inner pointer) will be constrained to null.
The only way to be sure is by checking the exploded graph and see where it goes.

LGTM! Thanks for formatting ^.^

I think this is a good targeted fix for that problem we've discussed, demonstrated by tests.

No-no, TrackControlDependencyCondBRVisitor's purpose is completely different. It tracks symbols that didn't necessarily participate in the report but participated in conditional statements that lexically surround the report. It's used for explaining how did we got ourselves into a given lexical spot but it doesn't explain why is this a problem.

Aha ok, seems reasonable, thx!

By tracking the call-expression you're basically tracking the raw pointer value because that's what operators * and -> return. Of course operator * returns an lvalue reference rather than a pointer but we don't make a difference when it comes to SVal representation.

Great, thanks!!

LGTM!

That's what i like to see!

In D98726#2629722, @steakhal wrote:

For example this code does not trigger any warning:
https://godbolt.org/z/aM4xe7

LGTM! Do any of the high-level comments at the beginning of the file need to be updated to reflect this change?

Hmm this probably even deserves a warning? Like, the mutex goes out of scope but we don't know whether we've successfully destroyed it? Even if we perform the check later, we can't do anything about it anymore? (not sure how bad it is in practice)

Actually many cases don't need to know an exact original type.

I can see that it adds many visitors to the BugReport passed to it.

You're testing it with the help of an alpha checker. Is this checker doing something special that we don't do normally? In particular, both array bound checkers that we have are very likely to be re-done from scratch if they are ever to be completed. A lot of decisions in them are questionable. So i'm worried that this test will become stale in that process. Maybe a unittest would be more appropriate so that to guarantee the specific functionality without relying on implementation details of the specific checker(?)

Ok then, let's try to land it! 🤞

why does this not work?

I still have concerns about these functions - isNull, isNonNull etc. They do indeed provide a quick test for nullness that doesn't involve constructing a new program state. This test, however, will never be as precise as constructing a new state. It used to be when RangeConstraintManager boiled down to simple range operations but now that it grew much larger (eg., the newly added support for symbolic == and != that involves tracking equivalence classes), there's really no way to tell whether a state is going to be feasible without running the whole machine and observing its emergent behavior, which is what assume() does. On the other hand i still don't see any indication that assume() is noticeably expensive. So i'm really worried that this is a premature optimization that sacrifices correctness for nothing. So i'm in favor of phasing out these functions entirely and converting all code to always use assume(). This may occasionally involve untangling unwanted recursions but i hope that all recursive-ish operations can be isolated within the constraint manager itself (and possibly in checker's evalAssume() which we can hopefully guard against with runtime assertions).

Looks all clear.

So, like, I think it's a start. You introduced a single source of truth for casting SVals and it's good. But i'm worried about our API surface.

tiny patch

I'm actually shocked that we provide such option at all. And that it's the first option in the list in scan-build --help. And that I haven't noticed it until now despite that. How did you discover the crash? Are people actually using it?

CERT rules are typically very vague and don't map nicely into specific static analysis algorithms. A lot of CERT rules flag valid code as well as bugs. I want you to explain what *exactly* does the checker checks (say, in the form of a state machine, i.e. what sequences of events in the program does it find) and whether flagged code is expected to be always invalid.

That's definitely an improvement for our API surface. I think this is good but like @steakhal said I recommend running on a large codebase looking for potential regressions because this code is (still) very much spaghetti and hard to reason about.

In some cases BugReport.isInteresting(InnerPointerVal.getAsSymbol()) would yield us exactly what we want. But if there's no symbol we have no choice but to interact with the trackExpressionValue facility and this may turn out to be pretty challenging.

The warning should be emitted but it shouldn't have a note at P.get() telling the user that an inner pointer was obtained.

I suspect you're adding too many notes. The note needs to not be there if the *raw* pointer is not tracked. Eg., I suspect that your patch would add a note in the following case in which it shouldn't be there because the raw pointer value doesn't participate in the report despite smart pointer region being interesting:

std::unique_ptr<A> P;
A *a = P.get(); // shound't emit a note here
P->foo();

It's important to not emit redundant notes because users typically take these checker-specific notes as an indication that this information is an essential piece of evidence of the bug in their program. In this example they'd believe that the analyzer has figured out that the smart pointer is null by looking at what happens to the raw pointer value. So they may become very confused if this isn't the case.

We doing it this way now? Thanks! o.o

Thx!~

but it is planned that the checker takes use of the planned new attributes to determine if a function should be checked

Ok no problem!

In D96215#2548492, @aaron.ballman wrote:

Should structured bindings be treated similarly as well (not necessarily as part of this patch)?

Aha, yup, makes sense!

We have a bad track record of finishing, but forgetting to enable GSoC projects -- did the same happen to SmartPtrChecker or that still needs refinement?

What I want to avoid is having a static analyzer-specific suppression attribute, and a different one for clang-tidy, and a third one for the frontend.

Unforget to git add tests.

I excluded remarks for now with an assertion. Other tests added :)

Address review comments!

In D95307#2519597, @vsavchenko wrote:

In D95307#2519309, @RedDocMD wrote:

Funnily enough, when I run ninja clang-check I don't get any errors.

I believe that ninja check-clang is the right command (clang-check is a tool) :-)

In D95307#2519283, @vsavchenko wrote:

In D95307#2518592, @NoQ wrote:

This patch shoots the messenger but someone still needs to conduct a proper investigation. The assertion is losing a lot more of its bug-finding power than necessary to uncover the current cornercase; i really hope to preserve it instead.

You mean the failing assertion from the tests?

Thanks!!

This patch shoots the messenger but someone still needs to conduct a proper investigation. The assertion is losing a lot more of its bug-finding power than necessary to uncover the current cornercase; i really hope to preserve it instead.

I added those for consistency but i think your effort is worth it so let's ditch them.

Add a few comments here and there.

this would be a great flow-sensitive check for the clang static analyzer

Yup, i'm sorry. I even saw this failure locally but was like "this test doesn't look familiar, probably someone else broke it" which i wouldn't have said if i looked at it for more than 1 second as it's strikingly obvious. My bad.

My code seems to think that a variable print_count has the value INT_MAX for some reason

Drop broken attributes. Fix naming.

Handle conflicts across redeclarations! Fix typos and documentation formatting.

This patch adds a new core checker. I wouldn't be comfortable enabling it by default without a much more careful evaluation (than a single lit test). In particular, i'm worried about false positives due to constraint solving issues and our incorrect cast representation - it sounds like both of these may disproportionally affect this checker. I also still wonder about "intentional" use of signed overflows; it's likely that some developers would prefer to silence the check. At least we should have a rough idea about how loud it is.

Amazing, thank you. I'm happy with the analysis and i have nothing more to say really :)

Add an error for conflicting attributes!

I think you've found a very nice and compact 50% solution to the problem. I didn't think of this while i was looking for a proper fix. Very nice.