-
Notifications
You must be signed in to change notification settings - Fork 505
[FEAT] Add module decoder_xds to lib_ccxr
#1679
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: