Skip to content

Commit 0c67076

Browse files
committed
Merge branch 'en/xdiff-cleanup-2'
Code clean-up. * en/xdiff-cleanup-2: xdiff: rename rindex -> reference_index xdiff: change rindex from long to size_t in xdfile_t xdiff: make xdfile_t.nreff a size_t instead of long xdiff: make xdfile_t.nrec a size_t instead of long xdiff: split xrecord_t.ha into line_hash and minimal_perfect_hash xdiff: use unambiguous types in xdl_hash_record() xdiff: use size_t for xrecord_t.size xdiff: make xrecord_t.ptr a uint8_t instead of char xdiff: use ptrdiff_t for dstart/dend doc: define unambiguous type mappings across C and Rust
2 parents f0ef5b6 + 22ce0cb commit 0c67076

13 files changed

Lines changed: 336 additions & 110 deletions

File tree

Documentation/Makefile

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,7 @@ TECH_DOCS += technical/shallow
143143
TECH_DOCS += technical/sparse-checkout
144144
TECH_DOCS += technical/sparse-index
145145
TECH_DOCS += technical/trivial-merge
146+
TECH_DOCS += technical/unambiguous-types
146147
TECH_DOCS += technical/unit-tests
147148
SP_ARTICLES += $(TECH_DOCS)
148149
SP_ARTICLES += technical/api-index

Documentation/technical/meson.build

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ articles = [
3232
'sparse-checkout.adoc',
3333
'sparse-index.adoc',
3434
'trivial-merge.adoc',
35+
'unambiguous-types.adoc',
3536
'unit-tests.adoc',
3637
]
3738

Lines changed: 224 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,224 @@
1+
= Unambiguous types
2+
3+
Most of these mappings are obvious, but there are some nuances and gotchas with
4+
Rust FFI (Foreign Function Interface).
5+
6+
This document defines clear, one-to-one mappings between primitive types in C,
7+
Rust (and possible other languages in the future). Its purpose is to eliminate
8+
ambiguity in type widths, signedness, and binary representation across
9+
platforms and languages.
10+
11+
For Git, the only header required to use these unambiguous types in C is
12+
`git-compat-util.h`.
13+
14+
== Boolean types
15+
[cols="1,1", options="header"]
16+
|===
17+
| C Type | Rust Type
18+
| bool^1^ | bool
19+
|===
20+
21+
== Integer types
22+
23+
In C, `<stdint.h>` (or an equivalent) must be included.
24+
25+
[cols="1,1", options="header"]
26+
|===
27+
| C Type | Rust Type
28+
| uint8_t | u8
29+
| uint16_t | u16
30+
| uint32_t | u32
31+
| uint64_t | u64
32+
33+
| int8_t | i8
34+
| int16_t | i16
35+
| int32_t | i32
36+
| int64_t | i64
37+
|===
38+
39+
== Floating-point types
40+
41+
Rust requires IEEE-754 semantics.
42+
In C, that is typically true, but not guaranteed by the standard.
43+
44+
[cols="1,1", options="header"]
45+
|===
46+
| C Type | Rust Type
47+
| float^2^ | f32
48+
| double^2^ | f64
49+
|===
50+
51+
== Size types
52+
53+
These types represent pointer-sized integers and are typically defined in
54+
`<stddef.h>` or an equivalent header.
55+
56+
Size types should be used any time pointer arithmetic is performed e.g.
57+
indexing an array, describing the number of elements in memory, etc...
58+
59+
[cols="1,1", options="header"]
60+
|===
61+
| C Type | Rust Type
62+
| size_t^3^ | usize
63+
| ptrdiff_t^3^ | isize
64+
|===
65+
66+
== Character types
67+
68+
This is where C and Rust don't have a clean one-to-one mapping.
69+
70+
A C `char` and a Rust `u8` share the same bit width, so any C struct containing
71+
a `char` will have the same size as the corresponding Rust struct using `u8`.
72+
In that sense, such structs are safe to pass over the FFI boundary, because
73+
their fields will be laid out identically. However, beyond bit width, C `char`
74+
has additional semantics and platform-dependent behavior that can cause
75+
problems, as discussed below.
76+
77+
The C language leaves the signedness of `char` implementation defined. Because
78+
our developer build enables -Wsign-compare, comparison of a value of `char`
79+
type with either signed or unsigned integers may trigger warnings from the
80+
compiler.
81+
82+
Note: Rust's `char` type is an unsigned 32-bit integer that is used to describe
83+
Unicode code points.
84+
85+
=== Notes
86+
^1^ This is only true if stdbool.h (or equivalent) is used. +
87+
^2^ C does not enforce IEEE-754 compatibility, but Rust expects it. If the
88+
platform/arch for C does not follow IEEE-754 then this equivalence does not
89+
hold. Also, it's assumed that `float` is 32 bits and `double` is 64, but
90+
there may be a strange platform/arch where even this isn't true. +
91+
^3^ C also defines uintptr_t, ssize_t and intptr_t, but these types are
92+
discouraged for FFI purposes. For functions like `read()` and `write()` ssize_t
93+
should be cast to a different, and unambiguous, type before being passed over
94+
the FFI boundary. +
95+
96+
== Problems with std::ffi::c_* types in Rust
97+
TL;DR: In practice, Rust's `c_*` types aren't guaranteed to match C types for
98+
all possible C compilers, platforms, or architectures, because Rust only
99+
ensures correctness of C types on officially supported targets. These
100+
definitions have changed over time to match more targets which means that the
101+
c_* definitions will differ based on which Rust version Git chooses to use.
102+
103+
Current list of safe, Rust side, FFI types in Git: +
104+
105+
* `c_void`
106+
* `CStr`
107+
* `CString`
108+
109+
Even then, they should be used sparingly, and only where the semantics match
110+
exactly.
111+
112+
The std::os::raw::c_* directly inherits the problems of core::ffi, which
113+
changes over time and seems to make a best guess at the correct definition for
114+
a given platform/target. This probably isn't a problem for all other platforms
115+
that Rust supports currently, but can anyone say that Rust got it right for all
116+
C compilers of all platforms/targets?
117+
118+
To give an example: c_long is defined in
119+
footnote:[https://doc.rust-lang.org/1.63.0/src/core/ffi/mod.rs.html#175-189[c_long in 1.63.0]]
120+
footnote:[https://doc.rust-lang.org/1.89.0/src/core/ffi/primitives.rs.html#135-151[c_long in 1.89.0]]
121+
122+
=== Rust version 1.63.0
123+
124+
```
125+
mod c_long_definition {
126+
cfg_if! {
127+
if #[cfg(all(target_pointer_width = "64", not(windows)))] {
128+
pub type c_long = i64;
129+
pub type NonZero_c_long = crate::num::NonZeroI64;
130+
pub type c_ulong = u64;
131+
pub type NonZero_c_ulong = crate::num::NonZeroU64;
132+
} else {
133+
// The minimal size of `long` in the C standard is 32 bits
134+
pub type c_long = i32;
135+
pub type NonZero_c_long = crate::num::NonZeroI32;
136+
pub type c_ulong = u32;
137+
pub type NonZero_c_ulong = crate::num::NonZeroU32;
138+
}
139+
}
140+
}
141+
```
142+
143+
=== Rust version 1.89.0
144+
145+
```
146+
mod c_long_definition {
147+
crate::cfg_select! {
148+
any(
149+
all(target_pointer_width = "64", not(windows)),
150+
// wasm32 Linux ABI uses 64-bit long
151+
all(target_arch = "wasm32", target_os = "linux")
152+
) => {
153+
pub(super) type c_long = i64;
154+
pub(super) type c_ulong = u64;
155+
}
156+
_ => {
157+
// The minimal size of `long` in the C standard is 32 bits
158+
pub(super) type c_long = i32;
159+
pub(super) type c_ulong = u32;
160+
}
161+
}
162+
}
163+
```
164+
165+
Even for the cases where C types are correctly mapped to Rust types via
166+
std::ffi::c_* there are still problems. Let's take c_char for example. On some
167+
platforms it's u8 on others it's i8.
168+
169+
=== Subtraction underflow in debug mode
170+
171+
The following code will panic in debug on platforms that define c_char as u8,
172+
but won't if it's an i8.
173+
174+
```
175+
let mut x: std::ffi::c_char = 0;
176+
x -= 1;
177+
```
178+
179+
=== Inconsistent shift behavior
180+
181+
`x` will be 0xC0 for platforms that use i8, but will be 0x40 where it's u8.
182+
183+
```
184+
let mut x: std::ffi::c_char = 0x80;
185+
x >>= 1;
186+
```
187+
188+
=== Equality fails to compile on some platforms
189+
190+
The following will not compile on platforms that define c_char as i8, but will
191+
if it's u8. You can cast x e.g. `assert_eq!(x as u8, b'a');`, but then you get
192+
a warning on platforms that use u8 and a clean compilation where i8 is used.
193+
194+
```
195+
let mut x: std::ffi::c_char = 0x61;
196+
assert_eq!(x, b'a');
197+
```
198+
199+
== Enum types
200+
Rust enum types should not be used as FFI types. Rust enum types are more like
201+
C union types than C enum's. For something like:
202+
203+
```
204+
#[repr(C, u8)]
205+
enum Fruit {
206+
Apple,
207+
Banana,
208+
Cherry,
209+
}
210+
```
211+
212+
It's easy enough to make sure the Rust enum matches what C would expect, but a
213+
more complex type like.
214+
215+
```
216+
enum HashResult {
217+
SHA1([u8; 20]),
218+
SHA256([u8; 32]),
219+
}
220+
```
221+
222+
The Rust compiler has to add a discriminant to the enum to distinguish between
223+
the variants. The width, location, and values for that discriminant is up to
224+
the Rust compiler and is not ABI stable.

xdiff-interface.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -300,7 +300,7 @@ void xdiff_clear_find_func(xdemitconf_t *xecfg)
300300

301301
unsigned long xdiff_hash_string(const char *s, size_t len, long flags)
302302
{
303-
return xdl_hash_record(&s, s + len, flags);
303+
return xdl_hash_record((uint8_t const**)&s, (uint8_t const*)s + len, flags);
304304
}
305305

306306
int xdiff_compare_lines(const char *l1, long s1,

xdiff/xdiffi.c

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,9 @@
2222

2323
#include "xinclude.h"
2424

25-
static unsigned long get_hash(xdfile_t *xdf, long index)
25+
static size_t get_hash(xdfile_t *xdf, long index)
2626
{
27-
return xdf->recs[xdf->rindex[index]].ha;
27+
return xdf->recs[xdf->reference_index[index]].minimal_perfect_hash;
2828
}
2929

3030
#define XDL_MAX_COST_MIN 256
@@ -278,10 +278,10 @@ int xdl_recs_cmp(xdfile_t *xdf1, long off1, long lim1,
278278
*/
279279
if (off1 == lim1) {
280280
for (; off2 < lim2; off2++)
281-
xdf2->changed[xdf2->rindex[off2]] = true;
281+
xdf2->changed[xdf2->reference_index[off2]] = true;
282282
} else if (off2 == lim2) {
283283
for (; off1 < lim1; off1++)
284-
xdf1->changed[xdf1->rindex[off1]] = true;
284+
xdf1->changed[xdf1->reference_index[off1]] = true;
285285
} else {
286286
xdpsplit_t spl;
287287
spl.i1 = spl.i2 = 0;
@@ -385,7 +385,7 @@ static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1,
385385

386386
static int recs_match(xrecord_t *rec1, xrecord_t *rec2)
387387
{
388-
return (rec1->ha == rec2->ha);
388+
return rec1->minimal_perfect_hash == rec2->minimal_perfect_hash;
389389
}
390390

391391
/*
@@ -403,11 +403,10 @@ static int recs_match(xrecord_t *rec1, xrecord_t *rec2)
403403
*/
404404
static int get_indent(xrecord_t *rec)
405405
{
406-
long i;
407406
int ret = 0;
408407

409-
for (i = 0; i < rec->size; i++) {
410-
char c = rec->ptr[i];
408+
for (size_t i = 0; i < rec->size; i++) {
409+
char c = (char) rec->ptr[i];
411410

412411
if (!XDL_ISSPACE(c))
413412
return ret;
@@ -484,7 +483,7 @@ static void measure_split(const xdfile_t *xdf, long split,
484483
{
485484
long i;
486485

487-
if (split >= xdf->nrec) {
486+
if (split >= (long)xdf->nrec) {
488487
m->end_of_file = 1;
489488
m->indent = -1;
490489
} else {
@@ -507,7 +506,7 @@ static void measure_split(const xdfile_t *xdf, long split,
507506

508507
m->post_blank = 0;
509508
m->post_indent = -1;
510-
for (i = split + 1; i < xdf->nrec; i++) {
509+
for (i = split + 1; i < (long)xdf->nrec; i++) {
511510
m->post_indent = get_indent(&xdf->recs[i]);
512511
if (m->post_indent != -1)
513512
break;
@@ -718,7 +717,7 @@ static void group_init(xdfile_t *xdf, struct xdlgroup *g)
718717
*/
719718
static inline int group_next(xdfile_t *xdf, struct xdlgroup *g)
720719
{
721-
if (g->end == xdf->nrec)
720+
if (g->end == (long)xdf->nrec)
722721
return -1;
723722

724723
g->start = g->end + 1;
@@ -751,7 +750,7 @@ static inline int group_previous(xdfile_t *xdf, struct xdlgroup *g)
751750
*/
752751
static int group_slide_down(xdfile_t *xdf, struct xdlgroup *g)
753752
{
754-
if (g->end < xdf->nrec &&
753+
if (g->end < (long)xdf->nrec &&
755754
recs_match(&xdf->recs[g->start], &xdf->recs[g->end])) {
756755
xdf->changed[g->start++] = false;
757756
xdf->changed[g->end++] = true;
@@ -993,11 +992,11 @@ static void xdl_mark_ignorable_lines(xdchange_t *xscr, xdfenv_t *xe, long flags)
993992

994993
rec = &xe->xdf1.recs[xch->i1];
995994
for (i = 0; i < xch->chg1 && ignore; i++)
996-
ignore = xdl_blankline(rec[i].ptr, rec[i].size, flags);
995+
ignore = xdl_blankline((const char *)rec[i].ptr, (long)rec[i].size, flags);
997996

998997
rec = &xe->xdf2.recs[xch->i2];
999998
for (i = 0; i < xch->chg2 && ignore; i++)
1000-
ignore = xdl_blankline(rec[i].ptr, rec[i].size, flags);
999+
ignore = xdl_blankline((const char *)rec[i].ptr, (long)rec[i].size, flags);
10011000

10021001
xch->ignore = ignore;
10031002
}
@@ -1008,7 +1007,7 @@ static int record_matches_regex(xrecord_t *rec, xpparam_t const *xpp) {
10081007
size_t i;
10091008

10101009
for (i = 0; i < xpp->ignore_regex_nr; i++)
1011-
if (!regexec_buf(xpp->ignore_regex[i], rec->ptr, rec->size, 1,
1010+
if (!regexec_buf(xpp->ignore_regex[i], (const char *)rec->ptr, rec->size, 1,
10121011
&regmatch, 0))
10131012
return 1;
10141013

0 commit comments

Comments
 (0)