From: Phillip Wood Date: Sun, 09 Nov 2025 14:14:11 GMT Subject: Re: [PATCH v2 01/10] doc: define unambiguous type mappings across C and Rust Message-ID: In-Reply-To: On 06/11/2025 22:52, Ezekiel Newren wrote: > On Thu, Nov 6, 2025 at 2:55 AM Phillip Wood wrote: >> On 29/10/2025 22:19, Ezekiel Newren via GitGitGadget wrote: >>> From: Ezekiel Newren >>> >>> Document other nuances with crossing the FFI boundary. Other language >>> mappings may be added in the future. >> >> Thanks for adding this, I've left a few comments below. Overall I >> thought it was very well written. > > Thanks. > > I felt it was necessary since C vs Rust types keep coming up over and > over again. I'm flexible with the wording of this document. I was just > trying to convey a firm and clear stance on what is and isn't proper > in Git. That will definitely be useful as we add more rust code. In the future we may want to add a summary of which types to use to Documentation/CodingGuidelines but that doesn't need to be done in this series. >> I tried building an html version of >> this but even after adding it to the list of TECH_DOCS in >> Documentation/Makefile with >> >> diff --git a/Documentation/Makefile b/Documentation/Makefile >> index 47208269a2e..2699f0b24af 100644 >> --- a/Documentation/Makefile >> +++ b/Documentation/Makefile >> @@ -143,6 +143,7 @@ TECH_DOCS += technical/shallow >> TECH_DOCS += technical/sparse-checkout >> TECH_DOCS += technical/sparse-index >> TECH_DOCS += technical/trivial-merge >> +TECH_DOCS += technical/unambiguous-types >> TECH_DOCS += technical/unit-tests >> SP_ARTICLES += $(TECH_DOCS) >> SP_ARTICLES += technical/api-index >> >> it fails with >> >> $ make -C Documentation/ technical/unambiguous-types.html >> Merge branch >> 'ps/object-source-loose' into seen >> make: Entering directory '/home/phil/src/git/Documentation' >> GEN asciidoc.conf >> * new asciidoc flags >> ASCIIDOC technical/unambiguous-types.html >> asciidoc: ERROR: unambiguous-types.adoc: line 139: undefined filter >> attribute in command: source-highlight --gen-version -f xhtml -s >> {language} {src_numbered?--line-number=' '} {src_tab?--tab={src_tab}} >> {args=} >> asciidoc: ERROR: unambiguous-types.adoc: line 162: undefined filter >> attribute in command: source-highlight --gen-version -f xhtml -s >> {language} {src_numbered?--line-number=' '} {src_tab?--tab={src_tab}} >> {args=} >> asciidoc: ERROR: unambiguous-types.adoc: line 177: undefined filter >> attribute in command: source-highlight --gen-version -f xhtml -s >> {language} {src_numbered?--line-number=' '} {src_tab?--tab={src_tab}} >> {args=} >> asciidoc: ERROR: unambiguous-types.adoc: line 187: undefined filter >> attribute in command: source-highlight --gen-version -f xhtml -s >> {language} {src_numbered?--line-number=' '} {src_tab?--tab={src_tab}} >> {args=} >> asciidoc: ERROR: unambiguous-types.adoc: line 199: undefined filter >> attribute in command: source-highlight --gen-version -f xhtml -s >> {language} {src_numbered?--line-number=' '} {src_tab?--tab={src_tab}} >> {args=} >> asciidoc: ERROR: unambiguous-types.adoc: line 213: undefined filter >> attribute in command: source-highlight --gen-version -f xhtml -s >> {language} {src_numbered?--line-number=' '} {src_tab?--tab={src_tab}} >> {args=} >> asciidoc: ERROR: unambiguous-types.adoc: line 224: undefined filter >> attribute in command: source-highlight --gen-version -f xhtml -s >> {language} {src_numbered?--line-number=' '} {src_tab?--tab={src_tab}} >> {args=} >> make: *** [Makefile:396: technical/unambiguous-types.html] Error 1 >> make: *** Deleting file 'technical/unambiguous-types.html' >> make: Leaving directory '/home/phil/src/git/Documentation' > > I've never created documentation for Git before, so this helps. I'll > incorporate your suggestions. We should also add this file to Documentation/technical/meson.build. It seems those errors above are due to some incompatibility between asciidoc and asciidoctor as I just tried running make -C Documentation/ USE_ASCIIDOCTOR=1 technical/unambiguous-types.html and it worked just fine. I'm afraid I don't know enough asciidoc to make any helpful suggestions on how to fix it. >>> +== Character types >>> + >>> +This is where C and Rust don't have a clean one-to-one mapping. A C `char` is >>> +an 8-bit type that is signless (neither signed nor unsigned) >> >> I found this a bit confusing. Isn't the signedness of "char" >> implementation defined rather than it being "signless" >> >>> which causes >>> +problems with e.g. `make DEVELOPER=1`. >> >> I'm not sure what this is referring to - maybe -Wsign-compare? > > When I build Git with `make DEVELOPER=1` and I compare uint8_t with > char it complains about a difference in signedness. When I compare > int8_t with char it also complains about a difference in signedness. > So it is implementation defined, but it's also neither signed nor > unsigned according to DEVELOPER=1 since it complains either way. Oh, I see - this is saying mixing "char" and "uint8_t" causes problems. I agree, perhaps we could expand this slightly to mention comparison with uint8_t to make it clearer. >>> Rust's `char` type is an unsigned 32-bit >>> +integer that is used to describe Unicode code points. Even though a C `char` >>> +is the same width as `u8`, `char` should be converted to u8 where it is >>> +describing bytes in memory. >> >> I'm dreading the point where we start sharing "struct strbuf" with rust >> and have to change the "buf" member from "char*" to "uint8_t*". While it >> is not used in the xdiff code it is ubiquitous everywhere else and there >> are lots of places where be pass the "buf" member to functions expecting >> a "char*". >> >> git grep -E '(\.|->)buf\W' >> >> has over 4000 matches > > This is why I started in Xdiff since its code is mostly isolated. Good plan! > I > think that we might have to bite the bullet and deal with the ugly > mapping of char on the C side and u8 on the Rust side when dealing > with strbuf. Maybe as we translate more of C into Rust someone will > have a better suggestion. I think my ivec type would be better since > strbuf is almost a special case of my ivec type, but dealing with > strbuf is outside the scope of this patch series. Yes, hopefully it will become clearer what the least painful route forward is as we get more experience with rust <=> C iterop. >>> +While you could specify `char` in the C code and `u8` in Rust code, it's not as >>> +clear what the appropriate type is, but it would work across the FFI boundary. >>> +However the bigger problem comes from code generation tools like cbindgen and >>> +bindgen. When cbindgen see u8 in Rust it will generate uint8_t on the C side >>> +which will cause differ in signedness warnings/errors. Similarly if bindgen >>> +see `char` on the C side it will generate `std::ffi::c_char` which has its own >>> +problems. >> >> Yeah, we definitely don't want to be using "std::ffi::c_char" in our >> rust implementations. I do wonder if we might want to use it (or CStr) >> judiciously in function parameters and immediately convert it to u8 in >> the function body where the function is called from C though. > > That's basically the design pattern I've been using. > > In many of my translations from C to Rust I create a Rust stub > function that takes pointer types and wraps them into safe types which > then get handed off to a safe Rust function. I think that in the cases > where CString/CStr is required the Rust stub function would create a > &[u8] slice for the safe function to operate on. That sounds like a good pattern - we get a nice interface for the C code and the rust implementation uses the idiomatic rust types. Thanks Phillip >>> +=== Notes >>> +^1^ This is only true if stdbool.h (or equivalent) is used. + >>> +^2^ C does not enforce IEEE-754 compatibility, but Rust expects it. If the >>> +platform/arch for C does not follow IEEE-754 then this equivalence does not >>> +hold. Also, it's assumed that `float` is 32 bits and `double` is 64, but >>> +there may be a strange platform/arch where even this isn't true. + >>> +^3^ C also defines uintptr_t, but this should not be used in Git. + >>> +^4^ C also defines ssize_t and intptr_t, but these should not be used in Git. + >> >> [u]intptr_t and ssize_t are used in git already. As Junio has pointed >> out there are sane uses for these types but we don't want to use them in >> structs or function parameters where the struct or function is shared >> with rust. > > You're right, I should update the phrasing. Something like: "These > types shouldn't be used if their explicit purpose is for FFI. Whether > as a field in a struct or part of a function signature." I'll update > the wording. > >>> + >>> +== Problems with std::ffi::c_* types in Rust >>> +TL;DR: They're not guaranteed to match C types for all possible C >>> +compilers/platforms/architectures. >> >> Is this official policy of the rust project? > > No, this is a personal inference based on logical deduction. The c_* > definitions have changed over time with new Rust version releases, and > Git targets more platforms/architectures than what Rust officially > supports. While it's not guaranteed that it won't work everywhere. > It's also not guaranteed to work everywhere either. On top of that > we're targeting 1.63.0 who's c_* definitions are different in 1.89.0 > which I show an example of with c_long_definition. Can anyone say with > certainty that Rust got these mappings right or wrong for all possible > C compilers/architectures/platforms? If so (which I highly doubt) > could someone provide a link? >