This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7856

revlog-native: introduced ABI version in capsule
ClosedPublic

Authored by gracinet on Jan 14 2020, 6:05 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGf5d2720f3bea: revlog-native: introduced ABI version in capsule
rHG90af0a4023c3: revlog-native: introduced ABI version in capsule
rHGe300421d67f5: revlog-native: introduced ABI version in capsule
Summary

Concerns that an inconsistency could arise between the actual contents
of the capsule in revlog.c and the Rust consumer have been raised after
the switch to the array of data and function pointers in f384d68d8ea8.

It has been suggested that the version from parsers.c could be use for
this. In this change, we introduce instead a separate ABI version number,
which should have the following advantages:

  • no need to change the consuming Rust code for changes that have nothing to do with the contents of the capsule
  • the version number in parsers.c is not explicitely flagged as ABI. It's not obvious to me whether an ABI change that would be invisible to Python would warrant an increment

The drawback is that developers now have to consider two version numbers.

We expect the added cost of the check to be negligible because it occurs
at instantiation of CIndex only, which in turn is tied to instantiation
of Python objects such as LazyAncestors and MixedIndex. Frequent calls
to Cindex::new should also probably hit the CPU branch predictor.

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

gracinet created this revision.Jan 14 2020, 6:05 AM
gracinet added a subscriber: yuja.Jan 14 2020, 6:10 AM

@yuja I believe this could address the concerns you expressed on D7543

If you prefer to use the version from parsers.c, that's your call

gracinet added a commit: rHGe300421d67f5: revlog-native: introduced ABI version in capsule.Jan 14 2020, 9:22 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGe300421d67f5: revlog-native: introduced ABI version in capsule (authored by gracinet). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/cext/revlog.c (4 lines)
Mrust/hg-cpython/src/cindex.rs (18 lines)

Diff 19207

mercurial/cext/revlog.c

typedef struct indexObjectStruct indexObject; typedef struct indexObjectStruct indexObject;
typedef struct { typedef struct {
int children[16]; int children[16];
} nodetreenode; } nodetreenode;
typedef struct { typedef struct {
int abi_version;
int (*index_parents)(PyObject *, int, int *); int (*index_parents)(PyObject *, int, int *);
} Revlog_CAPI; } Revlog_CAPI;
/* /*
* A base-16 trie for fast node->rev mapping. * A base-16 trie for fast node->rev mapping.
* *
* Positive value is index of the next node in the trie * Positive value is index of the next node in the trie
* Negative value is a leaf: -(rev + 2) * Negative value is a leaf: -(rev + 2)
0, /* tp_descr_set */ 0, /* tp_descr_set */
0, /* tp_dictoffset */ 0, /* tp_dictoffset */
(initproc)rustla_init, /* tp_init */ (initproc)rustla_init, /* tp_init */
0, /* tp_alloc */ 0, /* tp_alloc */
}; };
#endif /* WITH_RUST */ #endif /* WITH_RUST */
static Revlog_CAPI CAPI = { static Revlog_CAPI CAPI = {
/* increment the abi_version field upon each change in the Revlog_CAPI
struct or in the ABI of the listed functions */
1,
HgRevlogIndex_GetParents, HgRevlogIndex_GetParents,
}; };
void revlog_module_init(PyObject *mod) void revlog_module_init(PyObject *mod)
{ {
PyObject *caps = NULL; PyObject *caps = NULL;
HgRevlogIndex_Type.tp_new = PyType_GenericNew; HgRevlogIndex_Type.tp_new = PyType_GenericNew;
if (PyType_Ready(&HgRevlogIndex_Type) < 0) if (PyType_Ready(&HgRevlogIndex_Type) < 0)

rust/hg-cpython/src/cindex.rs

// cindex.rs // cindex.rs
// //
// Copyright 2018 Georges Racinet <gracinet@anybox.fr> // Copyright 2018 Georges Racinet <gracinet@anybox.fr>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
//! Bindings to use the Index defined by the parsers C extension //! Bindings to use the Index defined by the parsers C extension
//! //!
//! Ideally, we should use an Index entirely implemented in Rust, //! Ideally, we should use an Index entirely implemented in Rust,
//! but this will take some time to get there. //! but this will take some time to get there.
use cpython::{PyClone, PyObject, PyResult, Python}; use cpython::{exc::ImportError, PyClone, PyErr, PyObject, PyResult, Python};
use hg::{Graph, GraphError, Revision, WORKING_DIRECTORY_REVISION}; use hg::{Graph, GraphError, Revision, WORKING_DIRECTORY_REVISION};
use libc::c_int; use libc::c_int;
const REVLOG_CABI_VERSION: c_int = 1;
#[repr(C)] #[repr(C)]
pub struct Revlog_CAPI { pub struct Revlog_CAPI {
abi_version: c_int,
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,
} }
py_capsule!( py_capsule!(
/// mechanisms in other contexts. /// mechanisms in other contexts.
pub struct Index { pub struct Index {
index: PyObject, index: PyObject,
capi: &'static Revlog_CAPI, capi: &'static Revlog_CAPI,
} }
impl Index { impl Index {
pub fn new(py: Python, index: PyObject) -> PyResult<Self> { pub fn new(py: Python, index: PyObject) -> PyResult<Self> {
let capi = unsafe { revlog_capi::retrieve(py)? };
if capi.abi_version != REVLOG_CABI_VERSION {
return Err(PyErr::new::<ImportError, _>(
py,
format!(
"ABI version mismatch: the C ABI revlog version {} \
does not match the {} expected by Rust hg-cpython",
capi.abi_version, REVLOG_CABI_VERSION
),
));
}
Ok(Index { Ok(Index {
index: index, index: index,
capi: unsafe { revlog_capi::retrieve(py)? }, capi: capi,
}) })
} }
/// return a reference to the CPython Index object in this Struct /// return a reference to the CPython Index object in this Struct
pub fn inner(&self) -> &PyObject { pub fn inner(&self) -> &PyObject {
&self.index &self.index
} }
} }