Skip to content

Commit

Permalink
fix lifetime of cvk_entry_point (#721)
Browse files Browse the repository at this point in the history
cvk_entry_point are passed to other structures as a pointer. But it
can be deleted at any point when do_build it called.

To avoid invalid access, make entry point a shared_ptr in the program
map.

This has been detected running the compiler suite of the CTS on
ChromeOS (flaky behaviour).
  • Loading branch information
rjodinchr authored Nov 26, 2024
1 parent 1c378c2 commit adba303
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 15 deletions.
8 changes: 4 additions & 4 deletions src/kernel.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ struct cvk_kernel : public _cl_kernel, api_object<object_magic::kernel> {

std::mutex m_lock;
cvk_program_holder m_program;
cvk_entry_point* m_entry_point;
std::shared_ptr<cvk_entry_point> m_entry_point;
std::string m_name;
std::vector<kernel_argument> m_args;
std::shared_ptr<cvk_kernel_argument_values> m_argument_values;
Expand All @@ -177,7 +177,7 @@ using cvk_kernel_holder = refcounted_holder<cvk_kernel>;

struct cvk_kernel_argument_values {

cvk_kernel_argument_values(cvk_entry_point* entry_point)
cvk_kernel_argument_values(std::shared_ptr<cvk_entry_point> entry_point)
: m_entry_point(entry_point), m_is_enqueued(false),
m_args(m_entry_point->args()), m_pod_arg(nullptr),
m_kernel_resources(m_entry_point->num_resource_slots()),
Expand All @@ -203,7 +203,7 @@ struct cvk_kernel_argument_values {
}

static std::shared_ptr<cvk_kernel_argument_values>
create(cvk_entry_point* entry_point) {
create(std::shared_ptr<cvk_entry_point> entry_point) {
auto val = std::make_shared<cvk_kernel_argument_values>(entry_point);

if (!val->init()) {
Expand Down Expand Up @@ -416,7 +416,7 @@ struct cvk_kernel_argument_values {
}

std::mutex m_lock;
cvk_entry_point* m_entry_point;
std::shared_ptr<cvk_entry_point> m_entry_point;
std::unique_ptr<std::vector<uint8_t>> m_pod_data;
bool m_is_enqueued;
const std::vector<kernel_argument>& m_args;
Expand Down
14 changes: 6 additions & 8 deletions src/program.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1731,28 +1731,26 @@ cvk_entry_point::cvk_entry_point(cvk_device* dev, cvk_program* program,
TRACE_CNT(descriptor_set_allocated_counter, 0);
}

cvk_entry_point* cvk_program::get_entry_point(std::string& name,
cl_int* errcode_ret) {
std::shared_ptr<cvk_entry_point>
cvk_program::get_entry_point(std::string& name, cl_int* errcode_ret) {
std::lock_guard<std::mutex> lock(m_lock);

// Check for existing entry point in cache
if (m_entry_points.count(name)) {
*errcode_ret = CL_SUCCESS;
return m_entry_points.at(name).get();
return m_entry_points.at(name);
}

// Create and initialize entry point
cvk_entry_point* entry_point =
new cvk_entry_point(m_context->device(), this, name);
std::shared_ptr<cvk_entry_point> entry_point =
std::make_shared<cvk_entry_point>(m_context->device(), this, name);
*errcode_ret = entry_point->init();
if (*errcode_ret != CL_SUCCESS) {
delete entry_point;
return nullptr;
}

// Add to cache for reuse by other kernels
m_entry_points.insert(
{name, std::unique_ptr<cvk_entry_point>(entry_point)});
m_entry_points.insert({name, entry_point});

return entry_point;
}
Expand Down
6 changes: 3 additions & 3 deletions src/program.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -799,8 +799,8 @@ struct cvk_program : public _cl_program, api_object<object_magic::program> {

const VkPipelineCache& pipeline_cache() const { return m_pipeline_cache; }

CHECK_RETURN cvk_entry_point* get_entry_point(std::string& name,
cl_int* errcode_ret);
CHECK_RETURN std::shared_ptr<cvk_entry_point>
get_entry_point(std::string& name, cl_int* errcode_ret);

bool create_module_constant_data_buffer() {
cl_int err;
Expand Down Expand Up @@ -901,7 +901,7 @@ struct cvk_program : public _cl_program, api_object<object_magic::program> {
std::string m_build_log;
std::vector<cvk_sampler_holder> m_literal_samplers;
VkPushConstantRange m_push_constant_range;
std::unordered_map<std::string, std::unique_ptr<cvk_entry_point>>
std::unordered_map<std::string, std::shared_ptr<cvk_entry_point>>
m_entry_points;
std::vector<uint32_t> m_stripped_binary;
VkPipelineCache m_pipeline_cache;
Expand Down

0 comments on commit adba303

Please sign in to comment.