[WIP][clang]: Implement a conditional lifetimebound_if builtin. #125520
Conversation
|
I think we should heavily consider the wider solution space before going with this proposal. The problem we will quickly hit here is that we need to be sure that whatever we want can actually be expressed as a condition in the cases we want. That's not currently the case, which is going to pose problems. For example, we'll want to be able to handle this case (let's call this the type-erased case): As well as this case (call this the variadic case): (I feel there are more interesting cases here -- these are just the two off the top of my head.) To handle such cases we'd need to provide a way to get lifetimes as well, not merely set the attributes. Which means for Given the above, the solution I would suggest considering here is the following: What we ultimately want is to be able to specify things in terms of expressions, I think. That suggests we want should consider syntax like this: I think we should consider something along the lines of the above. An interesting observation here, though, is that we can avoid code duplication even more if we just make the inference based on existing The main problem I see here is that the There's a hack we can use to handle such cases: The compiler would then detect the ANDing with The proposal here would need some baking (e.g., perhaps it should be |
There was a problem hiding this comment.
It would be nice if you could share some code snippets how exactly this would be used in practice.
Alternatively, I wonder if we should have a more general proposal here, something like:
[[conditional(lifetimebound, cond)]]. A general way to conditionally apply attributes. This way one could do this with any attribute not just lifetimebound.
| return true; | ||
| if (const auto *AI = D->getAttr<LifetimeBoundIfAttr>()) { | ||
| bool Result; | ||
| // FIXME: we pay the cost of evaluating the binary condition everytime we |
There was a problem hiding this comment.
Do we need to cache the evaluation or would it make sense to replace LifetimeBoundIfAttr with LifetimeBoundAttr once it evaluated to true so clients who already check for LifetimeBoundAttr does not need to be updated? Admittedly this would be a departure from what Clang AST is today, that is a really close representation of what the user wrote.
|
In addition to the concerns raised above, I also wanted to highlight that as soon as we have the ability to inspect attributes (and I agree with @higher-performance that we want it to make this useful for cases like And since lifetimebound analysis is best-effort and does not provide actual safety guarantees (has both false positives and false negatives), it'd be nice to ensure the design does not allow it to slip past the Summing up, I would actually propose to have a slightly weird version of the "getters" that would only be usable in those // Terribly name for the attribute, could be better.
template<class... Args>
reference emplace_back(Args&&... args [[clang::lifetimebound_if_lifetimebound(*new T(std::forward<Args>(args)...)))]]);if we want more conditions, we could additionally allow some conditional expressions: // Slightly confusing syntax, could definitely be better.
template<class... Args>
reference emplace_back(Args&&... args [[clang::lifetimebound_if_lifetimebound_and(*new T(std::forward<Args>(args)...)), std::is_reference_type_v(args))]]);More generally, my call is to figure out how to not leak the results of this analysis to the outside world to avoid the Hyrum's law effect where we cannot change it. I am also unsure about the granularity here. For See https://gcc.godbolt.org/z/boPeheMsK #include <memory>
struct X {
X(const int &a, [[clang::lifetimebound]] const int&b);
};
void foo() {
int a = 1;
X x(a, 2); // warning.
X y(1, a); // no warning.
// make_unique would have both warnings, right?
// similarly, for emplace_back.
auto px = std::make_unique<X>(a, 2);
auto py = std::make_unique<X>(1, a);
}Is there a way to avoid it? |
|
Would the alternative that is discussed here mean we want to forward some attributes only when they are applicable, something like Regardless, I think it'd be also good to agree on the use cases we want to support in a concrete code snippet (that should also answer to @Xazax-hun 's question). There could also be a question of whether we really should try to support variadic templates in the current lifetime-bound too, because we're aware that the current semantic itself has a limitation. |
yes, something like that. And it gets a little tricky, because it's not that we want to forward only attributes from one decl to another, it's many decls (forwarding function itself and its parameters) to many decls (function we're forwarding too and its parameters). If we start talking about |
|
w.r.t. ABI, I think the end state here would conceptually be most likely be similar to w.r.t. False positives due to annotating the entire pack -- that's a great point. I think it's avoided by the |
I believe it does not as it only produces warnings. However, having an way to query the presence of attributes in the code would allow for things like overloading or template specializations based on lifetimes. Passing information to compiler from code seems fine, it's passing the information from compiler back to the code that gives me a pause: template <bool IsLifetimebound>
struct Foo {
static void call() {
if (IsLifetimebound)
std::cerr << "I am being called from a function that passes temporaries\n";
}
using type = std::conditional_t<IsLifetimebound, int, double>;
};
template <class T>
struct SomeClass {
void some_method(const T& a [[clang::lifetimebound]]) {
// Results of the program below in Clang and GCC are different.
// Not sure if C++ standard is okay with this.
using MyFoo = Foo<__is_lifetimebound(a)>;
MyFoo::call();
std::vector<MyFoo::type> vec;
// ...
}
};There are other ways to achieve that and there are attributes that may change behavior of the program and its meaning, so maybe some folks feel it is acceptable. I feel this is probably a bad idea and we should stick to the principle that attributes like lifetimebound can be safety removed without changing runtime semantics of the program.
Noexcept and lifetimebound are quite orthogonal, so I am a little unsure about the particular proposal as is. However, I am also for avoiding duplication if possible. And given that the template function bodies have to be available anyway, we could maybe even utilize them (for function that don't have // Can be taken from signature or body, e.g. from noexcept.
template<class ...Args>
void emplace_back(Args &&...args) [[clang::infered_fwd_lifetimebound_from_call]] noexcept(noexcept([[clang::infer_lifetimebound_from_here]] new T(std::forward(args)...))) {}
// Or from the body itself when there isn't noexcept
template<class ...Args>
void emplace_back(Args &&...args) [[clang::infered_lifetimebound_from_call]] {
[[clang::infer_lifetimebound_from_here]] my_vector.push_back(T(std::forward(args)...)));
}
// Presumably we could also have some default inferences rules, e.g.
// - we infer lifetimebound from a single call either in noexcept or inside the body mentioning that argument.
// - if there's no such call or more than one mention of an argument, one has to mark the call with a special attribute.
// - std::forward/static_cast<T&&> is ignored somehow. |
One question is whether the reflection proposal would address something like this. If it does, it might make more sense to invest in that than a custom solution. |
Ahh I see what you mean. Yeah, as you mentioned I think that cat is already out of the bag with attributes like That said, we can avoid most of this problem quite easily (...in principle; perhaps not simple in implementation) I think: by preventing the query built-in from being usable anywhere outside of lifetime analysis.
Their meanings are, but their specifications seem very similar: in both cases, they are trying to say, "when diagnosing {this characteristic} of this function, do it assuming the function is equivalent to {this code}". That said -- I do agree I'm not 100% on board with using
If we could utilize the actual body, that would be perfect. However... would it be feasible to make this work for would seem to make it pretty difficult for the compiler to analyze what the lifetime relationship to the return value is. Here's another idea though, combining all of the suggestions above (including @kinu's suggestion): if we forego trying to avoid code duplication, this seems like it could work: The semantics here would be that the call to the function has the same lifetime characteristics as or, if Thoughts? |
That seems like a great long-term direction, but the amount of investment between the two is also vastly different. I think we can either have something like an attribute now, or wait quite a long time for the reflection proposal to flesh out and be implemented. I think the reality is that this particular PR is only exploring a solution we can get relatively quickly and with relatively little effort. (Which also puts a burden on it to not cause too much maintenance cost in the long term, obviously, we don't want a quick hack that doesn't really scale or is too expensive in the long run).
Definitely agree, it's quite a nuanced topic.
Unrelated, but I'd be curious to see those if you have any examples. I thought that maybe SFINAE could cause this, but at least in simple examples
I definitely have the same concerns, every single analysis makes things more and more chatty and I already feel we're close to a tipping point where annotating things becomes too hard. FWIW, it would be great to get something that "magically" works with a single attribute and does not need complicated compile-time computations. I believe @hokein was about to prepare a list of interesting code examples that we want to support. It would probably be great to that and see how many we can cover with various options, it should help us make a more informed decision. |
The closest universal solution I can think of is something like the following, assuming we do proper bikeshedding for the name: The faux body could then be used for diagnosis purposes -- and the contract would be that it would never affect codegen. This would also leave the door open to further extensions in the future. e.g.,
https://godbolt.org/z/xozfs18Ta (but looks like the Note that any attribute that can result in template instantiations could cause this. (Not sure if that's a necessary condition, but it's sufficient.) |
|
Just a friendly bump to figure out how to proceed here. Does the |
|
It feels like the current direction & discussion is expanding into a broader problem space beyond the specific issue this PR aims to address. We have two major problems which seem to be orthogonal:
The prototype here primarily targets problem (1). While As the name indicates,
These differences might cause confusion for users. While they may not be a major issue, perhaps a more precise name could help clarify the intended behavior. I'm starting to feel that we’re introducing more and more builtins to address a specific issue, which doesn’t seem ideal or scalable. That said, I don’t have a better alternative at the moment. Problem (2) is a known limitation of the current |
This case is supported as well, https://godbolt.org/z/KKsvd8Kx1. |
Some simple examples from Abseil. When using
|
|
Another thought for supporting the The underlying implementation of the For example, consider an instantiated template (note that the function parameter should be an non-const rvalue reference): construct(std::string_view* p, std::string&& arg [[clang::lifetime_capture_by(p)]]);Here, we annotate construct(&view, std::string()); // Detects dangling pointerHowever, this approach doesn’t work for Potential Extension:We could consider extending the analysis scope for non-const rvalue references. Specifically if a non-const rvalue reference parameter is annotated, we could always emit a warning when this function is being called. Example: void add(std::vector<std::string_view>& container, std::string&& s [[clang::lifetime_capture_by]]);
void test() {
std::vector<std::string_view> abc;
add(abc, std::string()); // Case 1: Warning, point to a dead object
std::string b;
add(abc, std::move(b)); // Case 2: point to a moved-from object
add(abc, b); // invalid c++ code, cannot bind a lvalue to rvalue reference
}For non-const rvalue reference function parameters, there are only two legal options to pass the argument:
If we extend the analysis to warn on Case (2), we should be able to detect the |
|
A moved from object could be reinitalized: That being said, maybe this is rare enough that we could have an opt-in warning. But we definitely cannot have something that is on by default. |
I think we are primarily concerned with pointers or references to a moved-from object. When the moved-from object is reinitialized, using a pointer to it can be very tricky and is likely undefined behavior, https://godbolt.org/z/Wh5vKe8z6 |
5 participants
This WIP PR explores the idea of introducing
[[clang::lifetimebound_if(<bool expression>)]], a built-in attribute that conditionally applies[[clang::lifetimebound]]based on the given boolean expression.One of the key challenges in adopting
[[clang::lifetimebound]]and[[clang::lifetime_capture_by]]is that these attributes should only apply to pointer-like types. Currently, we handle this by introducing multiple overloads, which increases code complexity and compilation overhead.This new attribute aims to simplify the implementation by enabling conditional application, reducing the need for overloads.
cc @usx95 @Xazax-hun @ilya-biryukov @higher-performance @kinu