This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9901

persistent-nodemap: Fix Rust declarations for Revlog_CAPI signatures
ClosedPublic

Authored by SimonSapin on Jan 28 2021, 7:41 AM.

Details

Reviewers
Alphare
pulkit
Group Reviewers
hg-reviewers
Commits
rHG7d0405e458a0: persistent-nodemap: Fix Rust declarations for Revlog_CAPI signatures
Summary

Use Rust’s libc::ssize_t as the closest match to C’s Py_ssize_t.
See details in test comment.

Going forward we should find a way to have such Rust declarations
auto-generated from C headers at build time,
or auto-checked against them in a test.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

SimonSapin created this revision.Jan 28 2021, 7:41 AM
SimonSapin updated this revision to Diff 25344.Jan 28 2021, 7:45 AM
SimonSapin added a comment.Jan 28 2021, 7:45 AM

This two patches are for the stable branch.

Alphare accepted this revision.Jan 28 2021, 7:47 AM
pulkit accepted this revision.Jan 28 2021, 1:17 PM
This revision is now accepted and ready to land.Jan 28 2021, 1:17 PM

Revision Contents
Changeset List

Diff 25368

rust/hg-cpython/src/cindex.rs

//! but this will take some time to get there. //! but this will take some time to get there.
use cpython::{ use cpython::{
exc::ImportError, ObjectProtocol, PyClone, PyErr, PyObject, PyResult, exc::ImportError, ObjectProtocol, PyClone, PyErr, PyObject, PyResult,
PyTuple, Python, PythonObject, PyTuple, Python, PythonObject,
}; };
use hg::revlog::{Node, RevlogIndex}; use hg::revlog::{Node, RevlogIndex};
use hg::{Graph, GraphError, Revision, WORKING_DIRECTORY_REVISION}; use hg::{Graph, GraphError, Revision, WORKING_DIRECTORY_REVISION};
use libc::c_int; use libc::{c_int, ssize_t};
const REVLOG_CABI_VERSION: c_int = 2; const REVLOG_CABI_VERSION: c_int = 2;
#[repr(C)] #[repr(C)]
pub struct Revlog_CAPI { pub struct Revlog_CAPI {
abi_version: c_int, abi_version: c_int,
index_length: index_length:
unsafe extern "C" fn(index: *mut revlog_capi::RawPyObject) -> c_int, unsafe extern "C" fn(index: *mut revlog_capi::RawPyObject) -> ssize_t,
index_node: unsafe extern "C" fn( index_node: unsafe extern "C" fn(
index: *mut revlog_capi::RawPyObject, index: *mut revlog_capi::RawPyObject,
rev: c_int, rev: ssize_t,
) -> *const Node, ) -> *const Node,
index_parents: unsafe extern "C" fn( index_parents: unsafe extern "C" fn(
index: *mut revlog_capi::RawPyObject, index: *mut revlog_capi::RawPyObject,
rev: c_int, rev: c_int,
ps: *mut [c_int; 2], ps: *mut [c_int; 2],
) -> c_int, ) -> c_int,
} }
/// Note C return type is Py_ssize_t (hence signed), but we shall /// Note C return type is Py_ssize_t (hence signed), but we shall
/// force it to unsigned, because it's a length /// force it to unsigned, because it's a length
fn len(&self) -> usize { fn len(&self) -> usize {
unsafe { (self.capi.index_length)(self.index.as_ptr()) as usize } unsafe { (self.capi.index_length)(self.index.as_ptr()) as usize }
} }
fn node(&self, rev: Revision) -> Option<&Node> { fn node(&self, rev: Revision) -> Option<&Node> {
let raw = unsafe { let raw = unsafe {
(self.capi.index_node)(self.index.as_ptr(), rev as c_int) (self.capi.index_node)(self.index.as_ptr(), rev as ssize_t)
}; };
if raw.is_null() { if raw.is_null() {
None None
} else { } else {
// TODO it would be much better for the C layer to give us // TODO it would be much better for the C layer to give us
// a length, since the hash length will change in the near // a length, since the hash length will change in the near
// future, but that's probably out of scope for the nodemap // future, but that's probably out of scope for the nodemap
// patch series. // patch series.
// //
// The root of that unsafety relies in the signature of // The root of that unsafety relies in the signature of
// `capi.index_node()` itself: returning a `Node` pointer // `capi.index_node()` itself: returning a `Node` pointer
// whereas it's a `char *` in the C counterpart. // whereas it's a `char *` in the C counterpart.
Some(unsafe { &*raw }) Some(unsafe { &*raw })
} }
} }
} }

tests/test-persistent-nodemap.t

> # to avoid spamming the test > # to avoid spamming the test
> revlog.persistent-nodemap.slow-path=allow > revlog.persistent-nodemap.slow-path=allow
> EOF > EOF
#endif #endif
#if rust #if rust
Reported bug: some Rust code panics when handling the null revision Regression test for a previous bug in Rust/C FFI for the `Revlog_CAPI` capsule:
in places where `mercurial/cext/revlog.c` function signatures use `Py_ssize_t`
(64 bits on Linux x86_64), corresponding declarations in `rust/hg-cpython/src/cindex.rs`
incorrectly used `libc::c_int` (32 bits).
As a result, -1 passed from Rust for the null revision became 4294967295 in C.
$ hg log -r 00000000
changeset: -1:000000000000
tag: tip
user:
date: Thu Jan 01 00:00:00 1970 +0000
$ hg log -r 00000000 2>&1 | grep panicked
thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', hg-cpython/src/revlog.rs:* (glob)
#endif #endif
$ hg debugformat $ hg debugformat
format-variant repo format-variant repo
fncache: yes fncache: yes
dotencode: yes dotencode: yes