llvm-project icon indicating copy to clipboard operation
llvm-project copied to clipboard

Add clang::lifetimebound annotation to ArrayRef constructors.

Open hokein opened this issue 1 year ago • 4 comments

This enables clang to detect more dangling issues.

ArrayRef<int> func() {
   constexpr int array[] = {...}; // oops, missing the static
   return array; // return a dangling reference, bomb.
}

See #113533.

hokein avatar Oct 24 '24 10:10 hokein

@llvm/pr-subscribers-llvm-adt

@llvm/pr-subscribers-llvm-support

Author: Haojian Wu (hokein)

Changes

This enables clang to detect more dangling issues.

ArrayRef&lt;int&gt; func() {
   constexpr int array[] = {...}; // oops, missing the static
   return array; // return a dangling reference, bomb.
}

See #113533.


Full diff: https://github.com/llvm/llvm-project/pull/113547.diff

2 Files Affected:

  • (modified) llvm/include/llvm/ADT/ArrayRef.h (+3-2)
  • (modified) llvm/include/llvm/Support/Compiler.h (+6)
diff --git a/llvm/include/llvm/ADT/ArrayRef.h b/llvm/include/llvm/ADT/ArrayRef.h
index d9897320ce091a..1a2a8a307a2995 100644
--- a/llvm/include/llvm/ADT/ArrayRef.h
+++ b/llvm/include/llvm/ADT/ArrayRef.h
@@ -70,7 +70,7 @@ namespace llvm {
     /*implicit*/ ArrayRef(std::nullopt_t) {}
 
     /// Construct an ArrayRef from a single element.
-    /*implicit*/ ArrayRef(const T &OneElt)
+    /*implicit*/ ArrayRef(const T &OneElt LLVM_LIFETIME_BOUND)
       : Data(&OneElt), Length(1) {}
 
     /// Construct an ArrayRef from a pointer and length.
@@ -103,7 +103,8 @@ namespace llvm {
 
     /// Construct an ArrayRef from a C array.
     template <size_t N>
-    /*implicit*/ constexpr ArrayRef(const T (&Arr)[N]) : Data(Arr), Length(N) {}
+    /*implicit*/ constexpr ArrayRef(const T (&Arr LLVM_LIFETIME_BOUND)[N])
+        : Data(Arr), Length(N) {}
 
     /// Construct an ArrayRef from a std::initializer_list.
 #if LLVM_GNUC_PREREQ(9, 0, 0)
diff --git a/llvm/include/llvm/Support/Compiler.h b/llvm/include/llvm/Support/Compiler.h
index 591e7647795bb2..3b03c2851a4214 100644
--- a/llvm/include/llvm/Support/Compiler.h
+++ b/llvm/include/llvm/Support/Compiler.h
@@ -413,6 +413,12 @@
 #define LLVM_GSL_POINTER
 #endif
 
+#if LLVM_HAS_CPP_ATTRIBUTE(clang::lifetimebound)
+#define LLVM_LIFETIME_BOUND [[clang::lifetimebound]]
+#else
+#define LLVM_LLVM_LIFETIME_BOUND
+#endif
+
 #if LLVM_HAS_CPP_ATTRIBUTE(nodiscard) >= 201907L
 #define LLVM_CTOR_NODISCARD [[nodiscard]]
 #else

llvmbot avatar Oct 24 '24 10:10 llvmbot

:white_check_mark: With the latest revision this PR passed the C/C++ code formatter.

github-actions[bot] avatar Oct 24 '24 11:10 github-actions[bot]

Any reason for not annotating the rest of the constructors?

Note that the ArrayRef is already annotated as gsl::Pointer, so constructor for the gsl::Owner already works without the lifetimebound annotation. I only target on the non-working cases, mostly the generic pointer/reference types.

Below is my test:

llvm::ArrayRef<int> test() {
   int array[10];
   std::vector<int> k;
   std::array<int, 10> k2;
   return k; // warning, local vector (owner)
   return k2; // warning, local array (owner)
   return {1, 2, 3}; // warning, initializing_list, backing array is destoryed.
   return llvm::ArrayRef<int>(array+0, array+10); // wanring
   return array; // warning
   return {array, 10}; // warning
}

hokein avatar Oct 24 '24 12:10 hokein

I only target on the non-working cases, mostly the generic pointer/reference types.

Ah, I see! Thanks! That makes sense. I guess one question is if the future direction is to use both annotations together or to standardize on one of them for more clarity. I don't have a strong opinion on this one.

Xazax-hun avatar Oct 24 '24 12:10 Xazax-hun