Skip to content

Conversation

@vatsalkeshav
Copy link
Contributor

@vatsalkeshav vatsalkeshav commented Mar 26, 2025

In raising this pull request, I confirm the following (please check boxes):

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.
  • I have mentioned this change in the changelog.

My familiarity with the project is as follows (check one):

  • I have never used CCExtractor.
  • I have used CCExtractor just a couple of times.
  • I absolutely love CCExtractor, but have not contributed previously.
  • I am an active contributor to CCExtractor.

This PR migrates /src/lib_ccx/ccx_decoders_xds.c to rust.
These changes have been made-

  1. Pure rust equivalent functions in /src/rust/lib_ccxr/src/decoder_xds/functions_xds.rs
  2. Required datatypes in /src/rust/lib_ccxr/src/decoder_xds/structs_xds.rs
  3. C-compatible rust functions/exports in src/rust/src/libccxr_exports/xds_exports.rs
  4. Declaration and use of written rust functions in /src/lib_ccx/ccx_decoders_xds.c
  5. Added some #[repr(C)]s in existing structs

To-do:

  • Unit tests

@vatsalkeshav vatsalkeshav force-pushed the xds-migration branch 2 times, most recently from b5cb634 to 029d630 Compare March 29, 2025 13:44
@ccextractor-bot

This comment was marked as outdated.

@vatsalkeshav vatsalkeshav changed the title [FEAT] Add module decoder_xds to lib_ccxr [FEAT] WIP module decoder_xds to lib_ccxr Apr 21, 2025
@vatsalkeshav vatsalkeshav changed the title [FEAT] WIP module decoder_xds to lib_ccxr [FEAT] Add module decoder_xds to lib_ccxr Apr 21, 2025
@ccextractor-bot

This comment was marked as outdated.

Copy link
Contributor

@cfsmp3 cfsmp3 left a 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:

  1. PR has merge conflicts - needs rebase
  2. Unit tests are listed as TODO

Once these fundamental issues are addressed, the PR will need another deep review.

@vatsalkeshav
Copy link
Contributor Author

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.

@vatsalkeshav
Copy link
Contributor Author

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

@vatsalkeshav
Copy link
Contributor Author

closed in favor of pr #1890

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants