https://pantsbuild.org/ logo
Powered by
w

witty-crayon-22786

06/22/2019, 12:33 AM
@hundreds-breakfast-49010: hey! so re: your question about `TypeId`s and https://github.com/pantsbuild/pants/issues/7819
depending how many situations you need to do the lookup in, you might consider adding an extern function that does something like: 
fn is_union(TypeId) -> Option<TypeId>
this might be complicated by the fact that we essentially "erase" the type of the union here: https://github.com/pantsbuild/pants/blob/62b566fb55096bc8ef44864667813257e792cb62/src/python/pants/engine/rules.py#L440-L448
h

hundreds-breakfast-49010

06/22/2019, 12:38 AM
interesting, so basically leave the mapping of a type to it's union on the python side
w

witty-crayon-22786

06/22/2019, 12:39 AM
um, maybe? yea. but you could also pass that across to rust
oh, so. here is one wrench: a type might be a member of multiple unions
oh oh.
so: a 
@union
can only possibly by used with the three argument form of 
Get
. example: https://github.com/pantsbuild/pants/blob/62b566fb55096bc8ef44864667813257e792cb62/src/python/pants/rules/core/test.py#L74-L76
what that means is that if this code passes those three arguments across unadulterated: https://github.com/pantsbuild/pants/blob/62b566fb55096bc8ef44864667813257e792cb62/src/python/pants/engine/native.py#L429-L444 ... then we would have two types: the type we were expecting, and the type we were given
that would help enrich this error, because when we fail, we could call back with something like 
fn is_union(TypeId) -> Option<TypeId>
to see whether we failed due to a union, or just a type mismatch
h

hundreds-breakfast-49010

06/24/2019, 6:32 PM
does the 
PyGeneratorResponse::Get
variant directly correspond to the 3-argument Get call in python?
looks like that's ultimately being used on the rust side in 
generator_send
in externs.rs
w

witty-crayon-22786

06/24/2019, 6:36 PM
yep
the two and three are forms are both converted into the three arg form under the hood
h

hundreds-breakfast-49010

06/24/2019, 6:42 PM
I think I've gotten a bit lost in the code here
w

witty-crayon-22786

06/24/2019, 6:43 PM
@hundreds-breakfast-49010: i would say: try reproing the error, and then adding print statements
h

hundreds-breakfast-49010

06/24/2019, 6:43 PM
the ultimate problem to be solved is making the error message in https://github.com/pantsbuild/pants/blob/f7d45dcc0e7ffa3f84d5fc99655a68e4d8ecd564/src/rust/engine/src/nodes.rs#L127 better, right?
w

witty-crayon-22786

06/24/2019, 6:43 PM
yes
h

hundreds-breakfast-49010

06/24/2019, 6:44 PM
ah, yeah
so what's the best way to repro the error?
I still haven't acutaly used pants very much
and I'm not sure how to set up a working example on myown system that will exercise this code path
w

witty-crayon-22786

06/24/2019, 6:44 PM
./pants --no-v1 --v2 test $something-that-is-not-a-python-test
h

hundreds-breakfast-49010

06/24/2019, 6:45 PM
those flags are saying "only use the v2 engine" I assume
can that variable be literally any random string?
oh no it probably has to be a real target
w

witty-crayon-22786

06/24/2019, 6:45 PM
no
yea
h

hundreds-breakfast-49010

06/24/2019, 6:45 PM
so a target in pants can be any file right?
w

witty-crayon-22786

06/24/2019, 6:45 PM
so this should work:
Copy code
./pants --no-v1 --v2 test 3rdparty/jvm/com/fasterxml/jackson/module:scala
sorry, editted
@hundreds-breakfast-49010: no: targets in pants are declared in BUILD files
h

hundreds-breakfast-49010

06/24/2019, 6:46 PM
ah right
w

witty-crayon-22786

06/24/2019, 6:46 PM
here's a better one:
h

hundreds-breakfast-49010

06/24/2019, 6:47 PM
okay so in that BUILD file there's the 
jar_library
function and that has a name and that's where 'scala' comes from
w

witty-crayon-22786

06/24/2019, 6:47 PM
Copy code
./pants --no-v1 --v2 test examples/src/java/org/pantsbuild/example/hello/simple
yep
this target is a 
jvm_binary
, which is definitely not a member of the test target union
so you get noise like
Copy code
Exception: WithDeps(Inner(InnerEntry { params: {Address, OptionsBootstrapper}, rule: Task(Task { product: TestResult, clause: [Select { product: HydratedTarget }], gets: [Get { product: TestResult, subject: PythonTestsAdaptor }], func: coordinator_of_tests(), cacheable: true }) })) did not declare a dependency on JustGet(Get { product: TestResult, subject: JvmBinaryAdaptor })
and in particular, the type that we're trying to use is 
JvmBinaryAdaptor
h

hundreds-breakfast-49010

06/24/2019, 7:01 PM
I don't see JvmBinaryAdaptor in my own output
Copy code
Exception: WithDeps(Inner(InnerEntry { params: {Address, OptionsBootstrapper}, rule: Task(Task { product: TestResult, clause: [Select { product: HydratedTarget }], gets: [Get { product: TestResult, subject
: PythonTestsAdaptor }], func: coordinator_of_tests(), cacheable: true }) })) did not declare a dependency on JustGet(Get { product: TestResult, subject: TargetAdaptor })
w

witty-crayon-22786

06/24/2019, 7:10 PM
See the second command I gave
Copy code
./pants --no-v1 --v2 test examples/src/java/org/pantsbuild/example/hello/simple
It's a better example because the type error is more obvious
h

hundreds-breakfast-49010

06/24/2019, 8:20 PM
so, 
get_type_for
expects an argument of type 
Value
, whihc seems to be a wrapper around a c void pointer
I'm thinking, maybe I want a similar extern fn 
get_union_for
👍 1
and in the ok_or_else block in nodes.rs where the failure happens, I'd want to call that on 
dependency_key
, and that would give me back an Option<TypeId>
if it's Some(x), we have a TypeId corresponding to the Union and we can make the error message do something ueful with it
if it's none, then we just print what we're currently printing
does that make sense?
w

witty-crayon-22786

06/24/2019, 8:34 PM
Yea, sounds good!
You should see the links in the concrete proposal I posted too though: it's possible that you will need to change the rust signature for the generator response 
Get
h

hundreds-breakfast-49010

06/24/2019, 8:48 PM
right that's PyGeneratorResponse::Get
I'm not sure where that variant shows up in 
src/nodes.rs
though
dependency_key
is a 
Get
but that's not the same 
Get
as that variant of 
PyGeneratorResponse
, right?
and if I'm making a new function to call back into the python get the union information for 
dependency_key
, I'm not sure why that would entail changing how 
PyGeneratorResponse::Get
works
but I'm also not sure how to write the union lookup function on the python side or why that needs to be in the python
sorry if these are basic questions I think my mental model of what exactly is going on with these data structures is still incomplete
w

witty-crayon-22786

06/24/2019, 9:06 PM
Try it out, and when you don't see your new error message triggering, you can take another look at my comment
h

hundreds-breakfast-49010

06/25/2019, 5:26 PM
so I think I've got this mostly figured out
except that I'm not sure how to write the FFI function that takes a 
TypeId
on the rust side, and gets back the python object corresponding to the class so I can check for the '__is_union' attribute on it
I'm also not sure how to use the 
_extern_decl
decorator to say that the return type of that function is a rust-level 
Option<TypeId>
I can't tell if the return_type there expects python paramaterized type synax, rust synax, or just doesn't support paramaterized types
w

witty-crayon-22786

06/25/2019, 6:50 PM
the 
decl
is a c-level decl, not rust
because it's used with 
cffi
, which is not rust aware
i'm not sure (@aloof-angle-91616 or @average-vr-56795 might know) whether 
cbindgen
runs before that code, and whether the types it generates would be usable in that position
a

aloof-angle-91616

06/25/2019, 6:53 PM
@hundreds-breakfast-49010 these questions are all good questions, the 
@_extern_decl
thing is not supposed to be anything other than better than before
is there a diff with your current code i can see?
can look into your question async
w

witty-crayon-22786

06/25/2019, 6:54 PM
h

hundreds-breakfast-49010

06/25/2019, 6:54 PM
I'm chatting with john about the design of this right now
but basically I think I want a FFI function that returns an 
Option<TypeId>
into rust
a

aloof-angle-91616

06/25/2019, 6:55 PM
i think 
rust-bindgen
(or just 
bindgen
?) is the crate that generates rust methods from a C header -- i think 
cbindgen
is the inverse
so need to verify this but @witty-crayon-22786 i think it might be 
cbindgen
running first, then the 
@_extern_decl
code is built including the header file from 
cbindgen
w

witty-crayon-22786

06/25/2019, 6:57 PM
ok
hm. so maybe worth experimenting with there. @hundreds-breakfast-49010: you could spend a few minutes adding an Option-like 
enum
on the rust side, and then seeing whether you can use its 
cbindgen
type in that position
the thing that would help with that would be to visually inspect what gets generated by cbindgen (by grepping in the generated code for a keyword like `Broke`: https://github.com/pantsbuild/pants/blob/master/src/python/pants/engine/native.py#L443)
☝️ 1
a

aloof-angle-91616

06/25/2019, 7:00 PM
visually inspecting is how i got 
@_extern_decl
working in the first place
h

hundreds-breakfast-49010

06/25/2019, 7:00 PM
yeah I'll give that a shot, it would be handy to be able to use Option in FFIs, and there's at least one place where we're constructing a rust enum type in pythonanyway
a

aloof-angle-91616

06/25/2019, 7:00 PM
yes, i really agree
h

hundreds-breakfast-49010

06/25/2019, 7:00 PM
I think what's important here is that Option is paramaterized in rust though
an Option-like enum should work the same way as 
PyGeneratorResponse
, right?
but 
PyGeneratorResponse
isn't paramaterized
a

aloof-angle-91616

06/25/2019, 7:01 PM
i don't think we'd be able to (easily) have something return a literal 
Option<T>
for a specific 
Option<u8>
or something that should have a specific set of C code it can correspond to i think
w

witty-crayon-22786

06/25/2019, 7:05 PM
i don't think we'd be able to (easily) have something return a literal 
Option<T>
the confusing thing though is that 
cbindgen
does such a good job with "`enum`s in general"... why would Option not be accounted for?
a

aloof-angle-91616

06/25/2019, 7:05 PM
because C doesn't have templates
i would think e.g. an instance 
Option<u8>
for a given 
T
would be very within scope
w

witty-crayon-22786

06/25/2019, 7:05 PM
sure. and so for enums in general, it generates a tagged union
a

aloof-angle-91616

06/25/2019, 7:05 PM
yes
w

witty-crayon-22786

06/25/2019, 7:05 PM
and Option is just an enum
a

aloof-angle-91616

06/25/2019, 7:05 PM
i recall seeing a post about it it's very good
w

witty-crayon-22786

06/25/2019, 7:06 PM
well, we're using the generated union in the example i linked
(PyGeneratorResponse)
a

average-vr-56795

06/25/2019, 7:06 PM
The reason it doesn’t handle 
Option
is that 
Option
doesn’t have a 
repr
, so doesn’t have a stable representation
a

aloof-angle-91616

06/25/2019, 7:06 PM
yes, @hundreds-breakfast-49010 was asking about the 
PyGeneratorResponse
not being parameterized, and i was saying that in the context of thinking it might be a blocker to making 
PyGeneratorResponse
parameterizable
a

average-vr-56795

06/25/2019, 7:07 PM
And in fact rust does null-pointer optimising of non-nullable options for space packing
h

hundreds-breakfast-49010

06/25/2019, 7:07 PM
Option is just an enum, but it's a parameterized enum
👍 1
w

witty-crayon-22786

06/25/2019, 7:07 PM
got it.
a

aloof-angle-91616

06/25/2019, 7:07 PM
i didn't make that part clear, thanks
w

witty-crayon-22786

06/25/2019, 7:07 PM
because we can't mark it 
repr(c)
, basically
ok. so then creating your own enum and marking it 
repr(c)
would work
h

hundreds-breakfast-49010

06/25/2019, 7:08 PM
rust does the null-pointer optimizing thing with Option<&T> because &T is a non-nullable pointer
w

witty-crayon-22786

06/25/2019, 7:08 PM
thanks, got it now.
a

average-vr-56795

06/25/2019, 7:08 PM
Yeah, I think so - I think cbindgen handles parameterised ones fine, as long as they have a repr
h

hundreds-breakfast-49010

06/25/2019, 7:08 PM
but in general it can't assume for any Option<T> that the 0 representation of T is illegal
w

witty-crayon-22786

06/25/2019, 7:08 PM
@hundreds-breakfast-49010: the 
repr(c)
bit is the overriding factor.
because when you mark it 
repr(c)
you disable a lot of that.
a

aloof-angle-91616

06/25/2019, 7:09 PM
rust's type system can make handling edge cases within the system not the worst thing ever
(you're all referring to the rust 
#[repr(C)]
, right??)
@hundreds-breakfast-49010 please let me/us know if the above didn't make it clear enough how to muck about with the 
@_extern_decl
magic, can ping this slack for any qs
w

witty-crayon-22786

06/25/2019, 7:34 PM
yea, that
h

hundreds-breakfast-49010

06/25/2019, 7:47 PM
sorry was just eating lunch
so, I might need to make my own type that is structurally the same as an Option<T>
that's probably the simplest thing to do right now to get this feature in
I assume 
enum OptionalType { None, Some(TypeId) }
can have a 
#[repr(C)]
?
w

witty-crayon-22786

06/25/2019, 8:28 PM
Yep, I think so.
Could be 
ExternOption
or something
h

hundreds-breakfast-49010

06/25/2019, 11:35 PM
https://github.com/gshuflin/pants/commit/9c85bd7ebf7907f421017a57e1d7bb34d2e79b51 <- this is blowing up at the cffi stage and I'm not sure why
Copy code
--- stderr
Traceback (most recent call last):
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/cffi/cparser.py", line 276, in _parse
    ast = _get_parser().parse(fullcsource)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/pycparser/c_parser.py", line 152, in parse
    debug=debuglevel)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/pycparser/ply/yacc.py", line 331, in parse
    return self.parseopt_notrack(input, lexer, debug, tracking, tokenfunc)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/pycparser/ply/yacc.py", line 1199, in parseopt_notrack
    tok = call_errorfunc(self.errorfunc, errtoken, self)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/pycparser/ply/yacc.py", line 193, in call_errorfunc
    r = errorfunc(token)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/pycparser/c_parser.py", line 1848, in p_error
    column=self.clex.find_tok_column(p)))
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/pycparser/plyparser.py", line 67, in _parse_error
    raise ParseError("%s: %s" % (coord, msg))
pycparser.plyparser.ParseError: <cdef source string>:3:18: before: extern_get_union_for

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
  File "/home/gregs/code/pants/src/python/pants/engine/native.py", line 148, in bootstrap_c_source
    ffibuilder.cdef(_FFISpecification.format_cffi_externs())
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/cffi/api.py", line 107, in cdef
    self._cdef(csource, override=override, packed=packed)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/cffi/api.py", line 121, in _cdef
    self._parser.parse(csource, override=override, **options)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/cffi/cparser.py", line 315, in parse
    self._internal_parse(csource)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/cffi/cparser.py", line 320, in _internal_parse
    ast, macros, csource = self._parse(csource)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/cffi/cparser.py", line 278, in _parse
    self.convert_pycparser_error(e, csource)
  File "/home/gregs/code/pants/build-support/pants_dev_deps.py37.venv/lib/python3.7/site-packages/cffi/cparser.py", line 307, in convert_pycparser_error
    raise CDefError(msg)
cffi.error.CDefError: cannot parse "OptionalTypeId      extern_get_union_for(ExternContext*, Handle*);"
<cdef source string>:3:18: before: extern_get_union_for
Execution of "/home/gregs/code/pants/build-support/bin/native/bootstrap_cffi.sh" failed with exit code Some(1)
you implied I think there was a way to see what the generated output of CFFI on the rust code with respect to that new 
OptionalTypeId
was?
a

aloof-angle-91616

06/25/2019, 11:37 PM
check the output of 
ls src/rust/engine/src/cffi/
the 
scheduler.h
is i think generated from cbindgen and the 
native_engine.c
is something we generate from 
@_extern_decl
i believe
h

hundreds-breakfast-49010

06/25/2019, 11:42 PM
hm maybe i need to explicitly build the rust or it won't pick up on the new type?
a

aloof-angle-91616

06/25/2019, 11:43 PM
"explicitly build the rust" meaning?
h

hundreds-breakfast-49010

06/25/2019, 11:43 PM
hm, no, 
./build-support/bin/native/cargo build --manifest-path src/rust/engine/process_execution/Cargo.toml
doesn't seem to add the string 
OptionalTypeId
to any of those files
a

aloof-angle-91616

06/25/2019, 11:44 PM
where is 
OptionalTypeId
coming from?
h

hundreds-breakfast-49010

06/25/2019, 11:45 PM
that's the new type I defined as #[repr(C)]
in src/engine/src/externs.rs
as far as I can tell I'm doing it hte same way as 
PyGeneratorResponse
a

aloof-angle-91616

06/25/2019, 11:49 PM
i would run 
./pants -V
in case it's doing some invalidation that the raw cargo build command isn't
and then recheck the output
h

hundreds-breakfast-49010

06/25/2019, 11:49 PM
what's the -V flag for?
a

aloof-angle-91616

06/25/2019, 11:49 PM
or deleting the file
it makes pants say the version
same as 
./pants --version
a bit confusing
h

hundreds-breakfast-49010

06/25/2019, 11:51 PM
deleting the file and regenerating doesn't make 
OptionalTypeId
show up there
neither does running ./pants -V
a

aloof-angle-91616

06/25/2019, 11:55 PM
can you try making it a struct, or can you make the 
NoTypeId
case contain a value? it's possible the cbindgen settings require a flag for that to be on. are you able to edit any of the other structs in the file and see the change in output?
hm
i was going to say it's possible you need to define a 
#[no_mangle] pub extern fn
to go along with it to be visible
not sure
h

hundreds-breakfast-49010

06/25/2019, 11:57 PM
hm, editing hte names of existing types (PyGeneratorResponse) and running ./pants --version doesn't seem to generate a new file
a

aloof-angle-91616

06/25/2019, 11:59 PM
there's a set of things you need to clear
i run
(i am aware this is absurd)
git clean -xfd src/ && rm -rf ~/.cache/pants/bin/native-engine
which will take a while to recompile rust unless you set 
MODE=debug
because the current rust bootstrap process in the pants repo copies over the native engine binary multiple places we have to clean out more than we might want
h

hundreds-breakfast-49010

06/26/2019, 12:01 AM
kk trying that
yeah that regenerates only 
scheduler.h
which still lacks my new type
a

aloof-angle-91616

06/26/2019, 12:06 AM
is there any entry for the function you added?
h

hundreds-breakfast-49010

06/26/2019, 12:07 AM
no there's not
that's the 
GetUnionForExtern
I added
there is an entry for 
GetTypeForExtern
which is definedin basically the same way right next to it in the file
in 
<http://externs.rs|externs.rs>
I mean
a

aloof-angle-91616

06/26/2019, 12:08 AM
showing a diff of 
scheduler.h
before and after your changes might be helpful
i'm p sure i know
i think it's symbols exposed in 
<http://lib.rs|lib.rs>
(so just add your types to that 
use
at the top of 
src/rust/engine/src/lib.rs
?)
oh hm
oh no i think that might be it
because i think the rest are exposed via being arguments to the
Copy code
#[no_mangle]
pub extern "C" fn scheduler_create(
h

hundreds-breakfast-49010

06/26/2019, 12:13 AM
yeah the diffs between the two versoins of scheduler.h are null
oh 
externs_set
hm
a

aloof-angle-91616

06/26/2019, 12:14 AM
don't have it in front of me right now but https://github.com/eqrion/cbindgen/blob/master/docs.md#writing-your-c-api explicitly says types are exposed transitively via extern fns
h

hundreds-breakfast-49010

06/26/2019, 12:14 AM
I think that might've been it
yeah that generated the types in scheduler.h
🎉 1
🔥 1
a

aloof-angle-91616

06/26/2019, 12:16 AM
is this enough to unblock you?
h

hundreds-breakfast-49010

06/26/2019, 12:18 AM
yup, thanks 🙂
a

aloof-angle-91616

06/26/2019, 12:19 AM
ok, great! let me know if we can answer further questions. nobody should be pretending this is at all obvious, this is all stuff we made up
h

hundreds-breakfast-49010

06/26/2019, 12:25 AM
it takes time to get familiar with a new codebase
I already feel like Iknow more than I did on friday 🙂
👌 1
a

aloof-angle-91616

06/26/2019, 12:26 AM
i also said that in order to impress upon you how a lot of what we do is incredibly groundbreaking
everyone else too
but pants is cool
a

average-vr-56795

06/26/2019, 1:41 PM
there’s a set of things you need to clear
You shouldn’t need to do this, ever. Any time you do, please find out why and fix it. @aloof-angle-91616
(Or ask for help in doing so)
h

hundreds-breakfast-49010

06/26/2019, 7:30 PM
https://github.com/gshuflin/pants/commit/e08dac59d7b4923ca8591ea1f4e1cd0c8b58cc09 <- with this code, I'm running into the following FFI error
Copy code
Fatal Python error: ffi.from_handle() detected that the address passed points to garbage. If it is really the result of ffi.new_handle(), then the Python object has already been garbage collected
                                                                                                          
Current thread 0x00007dca243f6700 (most recent call first):                                               
  File "/home/gregs/code/pants/src/python/pants/engine/native.py", line 293 in extern_get_union_for
as far as I can tell I'm using my new 
extern_get_union_for
function in the same way as other python extern functions
the 
context_handle
is being provided by the same 
with_externs
helper that every other rust function in 
<http://externs.rs|externs.rs>
is using
am I doing anything obviously wrong?
a

aloof-angle-91616

06/26/2019, 7:37 PM
You shouldn’t need to do this, ever. Any time you do, please find out why and fix it.
any time i try to describe RTB we really need to be doing, including specifically with respect to the rust build, it gets shot down, so i get to do it on my own time. i did say specifically it was a huge hack in order to avoid creating the impression that we think this is ok.
sorry about that
i'm looking now
this is failing at the first 
from_handle()
call in that method?
what is the full stack trace?
the first thing i see is that no other extern is receiving anything complex except for a 
Handle
seeing why that would be an issue
h

hundreds-breakfast-49010

06/26/2019, 7:42 PM
there's at least one other one that's receiving a TypeId
a

aloof-angle-91616

06/26/2019, 7:42 PM
got it
receiving a TypeId?
h

hundreds-breakfast-49010

06/26/2019, 7:43 PM
yeah 
extern_type_to_str
a

aloof-angle-91616

06/26/2019, 7:45 PM
i am silly yes
can you provide the pants command line and full output? just bootstrapping pants isn't reproing
h

hundreds-breakfast-49010

06/26/2019, 7:50 PM
what pastebin does this project use?
a

aloof-angle-91616

06/26/2019, 7:50 PM
that is a question i have never heard before! we are based on github so gist seems appropriate
h

hundreds-breakfast-49010

06/26/2019, 7:51 PM
a

aloof-angle-91616

06/26/2019, 8:15 PM
ok, got distracted, but i repro. investigating
h

hundreds-breakfast-49010

06/26/2019, 8:15 PM
cool
is it possible that the *const ExternContext pointer is invalid because I'm calling this in error handling?
a

aloof-angle-91616

06/26/2019, 8:16 PM
that sounds very reasonable, and i am paging into what the extern context is exactly
ah
the type is possibly not wrapped correctly (even though i see no obvious issues)
the explanation why this is only breaking for your method is possibly because type_to_str hasn't been used in this code path yet. seeing if that's true
h

hundreds-breakfast-49010

06/26/2019, 8:46 PM
so, maybe type_to_str would break in the same way if used here?
a

aloof-angle-91616

06/26/2019, 8:46 PM
yes, although i don't think i'm seeing that. adding another 
eprintln!()
to 
gen_get
rn
hm. it appears to be failing specifically at this
with (in 
gen_get()
):
Copy code
let declared_type_union = get.declared_subject
                .and_then(|ty| {
                  let t = externs::type_to_str(ty);
                  eprintln!("this type??? {:?}", t);
                  externs::get_union_for(ty)
                });
              eprintln!("declared: {:?}", declared_type_union);
i am seeing:
Copy code
this type??? "TestTarget"
`
then the same fatal python error immediately after
h

hundreds-breakfast-49010

06/26/2019, 8:48 PM
so type_to_str works but not get_union_for
a

aloof-angle-91616

06/26/2019, 8:48 PM
it seems so
h

hundreds-breakfast-49010

06/26/2019, 8:48 PM
hm
I'm gonna add some prints in the python to see exactly where that fails
a

aloof-angle-91616

06/26/2019, 8:49 PM
good call
the context you mentioned might be it (we can get a write lock on it using 
with_externs_exclusive
, i'll try that next although i don't think that's it). it might also be because (and this would be ridiculous) the cffi-generated C in 
native_engine.c
has this conditional based on 
sizeof()
which might be different
i'm seeing it fail at the very first line 
c = self._ffi.from_handle(context_handle)
the backtrace gives line numbers
h

hundreds-breakfast-49010

06/26/2019, 8:52 PM
yeah same
oh so maybe I need to write my wrapper around the extern differently?
a

aloof-angle-91616

06/26/2019, 8:53 PM
that would be the first idea, only because i happened to spy a 
with_externs_exclusive()
method and since we happen to have a bad context maybe a write lock would help
h

hundreds-breakfast-49010

06/26/2019, 8:53 PM
where's this conditional in native_engine.c?
a

aloof-angle-91616

06/26/2019, 8:54 PM
in the generated file in 
src/rust/engine/src/cffi/native_engine.c
, there's:
Copy code
static struct _cffi_externpy_s _cffi_externpy__extern_get_type_for =
  { "native_engine.extern_get_type_for", (int)sizeof(TypeId) };

static TypeId extern_get_type_for(void * a0, void * * a1)
{
  char a[sizeof(TypeId) > 16 ? sizeof(TypeId) : 16];
  char *p = a;
  *(void * *)(p + 0) = a0;
  *(void * * *)(p + 8) = a1;
  _cffi_call_python(&_cffi_externpy__extern_get_type_for, p);
  return *(TypeId *)p;
}

static struct _cffi_externpy_s _cffi_externpy__extern_get_union_for =
  { "native_engine.extern_get_union_for", (int)sizeof(OptionalTypeId) };

static OptionalTypeId extern_get_union_for(void * a0, TypeId a1)
{
  char a[sizeof(OptionalTypeId) > 16 ? sizeof(OptionalTypeId) : 16];
  char *p = a;
  *(void * *)(p + 0) = a0;
  *(TypeId * *)(p + 8) = &a1;
  _cffi_call_python(&_cffi_externpy__extern_get_union_for, p);
  return *(OptionalTypeId *)p;
}
and like idk if 
sizeof(OptionalTypeId)
happens to be greater than 16, or if that's different compared to 
TypeId
i do not expect this to be the issue right now
also going to google for this error
h

hundreds-breakfast-49010

06/26/2019, 9:09 PM
using with_externs_exclusive doesn't prevent the error
a

aloof-angle-91616

06/26/2019, 9:09 PM
yes, i also found that
i tried printing out the value of the 
context_handle
before calling 
.from_handle()
for 
type_to_str
and the subsequent 
get_union_for
, and it is different
but that might be 100% expected if i knew more about cffi
i need to switch to something else now but basically: (1) after applying https://pantsbuild.slack.com/archives/C0D7TNJHL/p1561582043101900?thread_ts=1561163619.038700&amp;cid=C0D7TNJHL (2) adding 
<http://logger.info|logger.info>('h: {}'.format(context_handle))
to the top of 
extern_get_union_for
(3) adding 
<http://logger.info|logger.info>('i: {}'.format(context_handle))
to the top of 
extern_type_to_str
(4) running 
RUST_BACKTRACE=full ./pants --no-v1 --v2 test examples/src/java/org/pantsbuild/example/hello/simple
(5) i see
Copy code
21:16:15 [INFO] i: <cdata 'void *' 0x110d02728>
21:16:15 [INFO] Starting tests: examples/src/java/org/pantsbuild/example/hello/simple
21:16:15 [INFO] i: <cdata 'void *' 0x110d02728>
this type??? "TestTarget"
21:16:15 [INFO] h: <cdata 'void *' 0x70000dcb9d58>
Fatal Python error: ffi.from_handle() detected that the address passed points to garbage. If it is really the result of ffi.new_handle(), then the Python object has already been garbage collected

Thread 0x000070000d4b4000 (most recent call first):

Current thread 0x000070000dcc0000 (most recent call first):
  File "/Users/dmcclanahan/tools/pants-v3/src/python/pants/engine/native.py", line 294 in extern_get_union_for
(6) turning the 
self._ffi.from_handle()
to use 
.new_handle()
fails with 
illegal hardware instruction
(7) mainly, i do not understand why the context object would be different when passed to 
extern_get_union_for
(as shown in the above output) immediately after being passed to 
extern_type_to_str
and there appear to be no successful invocations of 
externs::get_union_for
whatsoever
h

hundreds-breakfast-49010

06/26/2019, 9:28 PM
right, 
get_union_for
is the new function that I'm adding with this commit, meant to be used right here
Powered by