Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19007
Note: Links to docs will display an error until the docs builds have been completed. ❌ 6 New FailuresAs of commit 7108ed7 with merge base 8ed6e85 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
This PR needs a
|
lucylq
force-pushed
the
lfq.deprecate-constant-buffer
branch
from
38c53e4 to
7108ed7
Compare
|
@claude review |
|
Claude finished @lucylq's task in 1m 22s —— View job Review of PR #19007: Deprecate constant buffer
Good cleanup. The change removes the deprecated inline Potential edge case:
|
|
@lucylq has imported this pull request. If you are a Meta employee, you can view this in D101701124. |
There was a problem hiding this comment.
Pull request overview
This PR aims to fully deprecate/reject the legacy inline constant storage (Program.constant_buffer) in the runtime loader, aligning runtime behavior with the AoT-side deprecation (since ExecuTorch 0.7) and reducing exposure to potentially unsafe legacy parsing paths.
Changes:
- Make
Program::load()require constants to come fromconstant_segmentand reject deprecated inlineconstant_bufferprograms. - Simplify
Program::get_constant_buffer_data()to only read from the loaded constant segment. - Update executor tests to remove execution of legacy constant-buffer programs and add a load-time rejection test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| runtime/executor/program.cpp | Removes the deprecated inline-constant branch at load time and enforces constant-segment-only retrieval. |
| runtime/executor/test/program_test.cpp | Replaces legacy constant-buffer compatibility test with a load-time rejection test; adjusts constant-data bounds test. |
| runtime/executor/test/method_test.cpp | Stops loading/executing the deprecated constant-buffer test module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Constant data lives in a separate segment (constant_segment). | ||
| const auto* constant_segment = flatbuffer_program->constant_segment(); | ||
| if (constant_segment != nullptr && constant_segment->offsets() != nullptr && | ||
| constant_segment->offsets()->size() > 0) { | ||
| if (constant_segment->offsets()->size() == 1) { | ||
| // No constants; the constant segment is empty and does not | ||
| // need to be loaded. | ||
| return Program( | ||
| loader, | ||
| segment_base_offset, | ||
| std::move(program_data.get()), | ||
| flatbuffer_program, | ||
| /*constant_segment_data=*/FreeableBuffer{}, | ||
| std::move(pte_data_map)); | ||
| } | ||
| // The constant data is inside a separate segment. | ||
| const auto* constant_buffer = flatbuffer_program->constant_buffer(); | ||
| ET_CHECK_OR_RETURN_ERROR( | ||
| constant_buffer == nullptr || constant_buffer->size() == 0, | ||
| InvalidProgram, | ||
| "constant_buffer contains %zu items, " | ||
| "constant_segment.offsets contains %zu items. Only one should be used.", | ||
| static_cast<size_t>(constant_buffer->size()), | ||
| static_cast<size_t>(constant_segment->offsets()->size())); | ||
| const auto* segments = flatbuffer_program->segments(); | ||
| ET_CHECK_OR_RETURN_ERROR( | ||
| segments != nullptr, InvalidProgram, "No segments in program"); | ||
|
|
||
| // Load constant segment. | ||
| // TODO(T171839323): Add test for segment_index > num available segments. | ||
| ET_CHECK_OR_RETURN_ERROR( | ||
| constant_segment->segment_index() < segments->size(), | ||
| InvalidProgram, | ||
| "Constant segment index %zu invalid for program segments range %zu", | ||
| static_cast<size_t>(constant_segment->segment_index()), | ||
| static_cast<size_t>(segments->size())); | ||
|
|
||
| const executorch_flatbuffer::DataSegment* data_segment = | ||
| segments->Get(constant_segment->segment_index()); | ||
| Result<FreeableBuffer> constant_segment_data = loader->load( | ||
| segment_base_offset + data_segment->offset(), | ||
| data_segment->size(), | ||
| DataLoader::SegmentInfo( | ||
| DataLoader::SegmentInfo::Type::Constant, | ||
| constant_segment->segment_index())); | ||
| if (!constant_segment_data.ok()) { | ||
| return constant_segment_data.error(); | ||
| } | ||
| // The FreeableBuffer owns the data that flatbuffer_program points into. | ||
| // Also keep a pointer to the loader so it can load more segments when | ||
| // necessary. | ||
| return Program( | ||
| loader, | ||
| segment_base_offset, | ||
| std::move(program_data.get()), | ||
| flatbuffer_program, | ||
| std::move(constant_segment_data.get()), | ||
| std::move(pte_data_map)); | ||
| } else { | ||
| // The constant data is stored inside the flatbuffer, so this program does | ||
| // not contain a separate segment for it. | ||
|
|
||
| // NOTE: This branch is deprecated from ExecuTorch 0.7 onwards. | ||
| // Please regenerate your PTE file to ensure newer ExecuTorch runtimes can | ||
| // support it. ExecuTorch deprecation policy: | ||
| // https://docs.pytorch.org/executorch/stable/api-life-cycle.html#deprecation-policy. | ||
| // For support, contact the PyTorch Edge team or make an issue in: | ||
| // https://github.com/pytorch/executorch/issues. | ||
| ET_LOG( | ||
| Error, | ||
| "!!DEPRECATED!! This branch is deprecated from ExecuTorch 0.7; re-export this PTE file to ensure support on newer runtimes."); | ||
| ET_CHECK_OR_RETURN_ERROR( | ||
| constant_segment != nullptr && constant_segment->offsets() != nullptr, | ||
| InvalidProgram, | ||
| "Program constant segment is nullptr. Expect at least one constant placeholder."); | ||
| // The offsets list will always contain a placeholder value 0 for non-const | ||
| // tensors. If this is the only offset, the constant segment is empty and | ||
| // does not need to be loaded. | ||
| if (constant_segment->offsets()->size() == 1) { | ||
| return Program( |
There was a problem hiding this comment.
In Program::load(), the early return for constant_segment->offsets()->size() == 1 happens before validating Program.constant_buffer is empty. This means a malformed/corrupt PTE could still include deprecated inline constant_buffer data and not be rejected. Also consider explicitly rejecting offsets()->size() == 0 (no placeholder) since the loader currently treats it as “has constants” and attempts to load a segment, which contradicts the placeholder invariant used elsewhere/tests.
| // Constant data must live in a separate segment (constant_segment_data) that | ||
| // was loaded during Program::load. The deprecated inline | ||
| // Program.constant_buffer path is rejected at load time. | ||
| ET_CHECK_OR_RETURN_ERROR( | ||
| constant_segment_data_.data() != nullptr, | ||
| InvalidProgram, | ||
| "Program has no constant segment; cannot retrieve constant data."); | ||
|
|
||
| const auto* constant_segment = internal_program->constant_segment(); | ||
| size_t num_elems = constant_segment == nullptr | ||
| ? 0 | ||
| : (constant_segment->offsets() == nullptr | ||
| ? 0 | ||
| : constant_segment->offsets()->size()); | ||
| ET_CHECK_OR_RETURN_ERROR( | ||
| buffer_index < num_elems, | ||
| InvalidArgument, | ||
| "Constant segment buffer index %zu invalid for program constant segment range %zu", |
There was a problem hiding this comment.
Program::get_constant_buffer_data() now hard-fails with InvalidProgram when constant_segment_data_.data() == nullptr. However, valid programs with an empty constant segment (segment size 0 / offsets only placeholder) will naturally have constant_segment_data_ empty/null (and Program::load() intentionally doesn’t load it). Consider using constant_segment.offsets()->size() to distinguish “no constants / invalid buffer_index” (should be InvalidArgument) from “constants expected but segment data missing” (could be InvalidProgram).
| TEST_F(ProgramTest, GetConstantBufferDataRejectsOversizedRequest) { | ||
| const char* path = | ||
| std::getenv("DEPRECATED_ET_MODULE_LINEAR_CONSTANT_BUFFER_PATH"); | ||
| Result<FileDataLoader> loader = FileDataLoader::from(path); | ||
| ASSERT_EQ(loader.error(), Error::Ok); | ||
|
|
||
| Result<Program> program = Program::load(&loader.get()); | ||
| Result<Program> program = Program::load(add_loader_.get()); | ||
| ASSERT_EQ(program.error(), Error::Ok); | ||
|
|
||
| // Request way more bytes than any buffer could contain | ||
| // Request way more bytes than the constant segment could contain. | ||
| Result<const void*> data = ProgramTestFriend::get_constant_buffer_data( | ||
| &program.get(), /*buffer_index=*/0, /*nbytes=*/SIZE_MAX); | ||
|
|
There was a problem hiding this comment.
This test now loads add_loader_ (ET_MODULE_ADD), which earlier tests indicate has no constants (constant segment is only a placeholder / size 0). With the new runtime behavior, get_constant_buffer_data() will return InvalidProgram (no segment data) rather than exercising the oversized-request bounds check. Use a module that actually has constant-segment data (e.g., ET_MODULE_ADD_MUL_PATH) and a non-placeholder buffer_index (typically 1) to keep this test asserting the intended behavior.
It's been deprecated on AoT side since et 0.7. Let's try to deprecate it fully; it is also causing some security risk right now.
Import to check internal failures.