https://pantsbuild.org/ logo
#development
Title
# development
a

aloof-angle-91616

06/26/2019, 9:43 PM
and that's why i was thinking about the
sizeof
thing in the
native_engine.c
generated file
w

witty-crayon-22786

06/26/2019, 9:45 PM
mm, yea.
a

aloof-angle-91616

06/26/2019, 9:45 PM
i actually think this might be it and will look at it later
w

witty-crayon-22786

06/26/2019, 9:46 PM
thank you!
h

hundreds-breakfast-49010

06/26/2019, 9:48 PM
what flaw do you notice in the C?
a

aloof-angle-91616

06/26/2019, 9:49 PM
the size of
a
is determined by the return type, and then both arguments are stuffed into
p
, which is the same as
a
is all
not a flaw but it seems like exactly what would cause corruption of the first argument
i don't know if editing that file causes recompilation of cargo, it might but then it might get overwritten again
can try to add something hacky there to see if extending the size of
a
in
extern_get_union_for
unconditionally fixes the error
w

witty-crayon-22786

06/26/2019, 9:52 PM
but yea: Danny: agreed with what you said earlier: other things to focus on right now.
@hundreds-breakfast-49010: would timebox getting this particular strategy working, and swap to another one as in https://pantsbuild.slack.com/archives/C0D7TNJHL/p1561584224108300
h

hundreds-breakfast-49010

06/26/2019, 9:53 PM
kk
a

aloof-angle-91616

06/27/2019, 12:46 AM
i have made it stop yelling about memory by converting
OptionalTypeId
into a
pub struct
under the hypothesis that the memory required for this enum is somehow getting calculated incorrectly
am trying to logically bisect the changes
fwiw this is my current working diff
Copy code
diff --git a/src/python/pants/engine/native.py b/src/python/pants/engine/native.py
index 055764c..d96ca2b 100644
--- a/src/python/pants/engine/native.py
+++ b/src/python/pants/engine/native.py
@@ -290,16 +290,14 @@ class _FFISpecification(object):
   def extern_get_union_for(self, context_handle, type_id):
     """Return an optional union if the type belongs to one"""
 
+    <http://logger.info|logger.info>('h: {}'.format(context_handle))
     c = self._ffi.from_handle(context_handle)
     input_type = c.from_id(type_id.tup_0)
 
     build_configuration = self.build_configuration
     print("BUILD: {}".format(build_configuration))
 
-    response = self._ffi.new('OptionalTypeId*')
-    response.tag = self._lib.NoTypeId
-    response.no_type_id = ()
-    return response[0]
+    return OptionalTypeId(type_id=type_id.tup_0, is_some=False)
 
   @_extern_decl('Ident', ['ExternContext*', 'Handle*'])
   def extern_identify(self, context_handle, val):
@@ -335,6 +333,7 @@ class _FFISpecification(object):
   @_extern_decl('Buffer', ['ExternContext*', 'TypeId'])
   def extern_type_to_str(self, context_handle, type_id):
     """Given a TypeId, write type.__name__ and return it."""
+    <http://logger.info|logger.info>('i: {}'.format(context_handle))
     c = self._ffi.from_handle(context_handle)
     return c.utf8_buf(text_type(c.from_id(type_id.tup_0).__name__))
 
@@ -488,6 +487,10 @@ class TypeId(datatype(['tup_0'])):
   """Corresponds to the native object of the same name."""
 
 
+class OptionalTypeId(datatype(['type_id', 'is_some'])):
+  """Corresponds to the native object of the same name."""
+
+
 class PyResult(datatype(['is_throw', 'handle'])):
   """Corresponds to the native object of the same name."""
 
diff --git a/src/rust/engine/src/externs.rs b/src/rust/engine/src/externs.rs
index f8799fb..818cf2e 100644
--- a/src/rust/engine/src/externs.rs
+++ b/src/rust/engine/src/externs.rs
@@ -26,8 +26,7 @@ pub fn get_type_for(val: &Value) -> TypeId {
 
 pub fn get_union_for(ty: TypeId) -> Option<TypeId> {
   with_externs(|e| match (e.get_union_for)(e.context, ty) {
-    OptionalTypeId::NoTypeId => None,
-    OptionalTypeId::SomeTypeId(type_id) => Some(type_id),
+    OptionalTypeId(type_id, is_some) => if is_some { Some(type_id) } else { None },
   })
 }
 
@@ -448,10 +447,7 @@ impl From<Result<(), String>> for PyResult {
 ///This is basically an Option<TypeId>, but manually-monomorphized
 ///to make it work more smoothly with the Python FFI
 #[repr(C)]
-pub enum OptionalTypeId {
-  NoTypeId,
-  SomeTypeId(TypeId),
-}
+pub struct OptionalTypeId(TypeId, bool);
 
 ///
 /// The response from a call to extern_generator_send. Gets include Idents for their Handles
diff --git a/src/rust/engine/src/nodes.rs b/src/rust/engine/src/nodes.rs
index a73fed2..7217492 100644
--- a/src/rust/engine/src/nodes.rs
+++ b/src/rust/engine/src/nodes.rs
@@ -864,7 +864,8 @@ impl Task {
           .ok_or_else(|| throw(&format!("no edges for task {:?} exist!", entry)))
           .and_then(|edges| {
             edges.entry_for(&dependency_key).cloned().ok_or_else(|| {
-              let declared_type_union = get.declared_subject.and_then(|ty| externs::get_union_for(ty));
+              let declared_type_union = get.declared_subject
+                .and_then(|ty| externs::get_union_for(ty) );
               match declared_type_union {
                 Some(union_ty) => {
                   let description = "foo";
yeah this is really sad but i might use
pub struct OptionalTypeId(TypeId, bool);
as above because it's not immediately clear to me why the
enum
approach is failing
i tried having
native.py
regex-replace the
char a[...]
array for
extern_get_union_for()
in
native_engine.c
by adding a line to the
_hackily_recreate_includes_for_bindings()
method in
native.py
changing the size of any arrays i tried to modify in the generated code did not work
this would possibly be a good issue to make against the cffi project, but would really want to understand just how the enum is laid out in memory (and then whether the hypothesis is correct that it's clobbering the context somehow)
no worries if you want to keep digging, just want to unblock you if that's the case
could also just go with specifying a particular
TypeId
value (0?) to represent the "null" case as @witty-crayon-22786 suggested, which would be "idiomatic" enough if we were writing C. risk of collision seems small / can maybe be avoided entirely since i think we generate the ids ourself somewhere in native.py?
that would also be not as large aka more efficient but like unclear if that's an issue at the FFI right now