[RFC] Add a class attribute [[clang::null_after_move]] for use-after-move analysis

Clang Frontend
,
1

This RFC is based on a suggestion by Martin Brænne (@martinboehme)

TL;DR

I propose a new Clang annotation [[clang::null_after_move]] to mark user-defined smart-pointer-like types that make guarantees on the state of a moved-from object, leaving it in a valid and specified state where it is safe to use but not dereference. The primary motivation is to avoid false positives in the clang-tidy check bugprone-use-after-move.

Motivation

It is desirable to detect use-after-move bugs through static analysis and clang-tidy has a check for this: bugprone-use-after-move. However, there are cases in which an object may be used after it has been moved from, that cannot readily be identified by the automatic check.

Notably, user-defined smart-pointer-like types typically make the same guarantees about the state of a moved-from object as standard smart pointer types (e.g. std::unique_ptr): Moving from such a smart pointer leaves it in a valid and specified state that matches the default constructed state, nullptr. In that state it is safe to perform many operations on the pointer, such as comparing it against nullptr or other pointers; the only operations that are not allowed are the ones that dereference the pointer, e.g. operator*, operator-> or operator[].

The bugprone-use-after-move check already recognizes the semantics of smart pointers defined in the standard and allows using moved-from smart pointers as long as they are not dereferenced. The set of types that this applies to is currently hardcoded. This RFC proposes an attribute to annotate user-defined smart-pointer types so that they receive the same treatment.

Example

As a real-world example, consider Chromium’s HeapArray, a replacement for std::unique_ptr<T[]> that keeps track of its size. Its move-from semantics are specified: it leaves the move-from object empty and containing no allocated memory. Despite it, the next example:

1 void TestFunction() {
2   auto buffer = base::HeapArray<int>::WithSize(20);
3   auto moved_to = std::move(buffer);
4   std::cout << "moved from buffer size: " << buffer.size() << "\n"; // moved from buffer size: 0
5 }

Will generate a false positive use-after-move warning:

test.cc:4:46: warning: 'buffer' used after it was moved [bugprone-use-after-move]
    4 |   std::cout << "moved from buffer size: " << buffer.size() << "\n";
      |                                              ^
test.cc:3:19: note: move occurred here
    3 |   auto moved_to = std::move(buffer);
      |                   ^

Proposal

This RFC proposes a new attribute, [[clang::null_after_move]], for type declarations of smart-pointer-like classes with well-defined moved-from semantics compatible with standard smart pointers.

Because this attribute is designed to be flexible, it requires only that the logical state of the object is equivalent to nullptr after move. This allows the attribute to support types with multiple internal fields or encapsulated states that do not explicitly expose a nullptr interface, such as Chromium’s HeapArray referenced in the “Motivation” section.

Placing the attribute on a class signifies that a moved-from object is left in a valid and a specified state that is safe for use as long as it is not dereferenced. As with standard smart pointers, only operator*, operator-> and operator[] are considered unsafe accesses as they would be dereferencing a nullptr.

Once the attribute is available, it can be used by the bugprone-use-after-move check to avoid false positives on benign uses of smart-pointer-like types.

Alternatives considered

Provide a new tidy option

Provide a new tidy option (e.g. NullAfterMoveTypes) for bugprone-use-after-move, where the user can configure classes with specified move-from semantics that are compatible with standard smart pointers.

This option has been discarded because it doesn’t scale well. For big codebases, it’s not a great user experience having to add the type to a central location that may be far away from the code that actually implements the type. Seems a better solution to add the annotation directly on the type and have it be visible there.

Teach the static analyzer the smart pointer semantics

The check bugprone-use-after-move could deduce through static analysis that the object is default-constructed after the move. This analysis may be error-prone (leading to false negatives) and likely involve a too costly runtime for a clang-tidy check.

And, even if it can be deduced through static analysis that the object is default-constructed after the move, it’s impossible to know whether it is an intentional part of the API contract the users of the class can rely on, or it is an implementation detail subject to change, and use-after-move should be flagged, even if the object happens to be left in default-constructed state at the moment of the check.

Use a Function attribute

The current proposal is to annotate declaration types. Although there has been a conversation on whether annotating individual operations could be a better option.

Annotating operations involve solving the question: are the safe operations annotated? or the ones that aren’t safe? Both options come with some difficulty:

  • Annotating the operations that are safe means annotating quite a lot of operations because the majority of operations on a moved-from smart pointer are expected to be safe.

  • Annotating the operations that aren’t safe requires fewer annotations, but it’s a bit strange in that the default is for an operation to be unsafe after a move.

At this point the most straightforward approach here seems to be still to annotate the class.

Use two attributes

Use a combination of 2 attributes to overcome the difficulties of using just a function attribute:

  • A class attribute (e.g. [[clang::specified-after-move]]) to mark user defined types as left in a specified state after move, and therefore allowed to be used.
  • A function attribute (e.g. [[clang::unsafe-after-move]]) to overrule the class attribute and mark methods as unsafe to use after move. Note that the attribute would not have effect in types without the first attribute.

The 2 attributes solution is clearer on the semantics and more flexible, allowing to annotate types beyond smart-pointer-like types. But it comes with the disadvantages of adding 2 new attributes to Clang, whose interaction must be understood, and more annotations to user-code.

The disadvantages outweighs the flexibility of the solution for the problem this RFC is trying to solve, which is specific to smart-pointer-like user defined types, where unsafe dereferences are expected to happen through well-known operators in most of the cases.

Name of the attribute

There has been an open discussion regarding the name of the attribute, with several options in the mix:

  • [[clang::default_constructed_after_move]]: The original proposed name, oriented to be applied to smart-pointer-like types that generally leave the object default constructed after moves. Discarded because it fails to clearly communicate the specified state of the pointer type (nullptr).

  • [[clang::usable_after_move]]: Considered too vague as any moved-from object is arguably usable, in that it is in a valid but unspecified state.

  • [[clang::specified_after_move]]: An alternative to convey more clearly that the moved-from state is not just guaranteed to be valid but is precisely defined, although fails to communicate what is that specified state, leaving unclear if the tidy check can warn on an attempt to dereference the smart pointer.

2

I’m coming down on “this is too specific” of an attribute. I’d like us to step back a bit and figure out a more generic solution that covers more than just smart pointers. I don’t have much of an idea other than hoping others can explore/understand the space needed here for something better, but everything about this (null, move, smart ptr type) is so specific to the smart pointers, I’m concerned.

I MIGHT actually wonder if ‘dereferenceable-after-move’ (or something?) is closer to what you want, but still, not quite generic enough for me.

I think we perhaps want to provide a way for ALL classes (or at least a larger subset) to specify what ‘after move’ means to them. It is unspecified of course by standard, so what we REALLY want is a way to ‘specify’ to… something.

One could think of this as “what operations are valid/invalid”, or “what values are here”, but I don’t have a good feel of the design space to know which is more useful.

4 Likes
3

Thanks @erichkeane,

Would be the alternative of two attributes be more appealing to you?

Using 2 attributes provides a flexible framework that would allow to annotate classes beyond smart-type-like pointers. Do you think the flexibility overweight the disadvantages?

It’s a broader scope than I had in mind (for the original problem I faced), but if it is perceived as a better solution I wouldn’t mind to purse it.

4

I’m not sure? I don’t quite think that is quite right as well? But at least better? I’d very much like to avoid overly-specific-to-one-thing attributes, we have enough of those that basically never get used.

Perhaps we should see if others have ideas here.

6

That doesn’t actually work, because it’s the inverse of the semantics that a smart pointer has: When a smart pointer has been moved from, you can do anything except dereference it.

I think there’s a tension here in that, as a rule, you do want to have a general way of specifying these kinds of things, yet the only classes with a guaranteed moved-from state that I’ve seen in practice are smart pointers. So that makes me wonder – will a more general way of specifying this kind of behavior turn out to be premature generalization (aka YAGNI)?

7

So that makes me wonder – will a more general way of specifying this kind of behavior turn out to be premature generalization (aka YAGNI)?

This is of course the balance here we have to make. Just about EVERY proposal for an attribute (SA vendor or not!) ends up being VERY limited to their exact use case, and it makes it "No one but THAT person is going to use it, since it is so specific that no one else can use it”. We have a fountain of attributes that fit that description unfortunately where we got it wrong that way.

I’m pretty against anything that doesn’t solve a bigger problem. This applies to ONE situation for ONE form of structure type in ONE language mode, after ONE operation. This is getting pretty darn specific.

I’d like us to step back and come up with a more general model that we can reason about that will potentially solve/mutate into future solutions, rather than one very specific problem today.

8

FWIW, I think a name like valid_after_move may be closer to the mark if the goal is that the state is valid; sentinel but non-null values seem like a reasonable use case for example. The thrust is still the same: you cannot do a dereference operation, but anything else is fair game.

But taking a step back, this seems like it is statically known to the analyzer and we shouldn’t need any marking. Smart pointers are generally templates, so the implementation generally lives in a header file where the source is available in all TUs. So an attribute is really only needed in cases where the implementation of the smart pointer is hidden (so it would require cross-TU analysis in order to know internal state details). Correct? CC @ymand @NoQ @Xazax-hun for some additional perspectives

9

But taking a step back, this seems like it is statically known to the analyzer and we shouldn’t need any marking. Smart pointers are generally templates, so the implementation generally lives in a header file where the source is available in all TUs. So an attribute is really only needed in cases where the implementation of the smart pointer is hidden (so it would require cross-TU analysis in order to know internal state details)

@AaronBallman If I understand correctly what yo are proposing is to use static analysis to decide if the user defined type is safe to use after move. We explored that option but came to the conclusion that apart from visibility, runtime-cost and user-intention are problems that an attribute would better solve.

Kindly let me know if I misunderstood what you are proposing. Or maybe I understood, but you still think the advantages of a new attribute are not worth adding it.

1 Like
10

Right – while a static analyzer may be able determine what the state of the moved-from object actually is after the move with the current implementation, it cannot determine whether that state is guaranteed by the interface specification, or simply an implementation detail. For example, it’s “wrong” to rely on the state of a std::vector after you’ve moved from it, despite that the libc++ implementation (…and every other implementation) unconditionally clears the source object.

So, it does seem like “valid_after_move” (or something like that) would be useful to express that intent.

This part feels wrong: on types that are valid after move, there aren’t functions which are unsafe after move which aren’t also unsafe after default-init. It seems kinda weird that bugprone-use-after-move is even the diagnostic responsible for handling null-pointer deref of smart pointers in the first place…

1 Like
11

Thank you, that clarifies things for me.

12

The reason to not go with valid_after_move in the proposal is that the C++ standard library, as I understand it, places moved-from objects in a valid but unspecified state. And because the state is unspecified, bugprone-use-after-move will warn about using moved-from objects (unless they are reinitialized).

Therefore, to me, is confusing (or contradictory) that for user-defined types valid_after_move will be enough to use a moved-from object. But apart from the naming thoughts, I think we agree on the underlying idea :slight_smile:

Warning on null-dereferences is a job that arguably overlaps with something like a nullability analysis.

I think the motivation to support such dereferences in bugprone-use-after-move comes from the fact that smart pointers is one of the few types in the c++ library to specify a moved-from. And that specified state is nullptr. Hence, when bugprone-use-after-move is warning on unsafe use-after-moves of smart pointers is warning on null-dereferences. But not because the check is trying to perform nullability anlysis in a more generic sense. ping @martinboehme to keep me honest on the bugprone-use-after-move history =)

The motivation to use [[clang::null_after_move]] in the proposal is that it’s expected that user-defined-types with a specified move-from state will (most of the time) be a smart-pointer-like type, and therefore their moved-from state will be likely compatible with nullptr.

Although, given the discussions, maybe [[clang::null_after_move]] is not the best name for the attribute after all.

On my side, I welcome using another name that communicates better the intent. I find clang::valid_after_move a little confusing for the reasons given before. But I haven’t strong feelings, and I am happy to use a name that gets consensus.

thoughts?

13

Yeah I think overlap is fine.

It is much harder to build a null pointer dereference analysis machine that “naturally” finds low-level null pointer dereferences caused by use-after-move of smart pointers, than it is to find the use-after-move crash as a high-level problem. Just because the source code of the smart pointer class is available, this doesn’t mean that the tool has time to delve 5 stack frames deep into each function call it sees to possibly uncover that the pointer is null. Scalable interprocedural analysis is a fairly hard problem in practice.

Even in clang-analyzer-cplusplus.Move (which is included with exactly that kind of complex know-it-all model-everything-perfectly machine) we’re somewhat ok with the use-after-move checker emitting null pointer warnings on its own. It’s also valuable that the use-after-move checker explains the same bugs to the user in a high-level way, i.e the warning “this smart pointer is null here because it was moved there” is significantly more pleasant to process than “this raw pointer is null because it was loaded from this smart pointer over there, where it was assigned null on that line of code which was three #includes deep into the standard library”.

So in tiny self-contained tools such as bugprone-use-after-move, that don’t communicate with a massive machine that reasons about null pointer dereferences, this may be a fancy way to find a few extra bugs where the bigger more complex machine will probably fail anyway. And I’m very much into that.

14

In my moderately humble opinion, what almost every C/C++ static analysis tool really needs is a general-purpose attribute [[is_basically_a(…)]].

As in:

[[clang::is_basically_a("std::shared_ptr")]]
class QSharedPointer {
public:
  ...
  [[clang::is_basically_a("std::shared_ptr::operator bool()")]]
  bool isNull() const;
  ...
};

Or even on standalone functions in C:

[[clang::is_basically_a("malloc")]]
void *kmalloc(size_t size, int flags);

We probably want to to specify what exactly is so similar about these classes or functions:

[[clang::is_basically_a_for_the_purposes_of(
  "std::shared_ptr",          // class name
  "behavior after move"       // purpose name
)]]
class QSharedPointer { ... };

Maybe even separate them by tools to avoid inter-tool misunderstandings:

[[clang::is_basically_a_for_the_purposes_of_according_to(
  "std::shared_ptr",          // class name
  "behavior after move"       // purpose name
  "clang-tidy",               // tool namespace (hopefully unique)
  "bugprone-use-after-move"   // sub-tool or checker name (optional ig)
)]]
class QSharedPointer { ... };

(Though FWIW I wouldn’t mind it if brand-new tools responded to attributes added for older tools long before their get their own annotations.)

Last time I ranted about this in [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives by BaLiKfromUA · Pull Request #144313 · llvm/llvm-project · GitHub (about the FlowSensitive optionals checker handling custom optionals) where I brought up use-after-move as the biggest example of this problem, that could be solved by that kind of annotation (but far from the only example).