It looks like this solution is not going to work in general. The problem is that it can be really hard to tell when it is valid to create a gsl::Pointer from an unannotated local type.

Committed in https://reviews.llvm.org/rG6379e5c8a441 due to it was urgent for some users. Will address any comments post-commit.

• Added a test.

• Add the actual diff.

Sounds good! Let's do that :)

Yeah, the analysis would work fine in this case. But that would mean that we should not propagate the Pointer annotations to derived classes as some of them in the wild do not follow the same ownership model.

In D66486#1643374, @mgehre wrote:

Also it feels a bit weird to change the ownership semantics in a derived class, I bet that would violate the Liskov substitution principle.

And then we see that llvm::OwningArrayRef inherits from llvm::ArrayRef ... But maybe this direction (strengthening a Pointer into an Owner)
is okay.

In D66486#1640203, @mgehre wrote:

In the false-positive example, after the DerivedToBase, we see a constructor call which I think is the copy constructor.

LG!

*actually accepting*

LGTM! I think the UIs could do better displaying this info in the future but this is not your job :)

Ok, I think I know what is going on.

In D66486#1638213, @mgehre wrote:

This change might be the cause for the false-positive in https://github.com/mgehre/llvm-project/issues/45. Could you check?

LG!

I do not know if there are some build bots with -Werror that would warn for unknown attributes. In case it fails the build bots we could extend the include/llvm/Support/Compiler.h header with a macro.

LG! But let's wait for Dmitri :)

In D66152#1627572, @Szelethus wrote:

Apologies for going off-topic, but would it make sense to start lifetime related patches with the tag [LifetimeAnalysis] or similar (like [analyzer] for static analyzer patches)? I dont feel knowledgable enough to participate, but would gladly add a herald rule to be subscribed and learn about the development process of it.

The changes look good to me. Added some folks who might have more knowledge about the original purpose.

In D64256#1626279, @leonardchan wrote:

In D64256#1626025, @xazax.hun wrote:

In D64256#1625998, @leonardchan wrote:

Hi. I noticed in our builders that both of the warnings introduced in this patch are being diagnosed for pointers that don't use GSL at all. I'm attempting to make a small reproducer now.

Hi!

Clang now apply GSL annotations for some STL types automatically. So this is the expected behavior. Are any of those warnings unwanted/false positive with the newest Clang version? If so, please share the reproducers and I will either fix them ASAP or revert/turn off the warnings.

Thanks! I didn't know that they were applied to stl types. This could explain why the warning is raised in my case (std::string), but I don't think there's anything wrong in this example that would lead to a warning:

leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ cat ~/misc/test.cpp
#include <string>

std::string MakeStr();
void other_func(const char *s);

void func(std::string s) {
  const char *x = MakeStr().c_str();
  other_func(x);
}
leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ ~/llvm-monorepo/llvm-build-3-master/bin/clang++ -c ~/misc/test.cpp
/usr/local/google/home/leonardchan/misc/test.cpp:7:19: warning: object backing the pointer will be destroyed at the end of the full-expression [-Wdangling]
  const char *x = MakeStr().c_str();
                  ^~~~~~~~~
1 warning generated.

This was made using a release build from ToT clang.

In D64256#1625998, @leonardchan wrote:

Hi. I noticed in our builders that both of the warnings introduced in this patch are being diagnosed for pointers that don't use GSL at all. I'm attempting to make a small reproducer now.

• Fix review comments.

As this thing will now be part of the checker interface it would be great to have some guidelines which interestingness kind to use (or, when to use interestingness at all). I am totally fine with addressing this in a followup patch though.

Yeah, this is important bike-shedding from the user point of view. Otherwise it looks good to me.

More tests are always welcome :)

I like the change.

Just wondering, did you check if we actually need shared ownership for this type? If not, do not waste your time checking for now :)

LG!

Looks good, some nits inline. I agree with Artem, we should consider omitting the trimming and document how to get something similar if desired for debugging.

LGTM! I also do not see much value in the old code at this point. I would expect PathDiagnosticConsumers to be independent of the interesting symbols/regions.

The direction looks good to me. I wonder if it is worth to add some warnings somewhere that it is really tricky to come up with reasonable fixits for most path sensitive checks.

Thanks for the review!
Since I did some refactoring I will wait for an additional accept before committing.

• Rebase, add the results of testing on real world projects to the description.

• Add new line to end of file.
• Rebase, calls no longer need special handling.

• Actually move the code snippet in question.

• A small refactoring based on an observation from a review comment.

• Rebase

• Remove unused parameter.

• Fix a false positive case found by running over ~200 open source projects

I have run this successfully on ~192 open source projects.

• Fix a false positive from previous change.

• Fix a TODO and add some more tests.

• Address the rest of the review comments.

Since we allow new kinds of SVals to be tracked it would be great to test this first on a larger corpus of projects just to see if there is a crash (due to an unhandled SVal type).

I have one small question otherwise looks good.