Skip to content

Commit 65432b1

Browse files
authored
introduce sanity check when creating bufferdatasink
Differential Revision: D70190912 Pull Request resolved: #8709
1 parent f9dc6ef commit 65432b1

File tree

3 files changed

+68
-13
lines changed

3 files changed

+68
-13
lines changed

devtools/etdump/buffer_data_sink.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,22 @@
1111

1212
using ::executorch::runtime::Error;
1313
using ::executorch::runtime::Result;
14+
using ::executorch::runtime::Span;
1415

1516
namespace executorch {
1617
namespace etdump {
1718

19+
Result<BufferDataSink> BufferDataSink::create(
20+
Span<uint8_t> buffer,
21+
size_t alignment) noexcept {
22+
// Check if alignment is a power of two and greater than 0
23+
if (alignment == 0 || (alignment & (alignment - 1)) != 0) {
24+
return Error::InvalidArgument;
25+
}
26+
27+
return BufferDataSink(buffer, alignment);
28+
}
29+
1830
Result<size_t> BufferDataSink::write(const void* ptr, size_t length) {
1931
if (length == 0) {
2032
return offset_;

devtools/etdump/buffer_data_sink.h

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,17 +24,20 @@ namespace etdump {
2424
class BufferDataSink : public DataSinkBase {
2525
public:
2626
/**
27-
* Constructs a BufferDataSink with a given buffer.
27+
* Creates a BufferDataSink with a given span buffer.
2828
*
2929
* @param[in] buffer A Span object representing the buffer where data will be
3030
* stored.
3131
* @param[in] alignment The alignment requirement for the buffer. It must be
32-
* a power of two. Default is 64.
32+
* a power of two and greater than zero. Default is 64.
33+
* @return A Result object containing either:
34+
* - A BufferDataSink object if succees, or
35+
* - An error code indicating the failure reason, if any issue
36+
* occurs during the creation process.
3337
*/
34-
explicit BufferDataSink(
38+
static ::executorch::runtime::Result<BufferDataSink> create(
3539
::executorch::runtime::Span<uint8_t> buffer,
36-
size_t alignment = 64)
37-
: debug_buffer_(buffer), offset_(0), alignment_(alignment) {}
40+
size_t alignment = 64) noexcept;
3841

3942
// Uncopiable and unassignable to avoid double assignment and free of the
4043
// internal buffer.
@@ -77,6 +80,19 @@ class BufferDataSink : public DataSinkBase {
7780
size_t get_used_bytes() const override;
7881

7982
private:
83+
/**
84+
* Constructs a BufferDataSink with a given buffer.
85+
*
86+
* @param[in] buffer A Span object representing the buffer where data will be
87+
* stored.
88+
* @param[in] alignment The alignment requirement for the buffer. It must be
89+
* a power of two. Default is 64.
90+
*/
91+
explicit BufferDataSink(
92+
::executorch::runtime::Span<uint8_t> buffer,
93+
size_t alignment)
94+
: debug_buffer_(buffer), offset_(0), alignment_(alignment) {}
95+
8096
// A Span object representing the buffer used for storing debug data.
8197
::executorch::runtime::Span<uint8_t> debug_buffer_;
8298

devtools/etdump/tests/buffer_data_sink_test.cpp

Lines changed: 35 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
using namespace ::testing;
1717
using ::executorch::aten::ScalarType;
1818
using ::executorch::aten::Tensor;
19+
using ::executorch::etdump::BufferDataSink;
1920
using ::executorch::runtime::Error;
2021
using ::executorch::runtime::Result;
2122
using ::executorch::runtime::Span;
@@ -29,7 +30,11 @@ class BufferDataSinkTest : public ::testing::Test {
2930
buffer_size_ = 128; // Small size for testing
3031
buffer_ptr_ = malloc(buffer_size_);
3132
buffer_ = Span<uint8_t>(static_cast<uint8_t*>(buffer_ptr_), buffer_size_);
32-
data_sink_ = std::make_unique<executorch::etdump::BufferDataSink>(buffer_);
33+
Result<BufferDataSink> buffer_data_sink_ret =
34+
BufferDataSink::create(buffer_);
35+
ASSERT_EQ(buffer_data_sink_ret.error(), Error::Ok);
36+
buffer_data_sink_ =
37+
std::make_unique<BufferDataSink>(std::move(buffer_data_sink_ret.get()));
3338
}
3439

3540
void TearDown() override {
@@ -39,11 +44,11 @@ class BufferDataSinkTest : public ::testing::Test {
3944
size_t buffer_size_;
4045
void* buffer_ptr_;
4146
Span<uint8_t> buffer_;
42-
std::unique_ptr<executorch::etdump::BufferDataSink> data_sink_;
47+
std::unique_ptr<BufferDataSink> buffer_data_sink_;
4348
};
4449

4550
TEST_F(BufferDataSinkTest, StorageSizeCheck) {
46-
Result<size_t> ret = data_sink_->get_storage_size();
51+
Result<size_t> ret = buffer_data_sink_->get_storage_size();
4752
ASSERT_EQ(ret.error(), Error::Ok);
4853

4954
size_t storage_size = ret.get();
@@ -55,7 +60,7 @@ TEST_F(BufferDataSinkTest, WriteOneTensorAndCheckData) {
5560
Tensor tensor = tf.make({1, 4}, {1.0, 2.0, 3.0, 4.0});
5661

5762
Result<size_t> ret =
58-
data_sink_->write(tensor.const_data_ptr(), tensor.nbytes());
63+
buffer_data_sink_->write(tensor.const_data_ptr(), tensor.nbytes());
5964
ASSERT_EQ(ret.error(), Error::Ok);
6065

6166
size_t offset = ret.get();
@@ -75,9 +80,10 @@ TEST_F(BufferDataSinkTest, WriteMultiTensorsAndCheckData) {
7580
std::vector<Tensor> tensors = {
7681
tf.make({1, 4}, {1.0, 2.0, 3.0, 4.0}),
7782
tf.make({1, 4}, {5.0, 6.0, 7.0, 8.0})};
83+
7884
for (const auto& tensor : tensors) {
7985
Result<size_t> ret =
80-
data_sink_->write(tensor.const_data_ptr(), tensor.nbytes());
86+
buffer_data_sink_->write(tensor.const_data_ptr(), tensor.nbytes());
8187
ASSERT_EQ(ret.error(), Error::Ok);
8288

8389
size_t offset = ret.get();
@@ -94,8 +100,9 @@ TEST_F(BufferDataSinkTest, WriteMultiTensorsAndCheckData) {
94100
TEST_F(BufferDataSinkTest, PointerAlignmentCheck) {
95101
TensorFactory<ScalarType::Float> tf;
96102
Tensor tensor = tf.make({1, 4}, {1.0, 2.0, 3.0, 4.0});
103+
97104
Result<size_t> ret =
98-
data_sink_->write(tensor.const_data_ptr(), tensor.nbytes());
105+
buffer_data_sink_->write(tensor.const_data_ptr(), tensor.nbytes());
99106
ASSERT_EQ(ret.error(), Error::Ok);
100107

101108
size_t offset = ret.get();
@@ -112,12 +119,32 @@ TEST_F(BufferDataSinkTest, WriteUntilOverflow) {
112119
// Write tensors until we run out of space
113120
for (size_t i = 0; i < 2; i++) {
114121
Result<size_t> ret =
115-
data_sink_->write(tensor.const_data_ptr(), tensor.nbytes());
122+
buffer_data_sink_->write(tensor.const_data_ptr(), tensor.nbytes());
116123
ASSERT_EQ(ret.error(), Error::Ok);
117124
}
118125

119126
// Attempting to write another tensor should raise an error
120127
Result<size_t> ret =
121-
data_sink_->write(tensor.const_data_ptr(), tensor.nbytes());
128+
buffer_data_sink_->write(tensor.const_data_ptr(), tensor.nbytes());
122129
ASSERT_EQ(ret.error(), Error::OutOfResources);
123130
}
131+
132+
TEST_F(BufferDataSinkTest, illegalAlignment) {
133+
// Create a buffer_data_sink_ with legal alignment that is a power of 2 and
134+
// greater than 0
135+
for (size_t i = 1; i <= 128; i <<= 1) {
136+
Result<BufferDataSink> buffer_data_sink_ret =
137+
BufferDataSink::create(buffer_, i);
138+
ASSERT_EQ(buffer_data_sink_ret.error(), Error::Ok);
139+
}
140+
141+
// Create a buffer_data_sink_ with illegal alignment that is not a power of 2
142+
// or greater than 0
143+
std::vector<size_t> illegal_alignments = {0, 3, 5, 7, 100, 127};
144+
145+
for (size_t i = 0; i < illegal_alignments.size(); i++) {
146+
Result<BufferDataSink> buffer_data_sink_ret =
147+
BufferDataSink::create(buffer_, illegal_alignments[i]);
148+
ASSERT_EQ(buffer_data_sink_ret.error(), Error::InvalidArgument);
149+
}
150+
}

0 commit comments

Comments
 (0)