llvm / llvm/llvm-project

Feature request: annotation for maybe deleted this [clang:temporal-safety]

Open
#206,472 6 comments 0 reactions 0 assignees View on GitHub
clang:temporal-safety
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

In Chromium network code, it is common to have a method like `HandleMessage()` that may synchronously send a response message. Sending a response message may fail if there is no longer a route to the remote host, in which case cleanup code may have run and deleted the current object.

We generally handle this in one of two ways

1) Use a weak pointer to keep track of whether `this` is still valid, or
2) Since 1) is expensive, in simpler cases just have a comment saying `// this may be deleted here` after the call.

For the second case, it would be useful to have an annotation instead of a comment so that that the compiler can check that members are not accessed afterwards. For example:
```
HandleMessage(message);
[[this_maybe_deleted]];
return;
```
If someone then added some code before `return` that relied on `this`, the compiler could issue a warning.

Another issue is that another method further up the stack may not be aware of the comment and may add code that touches `this`. To help prevent this, the annotation would also be useful as an attribute on function declarations, eg.
```
[[this_maybe_deleted]] void ReadThenHandleMessage();
```
Then code like
```
ReadThenHandleMessage();
RecordHistogram("Net.MessageCount", message_count_);
```
would also issue a warning.

Further enhancements could issue a warning if `ReadThenHandleMessage()` calls `HandleMessage()` which has the `[[this_maybe_deleted]]` annotation but `ReadThenHandleMessage()` itself doesn't have the annotation. Calling such a method from the destructor could also issue a warning.

Another possible enhancement would be to warn on incorrect use of the annotation. For example,
```
HandleMessage();
LOG(VERBOSE) << "Handled message with id " << last_id_;
[[this_maybe_deleted]];
```
is a bug which could be caught, assuming we can see inside the LOG macro to see that it will never delete `this`.

A common pattern in Chromium IPC code is to have an IPC handler which is owned by a factory object, and to call a method on the factory object to request deletion, something like:
```
factory_->RemoveHandler(this);
// this is deleted here.
```
In this case, `this` is always deleted, but it's probably not necessary to have a separate annotation for this case, as the warnings that need to be given are the same.

Contributor guide

Open the contributing guide

Research direction

No implementation file, test, or entry point is named. Start by reviewing the existing clang temporal-safety work and how Clang represents declaration attributes and statement annotations, then determine the diagnostic and propagation rules needed for the examples in the issue. Done means the annotation design is implemented with diagnostics and tests covering the listed use cases.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
28/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.