Skip to content
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

Support tagging driver handles with a name #8038

Merged
merged 17 commits into from
Aug 24, 2024
4 changes: 4 additions & 0 deletions filament/backend/include/private/backend/DriverAPI.inc
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,10 @@ DECL_DRIVER_API_0(finish)
// reset state tracking, if the driver does any state tracking (e.g. GL)
DECL_DRIVER_API_0(resetState)

DECL_DRIVER_API_N(setDebugTag,
backend::HandleBase::HandleId, handleId,
std::string&&, tag)
bejado marked this conversation as resolved.
Show resolved Hide resolved

/*
* Creating driver objects
* -----------------------
Expand Down
35 changes: 33 additions & 2 deletions filament/backend/include/private/backend/HandleAllocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@

#include <cstddef>
#include <exception>
#include <optional>
#include <string>
#include <type_traits>
#include <unordered_map>
#include <utility>
Expand Down Expand Up @@ -173,8 +175,10 @@ class HandleAllocator {
uint8_t const age = (tag & HANDLE_AGE_MASK) >> HANDLE_AGE_SHIFT;
auto const pNode = static_cast<typename Allocator::Node*>(p);
uint8_t const expectedAge = pNode[-1].age;
FILAMENT_CHECK_POSTCONDITION(expectedAge == age) <<
"use-after-free of Handle with id=" << handle.getId();
std::optional<std::string_view> const maybeTag = get_handle_tag(handle.getId());
bejado marked this conversation as resolved.
Show resolved Hide resolved
FILAMENT_CHECK_POSTCONDITION(expectedAge == age)
<< "use-after-free of Handle with id=" << handle.getId()
<< ", tag=" << maybeTag.value_or("(no tag)");
}
}

Expand All @@ -201,6 +205,32 @@ class HandleAllocator {
return handle_cast<Dp>(const_cast<Handle<B>&>(handle));
}

void associate_handle(HandleBase::HandleId id, std::string&& tag) noexcept {
// TODO: for now, only pool handles check for use-after-free, so we only keep tags for
// those
if (isPoolHandle(id)) {
// Truncate the tag's age to N bits.
constexpr uint8_t N = 2; // support a history of 4 tags
pixelflinger marked this conversation as resolved.
Show resolved Hide resolved
constexpr uint8_t mask = (1 << N) - 1;

uint8_t const age = (id & HANDLE_AGE_MASK) >> HANDLE_AGE_SHIFT;
uint8_t const newAge = age & mask;
uint32_t const key = (id & ~HANDLE_AGE_MASK) | (newAge << HANDLE_AGE_SHIFT);

mDebugTags[key] = std::move(tag);
bejado marked this conversation as resolved.
Show resolved Hide resolved
}
}

std::optional<std::string_view> get_handle_tag(HandleBase::HandleId id) const noexcept {
if (!isPoolHandle(id)) {
return {};
}
if (auto pos = mDebugTags.find(id); pos != mDebugTags.end()) {
return pos->second;
}
return {};
}

private:

template<typename D>
Expand Down Expand Up @@ -363,6 +393,7 @@ class HandleAllocator {
// Below is only used when running out of space in the HandleArena
mutable utils::Mutex mLock;
tsl::robin_map<HandleBase::HandleId, void*> mOverflowMap;
tsl::robin_map<HandleBase::HandleId, std::string> mDebugTags;
bejado marked this conversation as resolved.
Show resolved Hide resolved
HandleBase::HandleId mId = 0;
bool mUseAfterFreeCheckDisabled = false;
};
Expand Down
4 changes: 4 additions & 0 deletions filament/backend/src/metal/MetalDriver.mm
Original file line number Diff line number Diff line change
Expand Up @@ -2061,6 +2061,10 @@
void MetalDriver::resetState(int) {
}

void MetalDriver::setDebugTag(HandleBase::HandleId handleId, std::string&& tag) {
mHandleAllocator.associate_handle(handleId, std::move(tag));
}

void MetalDriver::runAtNextTick(const std::function<void()>& fn) noexcept {
std::lock_guard<std::mutex> const lock(mTickOpsLock);
mTickOps.push_back(fn);
Expand Down
3 changes: 3 additions & 0 deletions filament/backend/src/noop/NoopDriver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -388,4 +388,7 @@ void NoopDriver::endTimerQuery(Handle<HwTimerQuery> tqh) {
void NoopDriver::resetState(int) {
}

void NoopDriver::setDebugTag(HandleBase::HandleId handleId, std::string&& tag) {
}

} // namespace filament
4 changes: 4 additions & 0 deletions filament/backend/src/opengl/OpenGLDriver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2210,6 +2210,10 @@ void OpenGLDriver::makeCurrent(Handle<HwSwapChain> schDraw, Handle<HwSwapChain>
// Updating driver objects
// ------------------------------------------------------------------------------------------------

void OpenGLDriver::setDebugTag(HandleBase::HandleId handleId, std::string&& tag) {
mHandleAllocator.associate_handle(handleId, std::move(tag));
}

void OpenGLDriver::setVertexBufferObject(Handle<HwVertexBuffer> vbh,
uint32_t index, Handle<HwBufferObject> boh) {
DEBUG_MARKER()
Expand Down
5 changes: 5 additions & 0 deletions filament/backend/src/vulkan/VulkanDriver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2032,6 +2032,11 @@ void VulkanDriver::debugCommandBegin(CommandStream* cmds, bool synchronous, cons
void VulkanDriver::resetState(int) {
}

void VulkanDriver::setDebugTag(HandleBase::HandleId handleId, std::string&& tag) {
// TODO
// mResourceAllocator.associate_handle(handleId, tag, length);
}

// explicit instantiation of the Dispatcher
template class ConcreteDispatcher<VulkanDriver>;

Expand Down
11 changes: 11 additions & 0 deletions filament/include/filament/Texture.h
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,17 @@ class UTILS_PUBLIC Texture : public FilamentAPI {
*/
Builder& swizzle(Swizzle r, Swizzle g, Swizzle b, Swizzle a) noexcept;

/**
* Associate an optional tag with this Texture for debugging purposes.
*
* name will show in error messages and should be kept as short as possible.
* The name string must persist until build() has been called on this Builder.
*
* @param name A short string to identify this Texture
* @param len Length name tag, or 0 to compute the length if name is null-terminated
*/
Builder& name(const char* UTILS_NONNULL name, size_t len = 0) noexcept;
bejado marked this conversation as resolved.
Show resolved Hide resolved

/**
* Creates the Texture object and returns a pointer to it.
*
Expand Down
13 changes: 13 additions & 0 deletions filament/src/details/Texture.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@

#include <algorithm>
#include <array>
#include <string>
#include <type_traits>
#include <utility>

Expand Down Expand Up @@ -82,6 +83,7 @@ struct Texture::BuilderDetails {
std::array<Swizzle, 4> mSwizzle = {
Swizzle::CHANNEL_0, Swizzle::CHANNEL_1,
Swizzle::CHANNEL_2, Swizzle::CHANNEL_3 };
std::optional<std::string_view> mName;
};

using BuilderType = Texture;
Expand Down Expand Up @@ -140,6 +142,14 @@ Texture::Builder& Texture::Builder::swizzle(Swizzle r, Swizzle g, Swizzle b, Swi
return *this;
}

Texture::Builder& Texture::Builder::name(const char* name, size_t len) noexcept {
if (!name) {
return *this;
}
mImpl->mName = std::string_view(name, len == 0 ? strlen(name) : len);
return *this;
}

Texture* Texture::Builder::build(Engine& engine) {
FILAMENT_CHECK_PRECONDITION(Texture::isTextureFormatSupported(engine, mImpl->mFormat))
<< "Texture format " << uint16_t(mImpl->mFormat) << " not supported on this platform";
Expand Down Expand Up @@ -243,6 +253,9 @@ FTexture::FTexture(FEngine& engine, const Builder& builder) {
mHandle = driver.importTexture(builder->mImportedId,
mTarget, mLevelCount, mFormat, mSampleCount, mWidth, mHeight, mDepth, mUsage);
}
if (auto name = builder->mName) {
driver.setDebugTag(mHandle.getId(), std::string(name->data(), name->size()));
}
}

// frees driver resources, object becomes invalid
Expand Down
Loading