#25156 closed defect (wontfix)

Create unit tests for that check static strings match between Rust and C

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.3.2.1-alpha
Severity: Normal Keywords: rust, memory-leaks, technical-debt, 034-triage-20180328, 034-removed-20180328
Cc: chelseakomlo Actual Points:
Parent ID: Points: 1
Reviewer: Sponsor:

Description

In the protover crate, we list all supported protocol versions.
And then we do it again in C.

This makes it likely that they will get out of sync.
Also, passing static strings from Rust to C is error-prone (#25127).

Let's put all the static strings in C, and access them from Rust.
Yes, that might mean we pass them from C to Rust and back to C again. Oh well.

Child Tickets

Change History (13)

comment:1 Changed 15 months ago by nickm

Keywords: 034-triage-20180328 added

comment:2 Changed 15 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:3 Changed 15 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if time permits.

comment:4 Changed 13 months ago by chelseakomlo

Cc: chelseakomlo added

comment:5 Changed 13 months ago by chelseakomlo

I agree about having a single source of truth for cases like static strings- if we do this for protover, we should make it a coding standard for all new Rust functionality? It would be nice to standardize our approach.

comment:6 Changed 13 months ago by teor

I think we should have one copy of strings (and other constants) across Rust and C, as much as possible.

Let's make it a coding standard, and see how we go adopting it in protover?

comment:7 in reply to:  6 Changed 13 months ago by chelseakomlo

Replying to teor:

I think we should have one copy of strings (and other constants) across Rust and C, as much as possible.

Let's make it a coding standard, and see how we go adopting it in protover?

Yes I think that is a good idea. On one hand, removing duplication as much as possible is valuable. On the other hand, we'll need FFI calls for every static string (and we should look at the cost with string copying when moving across language boundaries). Either way it is messy but having a standard way of doing things will be helpful to managing complexity/possible bugs.

comment:8 Changed 13 months ago by teor

If calling into C for strings costs too much CPU, we can generate the files at build time.
Or, if we access the string many times, we can take a copy in Rust.

Also, duplicating strings costs RAM, so de-duplicating will save us RAM.

I'm not sure if either effect will be significant in practice.

comment:9 Changed 13 months ago by teor

Also, duplicating strings costs RAM, so de-duplicating will save us RAM.

This isn't true: we only compile the C module or the Rust module, not both.

comment:10 Changed 13 months ago by teor

We think that the fastest way to resolve this issue is to create unit tests in C that check that the Rust and C string, enum, and integer constants are equal.

In the longer term, we should think about generating C and Rust constant source files from a single source of truth.

comment:11 Changed 13 months ago by teor

Summary: Stop duplicating static strings between Rust and CCreate unit tests for that check static strings match between Rust and C

comment:12 Changed 13 months ago by chelseakomlo

Closing this in favor of #24249, as we will soon have a simple POC for Rust constants, which we can later expand into other shared types (within reason, as we still want to keep the shared interface small between Rust/C).

comment:13 Changed 13 months ago by chelseakomlo

Resolution: wontfix
Status: newclosed
Note: See TracTickets for help on using tickets.