Skip to content

Commit 5c6405e

Browse files
committed
Fix memory leak caused by accessing out-of-scope initializer_list
AddressSanitizer reported a memory leak in `expression_definition` due to the dependency list being stored as a `std::initializer_list`, which points to a temporary array on the caller’s stack. Once the temporary goes out of scope, the stored begin/end pointers become invalid, leading to stray reads when iterating over dependencies. - Replaced the `std::initializer_list` member with a `std::vector` to ensure ownership of the data is maintained - Introduced `counter_data` classes to distinguish between expression and hardware counters at runtime, avoiding memory allocation for both types when only one is needed - Added helper getter functions for easier data access based on counter type - Enabled ASAN for the unit test executables to improve memory issue detection in the future
1 parent e4cfb55 commit 5c6405e

File tree

12 files changed

+150
-112
lines changed

12 files changed

+150
-112
lines changed

.clang-tidy

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,7 @@ Checks: >-
1818
cppcoreguidelines-pro-type-member-init,
1919
cppcoreguidelines-pro-type-reinterpret-cast
2020
WarningsAsErrors: '*'
21-
HeaderFilterRegex: '^.*/hwcpipe2/(device|debug|test)/.*\.hpp$'
22-
AnalyzeTemporaryDtors: false
21+
HeaderFilterRegex: '^.*/github/(hwcpipe|test|examples)/.*\.hpp$'
2322
FormatStyle: file
2423
CheckOptions:
2524
- key: llvm-else-after-return.WarnOnConditionVariables

.github/workflows/android-clang.yaml

Lines changed: 0 additions & 32 deletions
This file was deleted.

.github/workflows/linux-clang.yaml

Lines changed: 0 additions & 22 deletions
This file was deleted.

.github/workflows/linux-gcc.yaml

Lines changed: 0 additions & 22 deletions
This file was deleted.

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,6 @@ graphics development or technology please submit them on the [Arm Community grap
5555

5656
- - -
5757

58-
_Copyright © 2023-2024, Arm Limited and contributors. All rights reserved._
58+
_Copyright © 2023-2025, Arm Limited and contributors. All rights reserved._
5959

6060
[1]: https://community.arm.com/support-forums/f/graphics-gaming-and-vr-forum/

backend/.pre-commit-config.yaml

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
# See https://pre-commit.com for more information
2+
# See https://pre-commit.com/hooks.html for more hooks
3+
4+
minimum_pre_commit_version: 2.9.0
5+
6+
exclude: "device/src/uapi/.*"
7+
repos:
8+
# global hooks
9+
- repo: https://github.com/pre-commit/pre-commit-hooks
10+
rev: v4.4.0
11+
hooks:
12+
- id: trailing-whitespace
13+
- id: end-of-file-fixer
14+
exclude: "third_party/.*/vendor/.*"
15+
- id: check-ast
16+
- id: check-json
17+
- id: check-yaml
18+
args: [--unsafe]
19+
- id: check-added-large-files
20+
exclude: "third_party/.*/vendor/.*"
21+
- id: check-merge-conflict
22+
- id: debug-statements
23+
- id: mixed-line-ending
24+
exclude: "third_party/.*/vendor/.*"
25+
- repo: https://github.com/PeterMosmans/jenkinslint
26+
rev: v1.0.0
27+
hooks:
28+
- id: jenkinslint
29+
files: Jenkinsfile
30+
- repo: https://github.com/alessandrojcm/commitlint-pre-commit-hook
31+
rev: v9.5.0
32+
hooks:
33+
- id: commitlint
34+
stages: [commit-msg]
35+
additional_dependencies: ['@commitlint/config-conventional']
36+
- repo: https://github.com/pre-commit/mirrors-prettier
37+
rev: v2.7.1
38+
hooks:
39+
- id: prettier
40+
types: [markdown]
41+
- repo: https://github.com/cheshirekow/cmake-format-precommit
42+
rev: v0.6.13
43+
hooks:
44+
- id: cmake-format
45+
- id: cmake-lint
46+
- repo: https://github.com/PyCQA/pylint
47+
rev: v3.0.0a6
48+
hooks:
49+
- id: pylint
50+
additional_dependencies: &pip-packages
51+
- pystache==0.6.0
52+
- PyYAML==6.0
53+
- types-PyYAML==6.0.7
54+
- parameterized==0.8.1
55+
- repo: https://github.com/PyCQA/flake8
56+
rev: 6.0.0
57+
hooks:
58+
- id: flake8
59+
args: ['--max-line-length=120']
60+
- repo: https://github.com/psf/black
61+
rev: 23.3.0
62+
hooks:
63+
- id: black
64+
- repo: https://github.com/pre-commit/mirrors-mypy
65+
rev: v1.3.0
66+
hooks:
67+
- id: mypy
68+
additional_dependencies: *pip-packages
69+
- repo: https://github.com/pre-commit/mirrors-clang-format
70+
rev: v16.0.4
71+
hooks:
72+
- id: clang-format
73+
- repo: https://github.com/codespell-project/codespell
74+
rev: v2.2.4
75+
hooks:
76+
- id: codespell
77+
exclude: "third_party/.*/vendor/.*"
78+
- repo: https://gitlab.gpu.arm.com/pre-commit/copyright.git
79+
rev: v1.0.0-alpha.16
80+
hooks:
81+
- id: copyright
82+
exclude: (?x)^(
83+
third_party/.*/vendor/.*
84+
|test/workload/src/workload_impl.[ch]pp
85+
)$
86+
types_or: [c, c++, python]

backend/CMakeLists.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ project(hwcpipe2)
2929
option(
3030
HWCPIPE_ENABLE_TESTS
3131
"If set, build unit tests."
32-
OFF
32+
ON
3333
)
3434
option(HWCPIPE_ENABLE_EXCEPTIONS "If set, c++ exceptions are enabled.")
3535
option(

examples/api_example.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2023-2024 Arm Limited.
2+
* Copyright (c) 2023-2025 Arm Limited.
33
*
44
* SPDX-License-Identifier: MIT
55
*/
@@ -40,6 +40,7 @@ void print_sample_value(const hwcpipe::counter_sample &sample) {
4040
std::cout << sample.value.float64;
4141
return;
4242
default:
43+
std::cout << "Unknown";
4344
return;
4445
}
4546
}

hwcpipe/include/hwcpipe/detail/internal_types.hpp

Lines changed: 36 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,14 @@
1818
#include <cstdint>
1919
#include <initializer_list>
2020
#include <system_error>
21+
#include <vector>
2122
namespace hwcpipe {
2223
namespace detail {
2324

25+
struct counter_data {
26+
virtual ~counter_data() noexcept = default;
27+
};
28+
2429
namespace expression {
2530
/**
2631
* The expression evaluation context provides an abstraction over some block of
@@ -66,7 +71,7 @@ using evaluator = double (*)(const context &);
6671
* Holds information about the expression that the sampler will need when
6772
* registering the counters and evaluating.
6873
*/
69-
struct expression_definition {
74+
struct expression_definition : public counter_data {
7075
/**
7176
* Pointer to the function that will evaluate the expression and return the
7277
* calculated result.
@@ -77,18 +82,29 @@ struct expression_definition {
7782
* be implicitly registered with the sampler so that they can be collected
7883
* when it is polled.
7984
*/
80-
const std::initializer_list<hwcpipe_counter> dependencies;
85+
const std::vector<hwcpipe_counter> dependencies;
86+
87+
expression_definition(evaluator eval, const std::initializer_list<hwcpipe_counter> dependencies)
88+
: eval(eval)
89+
, dependencies(std::move(dependencies)) {}
8190
};
8291

8392
} // namespace expression
8493
/**
8594
* Structure representing the block type/offset address and shift
8695
* scaling of a counter within a particular GPU's PMU data.
8796
*/
88-
struct block_offset {
97+
struct block_offset : public counter_data {
98+
using block_t = hwcpipe::device::hwcnt::block_type;
99+
89100
uint32_t offset;
90101
uint32_t shift;
91-
hwcpipe::device::hwcnt::block_type block_type;
102+
block_t block_type;
103+
104+
block_offset(uint32_t offset, uint32_t shift, block_t block_type)
105+
: offset(offset)
106+
, shift(shift)
107+
, block_type(block_type) {}
92108
};
93109

94110
/**
@@ -102,24 +118,29 @@ struct block_offset {
102118
struct counter_definition {
103119
enum class type { invalid, hardware, expression };
104120
type tag;
105-
union u {
106-
block_offset address{};
107-
expression::expression_definition expression;
108-
explicit u(expression::expression_definition expression)
109-
: expression(expression) {}
110-
explicit u(block_offset address)
111-
: address(address) {}
112-
} u;
121+
const std::shared_ptr<counter_data> data;
122+
123+
HWCP_NODISCARD const expression::expression_definition &get_expression() const {
124+
assert(tag == type::expression);
125+
auto const ptr = std::static_pointer_cast<const expression::expression_definition>(data);
126+
return *ptr;
127+
}
128+
129+
HWCP_NODISCARD const block_offset &get_address() const {
130+
assert(tag == type::hardware);
131+
auto const ptr = std::static_pointer_cast<const block_offset>(data);
132+
return *ptr;
133+
}
113134

114135
counter_definition()
115136
: tag(type::invalid)
116-
, u(block_offset{0, 0, device::hwcnt::block_type::fe}) {}
137+
, data(std::make_shared<block_offset>(0, 0, device::hwcnt::block_type::fe)) {}
117138
explicit counter_definition(expression::expression_definition expression)
118139
: tag(type::expression)
119-
, u(expression) {}
140+
, data(std::make_shared<expression::expression_definition>(expression)) {}
120141
explicit counter_definition(block_offset address)
121142
: tag(type::hardware)
122-
, u(address) {}
143+
, data(std::make_shared<block_offset>(address)) {}
123144
};
124145

125146
struct hwcpipe_backend_policy {

hwcpipe/include/hwcpipe/sampler.hpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -157,15 +157,15 @@ class sampler_config {
157157

158158
switch (definition.tag) {
159159
case detail::counter_definition::type::hardware: {
160-
const auto &address = definition.u.address;
160+
const auto &address = definition.get_address();
161161
counters_.emplace(counter, definition);
162162

163163
backend_config_[address.block_type].enable_map[address.offset] = 1;
164164
break;
165165
}
166166
case detail::counter_definition::type::expression: {
167167
counters_.emplace(counter, definition);
168-
ec = add_expression_depedencies(definition.u.expression);
168+
ec = add_expression_depedencies(definition.get_expression());
169169
if (ec) {
170170
return ec;
171171
}
@@ -621,14 +621,14 @@ class sampler : private detail::expression::context {
621621
counter_to_buffer_pos_[counter.counter] = buffer_pos;
622622
sample_buffer_.push_back({});
623623

624-
auto &block_pos_list = counters_by_block_map_[counter.definition.u.address.block_type];
625-
block_pos_list.emplace_back(counter.definition.u.address.offset, buffer_pos,
626-
counter.definition.u.address.shift);
624+
auto &address = counter.definition.get_address();
625+
auto &block_pos_list = counters_by_block_map_[address.block_type];
626+
block_pos_list.emplace_back(address.offset, buffer_pos, address.shift);
627627
break;
628628
}
629629
case detail::counter_definition::type::expression: {
630630
// store the function pointer to run this expression in get_counter_value()
631-
counter_to_evaluator_.emplace(counter.counter, counter.definition.u.expression.eval);
631+
counter_to_evaluator_.emplace(counter.counter, counter.definition.get_expression().eval);
632632
break;
633633
}
634634
case detail::counter_definition::type::invalid:

0 commit comments

Comments
 (0)