[FEAT] Add module decoder_xds to lib_ccxr#1679
[FEAT] Add module decoder_xds to lib_ccxr#1679vatsalkeshav wants to merge 2 commits intoCCExtractor:masterfrom
decoder_xds to lib_ccxr#1679Conversation
b5cb634 to
029d630
Compare
This comment was marked as outdated.
This comment was marked as outdated.
decoder_xds to lib_ccxrdecoder_xds to lib_ccxr
decoder_xds to lib_ccxrdecoder_xds to lib_ccxr
029d630 to
32fd02f
Compare
abe3fd8 to
9f03ab4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
cfsmp3
left a comment
There was a problem hiding this comment.
Thank you for working on migrating the XDS decoder to Rust! I see you're also the author of PR #1676 (ISDB decoder) - both are part of the Rust migration effort.
However, I found several critical issues that need to be addressed:
1. FFI Type Mismatch - Rust types not C-compatible
Your Rust struct uses Vec<String>, Vec<XdsBuffer>, String, etc. with #[repr(C)]:
#[repr(C)]
pub struct CcxDecodersXdsContext {
pub xds_program_description: Vec<String>, // NOT C-compatible!
pub xds_buffers: Vec<XdsBuffer>, // NOT C-compatible!
// ...
}The C struct uses fixed arrays:
char xds_program_description[8][33];
struct xds_buffer xds_buffers[NUM_XDS_BUFFERS];#[repr(C)] ensures C-compatible layout, but Vec<T> and String are fundamentally different from C arrays - they're heap-allocated with pointer+length+capacity, not contiguous fixed-size data.
2. Type mismatch in ccxr_ccx_decoders_xds_init_library
The Rust function expects TimingContext * but C passes ccx_common_timing_ctx *. These need to be verified as layout-compatible, or you need a conversion layer.
3. Missing destructor
Box::into_raw() allocates memory that must be freed. Add ccxr_ccx_decoders_xds_free_context():
#[no_mangle]
pub unsafe extern "C" fn ccxr_ccx_decoders_xds_free_context(ctx: *mut CcxDecodersXdsContext) {
if !ctx.is_null() {
drop(Box::from_raw(ctx));
}
}4. test_rust CI failing
Unlike your other PR (#1676) where only sample platform tests failed, this PR has actual Rust test failures. Please investigate.
Architecture suggestion: Consider keeping the Rust struct fully opaque to C. The C code should only ever hold an opaque pointer, and all field access should go through Rust FFI functions. This avoids layout compatibility issues.
Other notes:
- PR has merge conflicts - needs rebase
- Unit tests are listed as TODO
Once these fundamental issues are addressed, the PR will need another deep review.
|
Thanks for the review. I’ll push some local changes by EOD using a data transfer library (based on the pattern of recent Rust migrations), which I believe should address these issues. |
needs rebasing - might take some more time |
|
closed in favor of pr #1890 |
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
This PR migrates
/src/lib_ccx/ccx_decoders_xds.cto rust.These changes have been made-
/src/rust/lib_ccxr/src/decoder_xds/functions_xds.rs/src/rust/lib_ccxr/src/decoder_xds/structs_xds.rssrc/rust/src/libccxr_exports/xds_exports.rs/src/lib_ccx/ccx_decoders_xds.c#[repr(C)]s in existing structsTo-do: