llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.66k stars 11.85k forks source link

No dangling assignment involving lifetimebound argument #112234

Open usx95 opened 1 week ago

usx95 commented 1 week ago

https://godbolt.org/z/oedcc359z

struct S { int *x; };

S foo(const int &x [[clang::lifetimebound]]);

void usage() {
    S y = foo(1);  // Warning. Good.
    y = foo(1);    // No warning. Bad.
}
higher-performance commented 1 week ago

I think I'm confused. Neither of those warn for me, but should they? They do warn if the return type is a reference or a pointer, but not if it's an integer.

usx95 commented 1 week ago

Modified the example for a better example. Basically we should warn in all cases because "lifetimebound" means we return an entity which refers to the arg. It does not have to be that we return the same reference as we accept.

higher-performance commented 1 week ago

Yeah makes sense, I agree, thanks!

higher-performance commented 1 week ago

I think #112614 should fix this.

usx95 commented 1 week ago

Copying relevant text from comment on PR:

We already do a good job when GSL pointers and lifetimebound interact as in the bug: https://godbolt.org/z/9M9PsEGs7

I feel we need to fix the analysis for assignment operator here which would fix cases beyond std::string_view. cc: @Xazax-hun @hokein

hokein commented 3 days ago

This case involves the assignment operator, and since users can define their own operator=, we don't know the exact contract of a user-provided assignment operator.

In this case, the assignment operator is implicitly generated by the compiler. If we annotate the operator's parameter with [[clang::lifetimebound]], clang should be able to issue a warning for this scenario (path: assignment operator -> foo).

I think it would be great if the compiler could catch this case.

Fix the analysis for the assignment operator here

This approach alone may not be sufficient. It would continue traversing the expression AST node, potentially causing false positives in cases where the right-hand side is a temporary object, e.g., y = S(...);. We need a way to filter out these false positives. A reasonable heuristic might be:

hokein commented 3 days ago

In this case, the assignment operator is implicitly generated by the compiler. If we annotate the operator's parameter with [[clang::lifetimebound]], clang should be able to issue a warning for this scenario (path: assignment operator -> foo).

Oops, looks like it doesn't work with the trunk clang, the assignment analysis only considers gsl-annotated type, this is a test, I think it is a bug.

usx95 commented 3 days ago

Fix the analysis for the assignment operator here

This approach alone may not be sufficient. It would continue traversing the expression AST node, potentially causing false positives in cases where the right-hand side is a temporary object, e.g., y = S(...);. We need a way to filter out these false positives. A reasonable heuristic might be:

  • the assignment operator is not user-provided;

Doing this only for non-user-defined assignments makes sense to me.

  • check the recorded Path, and emit a warning if the path contains an IndirectLocalPathEntry with a LifetimeBoundCall kind.

I think one of the ways this could also work is to ignore the top level MaterializeTemoraryExpr (if it has same type with LHS). This has also come up in a couple of PRs 1, 2. It is also possible to be conservative and just strip everything until we get to a CallExpr and then do this analysis.