Skip to content

Add clang::lifetimebound annotation to ArrayRef constructors. #113547

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 28, 2024

Conversation

hokein
Copy link
Collaborator

@hokein hokein commented Oct 24, 2024

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.

@llvmbot
Copy link
Member

llvmbot commented Oct 24, 2024

@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

Copy link

github-actions bot commented Oct 24, 2024

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

Copy link
Collaborator

@Xazax-hun Xazax-hun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Any reason for not annotating the rest of the constructors?

Copy link
Collaborator

@Xazax-hun Xazax-hun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think the std::vector and std::array ones could also be annotated.

@hokein
Copy link
Collaborator Author

hokein commented Oct 24, 2024

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
}

@Xazax-hun
Copy link
Collaborator

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.

hokein added a commit that referenced this pull request Oct 24, 2024
The std::initializer_list is a lightweight object, it is passed by value
in general.

This would also avoid a false positive when adding the lifetimebound
annotation (#113547)

```
ArrayRef<int> foo(std::initializer_list<int> list) {
  return ArrayRef<int>(list);
}
```
@hokein hokein force-pushed the lifetime-bound-string-ref branch from 907a185 to 0ac2069 Compare October 24, 2024 17:38
@hokein hokein merged commit e7f422d into llvm:main Oct 28, 2024
8 checks passed
@hokein hokein deleted the lifetime-bound-string-ref branch October 28, 2024 07:39
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
…3590)

The std::initializer_list is a lightweight object, it is passed by value
in general.

This would also avoid a false positive when adding the lifetimebound
annotation (llvm#113547)

```
ArrayRef<int> foo(std::initializer_list<int> list) {
  return ArrayRef<int>(list);
}
```
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
…13547)

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 llvm#113533.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants