[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

xazax.hun abandoned this revision. xazax.hun added a comment. 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. For example the code below is definitely buggy:

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

gribozavr added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:6775 +if (!pathOnlyInitializesGslPointer(Path)) + Init = const_cast(Init->skipRValueSubobjectAdjustments()); xazax.hun wrote: > gribozavr wrote: > > I'm afraid this change could

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

xazax.hun added a comment. Sounds good! Let's do that :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66486/new/ https://reviews.llvm.org/D66486 ___ cfe-commits mailing list

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

mgehre added a comment. Yes, it means that the automatic inference of Pointer/Owner from base classes has the same issues as all of those automatic inferences: Once can construct a case where they are not true. So for having no false-positives at all, we should avoid this inference by default

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

xazax.hun added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

mgehre added a comment. When I understand you correctly, you are thinking about the cases OwningArrayRef getRef(); ArrayRef f() { ArrayRef r = getRef(); // warning: r points into temporary owner return r; } and ArrayRef getRef() { OwningArrayRef r; return r; //

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

xazax.hun added a comment. 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

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

mgehre added a comment. > 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

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

xazax.hun added a comment. 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. Yeah, but we do not have problems with the copy constructors.

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

mgehre added a comment. In the false-positive example, after the `DerivedToBase`, we see a constructor call which I think is the copy constructor. 1. We should consider `MutableArrayRef` to be a gsl::Pointer according to the paper, because it publicly derives from one. 2. Also in the paper,

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

xazax.hun marked an inline comment as done. xazax.hun added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:6775 +if (!pathOnlyInitializesGslPointer(Path)) + Init = const_cast(Init->skipRValueSubobjectAdjustments()); gribozavr wrote: > I'm

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

gribozavr added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:6775 +if (!pathOnlyInitializesGslPointer(Path)) + Init = const_cast(Init->skipRValueSubobjectAdjustments()); I'm afraid this change could disable some other analysis, which would

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

xazax.hun added a comment. Ok, I think I know what is going on. Basically, we have a `MutableArrayRef` which is NOT a pointer as it is NOT annotated as `gsl::Pointer`. So in the AST we see a local non-pointer object, and we derive a pointer from this local object. This is why the warnings

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

xazax.hun added a comment. 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? Sure, looking into it! Thanks for pointing this out.

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

mgehre added a comment. This change might be the cause for the false-positive in https://github.com/mgehre/llvm-project/issues/45. Could you check? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66486/new/ https://reviews.llvm.org/D66486

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

xazax.hun created this revision. xazax.hun added reviewers: gribozavr, mgehre. xazax.hun added a project: clang. Herald added subscribers: Szelethus, Charusso, gamesh411, dkrupp, rnkovacs. This patch relaxes some of the checks so we can detect more cases where local variable escapes.