Until now, our kernel has reimplemented a number of AK classes to
provide automatic internal locking:
- RefPtr
- NonnullRefPtr
- WeakPtr
- Weakable
This patch renames the Kernel classes so that they can coexist with
the original AK classes:
- RefPtr => LockRefPtr
- NonnullRefPtr => NonnullLockRefPtr
- WeakPtr => LockWeakPtr
- Weakable => LockWeakable
The goal here is to eventually get rid of the Lock* classes in favor of
using external locking.
This makes it an error to not do something with a returned smart
pointer, which should help prevent mistakes. In cases where you do need
to ignore the value, casting to void will placate the compiler.
I did have to add comments to disable clang-format on a couple of lines,
where it wanted to format the code like this:
```c++
private : NonnullRefPtr() = delete;
```
This isn't a complete conversion to ErrorOr<void>, but a good chunk.
The end goal here is to propagate buffer allocation failures to the
caller, and allow the use of TRY() with formatting functions.
While I was working on LibWeb, I got a page fault at 0xe0e0e0e4.
This indicates a destroyed RefPtr if compiled with SANITIZE_PTRS
defined. However, the page fault handler didn't print out this
indication.
This makes the page fault handler print out a note if the faulting
address looks like a recently destroyed RefPtr, OwnPtr, NonnullRefPtr,
NonnullOwnPtr, ThreadSafeRefPtr or ThreadSafeNonnullRefPtr. It will
only do this if SANITIZE_PTRS is defined, as smart pointers don't get
scrubbed without it being defined.
Some time ago, automatic locking was added to the AK smart pointers to
paper over various race conditions in the kernel. Until we've actually
solved the issues in the kernel, we're stuck with the locking.
However, we don't need to punish single-threaded userspace programs with
the high cost of locking. This patch moves the thread-safe variants of
RefPtr, NonnullRefPtr, WeakPtr and RefCounted into Kernel/Library/.
Our existing implementation did not check the element type of the other
pointer in the constructors and move assignment operators. This meant
that some operations that would require explicit casting on raw pointers
were done implicitly, such as:
- downcasting a base class to a derived class (e.g. `Kernel::Inode` =>
`Kernel::ProcFSDirectoryInode` in Kernel/ProcFS.cpp),
- casting to an unrelated type (e.g. `Promise<bool>` => `Promise<Empty>`
in LibIMAP/Client.cpp)
This, of course, allows gross violations of the type system, and makes
the need to type-check less obvious before downcasting. Luckily, while
adding the `static_ptr_cast`s, only two truly incorrect usages were
found; in the other instances, our casts just needed to be made
explicit.
And also try_create<T> => try_make_ref_counted<T>.
A global "create" was a bit much. The new name matches make<T> better,
which we've used for making single-owner objects since forever.
Aggregate initialization with brace-enclosed parameters is a
[C++20 feature][1] not yet implemented by Clang. This caused compile
errors if we tried to use the factory functions to create smart pointers
to aggregates.
As a (temporary) fix, [the LWG's previously proposed solution][2] is
implemented by this commit.
Now, wherever it's not possible to direct-initialize, aggregate
initialization is performed.
[1]:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0960r3.html
[2]: http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-active.html#2089
This attribute tells compilers that the pointer returned by a function
is never null, which lets it optimize away null checks in some places.
This seems like a nice addition to `NonnullOwnPtr` and `NonnullRefPtr`.
Using this attribute causes extra UBSan checks to be emitted. To offset
its performance loss, some additional methods were marked ALWAYS_INLINE,
which lets the compiler optimize duplicate checks
These functions abstract away the need to call the proper new operator
("throwing" or "non-throwing") and manually adopt the resulting raw
pointer. Modelled after the existing `NonnullOwnPtr<T> make()`
functions, these forward their parameters to the object's constructor.
Note: These can't be used in the common "factory method" idiom, as
private constructors can't be called from a standalone function.
The naming is consistent with AK's and Shell's previous implementation
of these:
- `make` creates a `NonnullOwnPtr<T>` and aborts if the allocation could
not be performed.
- `try_make` creates an `OwnPtr<T>`, which may be null if the allocation
failed.
- `create` creates a `NonnullRefPtr<T>`, and aborts on allocation
failure.
- `try_create` creates a `RefPtr<T>`, which may be null if the
allocation was not successful.
This enables us to use keys of type NonnullRefPtr in HashMaps and
HashTables.
This commit also includes fixes in various places that used
HashMap<T, NonnullRefPtr<U>>::get() and expected to get an
Optional<NonnullRefPtr<U>> and now get an Optional<U*>.
SPDX License Identifiers are a more compact / standardized
way of representing file license information.
See: https://spdx.dev/resources/use/#identifiers
This was done with the `ambr` search and replace tool.
ambr --no-parent-ignore --key-from-file --rep-from-file key.txt rep.txt *
Alot of code is shared between i386/i686/x86 and x86_64
and a lot probably will be used for compatability modes.
So we start by moving the headers into one Directory.
We will probalby be able to move some cpp files aswell.
(...and ASSERT_NOT_REACHED => VERIFY_NOT_REACHED)
Since all of these checks are done in release builds as well,
let's rename them to VERIFY to prevent confusion, as everyone is
used to assertions being compiled out in release.
We can introduce a new ASSERT macro that is specifically for debug
checks, but I'm doing this wholesale conversion first since we've
accumulated thousands of these already, and it's not immediately
obvious which ones are suitable for ASSERT.
Arbitrarily split up to make git bisect easier.
These unnecessary #include's were found by combining an automated tool (which
determined likely candidates) and some brain power (which decided whether
the #include is also semantically superfluous).
Problem:
- `typedef` is a keyword which comes from C and carries with it old
syntax that is hard to read.
- Creating type aliases with the `using` keyword allows for easier
future maintenance because it supports template syntax.
- There is inconsistent use of `typedef` vs `using`.
Solution:
- Use `clang-tidy`'s checker called `modernize-use-using` to update
the syntax to use the newer syntax.
- Remove unused functions to make `clang-tidy` happy.
- This results in consistency within the codebase.
This makes most operations thread safe, especially so that they
can safely be used in the Kernel. This includes obtaining a strong
reference from a weak reference, which now requires an explicit
call to WeakPtr::strong_ref(). Another major change is that
Weakable::make_weak_ref() may require the explicit target type.
Previously we used reinterpret_cast in WeakPtr, assuming that it
can be properly converted. But WeakPtr does not necessarily have
the knowledge to be able to do this. Instead, we now ask the class
itself to deliver a WeakPtr to the type that we want.
Also, WeakLink is no longer specific to a target type. The reason
for this is that we want to be able to safely convert e.g. WeakPtr<T>
to WeakPtr<U>, and before this we just reinterpret_cast the internal
WeakLink<T> to WeakLink<U>, which is a bold assumption that it would
actually produce the correct code. Instead, WeakLink now operates
on just a raw pointer and we only make those constructors/operators
available if we can verify that it can be safely cast.
In order to guarantee thread safety, we now use the least significant
bit in the pointer for locking purposes. This also means that only
properly aligned pointers can be used.
If these methods get inlined, the compiler is able to statically eliminate most
of the assertions. Alas, it doesn't realize this, and believes inlining them to
be too expensive. So give it a strong hint that it's not the case.
This *decreases* the kernel binary size.
This stopped working quite some time ago due to Clang losing track of
typestates for some reason and everything becoming "unknown".
Since we're primarily using GCC anyway, it doesn't seem worth it to try
and maintain this non-working experiment for a secondary compiler.
Also it doesn't look like the Clang team is actively maintaining this
flag anyway. So good-bye, -Wconsumed. :/
We were allowing this dangerous kind of thing:
RefPtr<Base> base;
RefPtr<Derived> derived = base;
This patch changes the {Nonnull,}RefPtr constructors so this is no
longer possible.
To downcast one of these pointers, there is now static_ptr_cast<T>:
RefPtr<Derived> derived = static_ptr_cast<Derived>(base);
Fixing this exposed a ton of cowboy-downcasts in various places,
which we're now forced to fix. :^)
The generic swap() is not able to swap a NonnullRefPtr with itself,
due to its use of a temporary and NonnullRefPtr asserting when trying
to move() from an already move()'d instance.
Given the following situation:
struct Object : public RefCounted<Object> {
RefPtr<Object> parent;
}
NonnullRefPtr<Object> object = get_some_object();
object = *object->parent;
We would previously crash if 'object' was the only strongly referencing
pointer to 'parent'. This happened because NonnullRefPtr would unref
the outgoing pointee before reffing the incoming pointee.
This patch fixes that by implementing NonnullRefPtr assignments using
pointer swaps, just like RefPtr already did.
As suggested by Joshua, this commit adds the 2-clause BSD license as a
comment block to the top of every source file.
For the first pass, I've just added myself for simplicity. I encourage
everyone to add themselves as copyright holders of any file they've
added or modified in some significant way. If I've added myself in
error somewhere, feel free to replace it with the appropriate copyright
holder instead.
Going forward, all new source files should include a license header.
Since NonnullRefPtr and NonnullOwnPtr cannot be null, it is pointless
to convert them to a bool, since it would always be true.
This patch makes it an error to null-check one of these pointers.
Use AK::exchange() to switch out the internal storage. Also mark these
functions with [[nodiscard]] to provoke an compile-time error if they
are called without using the return value.
This gives us much better error messages when you try to use them.
Without this change, it would complain about the absence of functions
named ref() and deref() on RefPtr itself. With it, we instead get a
"hey, this function is deleted" error.
Change operator=(T&) to operator=T(const T&) also, to keep assigning
a const T& to a NonnullRefPtr working.
Clang loses the typestate when passing NonnullRefPtr's via lambda captures.
This is unfortunate, but not much we can do about it. Allowing ptr() makes
it possible to use captured NonnullRefPtrs as you'd expect.
Add an "ElementType" typedef to NonnullOwnPtr and NonnullRefPtr to allow
clients to easily find the pointee type. Then use this to remove a template
argument from NonnullPtrVector. :^)
We shouldn't allow constructing e.g an OwnPtr from a RefPtr, and similar
conversions. Instead just delete those functions so the compiler whines
loudly if you try to use them.
This patch also deletes constructing OwnPtr from a WeakPtr, even though
that *may* be a valid thing to do, it's sufficiently weird that we can
make the client jump through some hoops if he really wants it. :^)
This patch removes copy_ref() from RefPtr and NonnullRefPtr. This means that
it's now okay to simply copy these smart pointers instead:
- RefPtr = RefPtr // Okay!
- RefPtr = NonnullRefPtr // Okay!
- NonnullRefPtr = NonnullRefPtr // Okay!
- NonnullRefPtr = RefPtr // Not okay, since RefPtr can be null.
- Delete the default constructor instead of just making it private.
It's never valid to create an empty NonnullRefPtr.
- Add copy assignment operators. I originally omitted these to force use
of .copy_ref() at call sites, but the hassle/gain ratio is minuscule.
- Allow calling all the assignment operators in all consumable states.
This codifies that it's okay to overwrite a moved-from NonnullRefPtr.
