llvm / llvm-project

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

[[gsl::Pointer]] does not diagnose dangling-through-function-call #111768

Open pkasting opened 3 weeks ago

pkasting commented 3 weeks ago

Similar to issue #99685.

Trivial forwarding helpers defeat the compiler's ability to notice -Wdangling-gsl issues. For example:

#include <string>
#include <utility>

struct [[gsl::Pointer]] S {
  S(const std::string&);
};

template <typename T>
[[clang::always_inline]] S fwd(T&& t) {
  return S(std::forward<T>(t));
}

[[maybe_unused]] static void f() {
  [[maybe_unused]] S s1(std::string("123"));       // Marked dangling, yay!
  [[maybe_unused]] S s2(fwd(std::string("123")));  // Not marked dangling, boo :(
}

Live example: https://godbolt.org/z/T5q3zj6eo.

zmodem commented 3 weeks ago

@hokein maybe

hokein commented 3 weeks ago

[[maybe_unused]] S s2(fwd(std::string("123"))); // Not marked dangling, boo :(

I think this behavior is expected.

When analyzing the constructor call s2(fwd(std::string("123"))), the lifetime analysis doesn't inspect the body of the fwd function, so the compiler isn't aware that the return value is tied to the function parameter.

To make this work, you'll need to annotate fwd:

template <typename T>
[[clang::always_inline]] S fwd(T&& t [[clang::lifetimebound]]) {
  return S(std::forward<T>(t));
}

The lifetimebound annotation informs the compiler that the return value is bound to the function parameter t, https://godbolt.org/z/9dY5v5jKv.

pkasting commented 3 weeks ago

Sadly, trying to annotate fwd() with [[clang::lifetimebound]] brings its own set of problems. There is no usage I can come up with that produces correct results overall. See https://godbolt.org/z/cM1oMdWoE for details.

hokein commented 1 week ago

// Level 2: Try to plumb understanding of borrowed ranges. Misses warning onfwd_bad_2.

Yes, this is an expected false negative, and a limitation of lifetimebound. I think the Level 2 version is the best we have at the moment.

In the case of S fwd_bad_2(fwd(std::string_view(std::string("123")))); at Level 2, the fwd function is the overload for borrowed_range and lacks a lifetimebound annotation on its parameter. As a result, the compiler doesn't realize that the return value is tied to the argument being passed, and thus, no warning is generated.

The constructor case is different, as you've observed. The GSL annotations and lifetimebound annotations are orthogonal features. For GSL-annotated classes, if a GSL pointer is constructed from a GSL owner (or through a chain of initializing gsl::pointerobjects), Clang assumes the pointer retains a reference to the owner object, which triggers the warning.

pkasting commented 1 week ago

What we need to make the fwd_bad_2 case to work is some way of explicitly annotating or else implicitly inferring that for a borrowed_range, we're still lifetime-bound, but now we're bound to the GSL_OWNER's lifetime instead of the GSL_POINTER's lifetime. I don't know if you have a design on the drawing board that could allow such knowledge.