Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experience: generating bindings for nextpnr #1167

Closed
Ravenslofty opened this issue Oct 17, 2022 · 1 comment
Closed

Experience: generating bindings for nextpnr #1167

Ravenslofty opened this issue Oct 17, 2022 · 1 comment

Comments

@Ravenslofty
Copy link
Contributor

This is in the spirit of #1073, except it's for nextpnr instead; internally nextpnr has an API that uses a number of more specifically-C++ idioms, without being designed with FFI in mind. So, I knew I was getting in for some trouble, but maybe it would be useful for me to describe what the issues I've ran into were.

error[E0277]: the trait bound `root::GraphicElement: VectorElement` is not satisfied
  --> /home/lofty/nextpnr/common/route/awooter/target/debug/build/awooter-a283d7034f88f2ba/out/autocxx-build-dir/rs/autocxx-ffi-default-gen.rs:1:45356
   |
1  | ...am < root :: DecalId >) -> cxx :: UniquePtr < cxx :: CxxVector < root :: GraphicElement > > { let mut space0 = autocxx :: ValueParamHa...
   |                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `VectorElement` is not implemented for `root::GraphicElement`
   |
   = help: the following other types implement trait `VectorElement`:
             CxxString
             f32
             f64
             i16
             i32
             i64
             i8
             isize
           and 30 others
   = note: required because of the requirements on the impl of `UniquePtrTarget` for `CxxVector<root::GraphicElement>`
note: required by a bound in `UniquePtr`
  --> /home/lofty/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/cxx-1.0.78/src/unique_ptr.rs:17:8
   |
17 |     T: UniquePtrTarget,
   |        ^^^^^^^^^^^^^^^ required by this bound in `UniquePtr`

I would like to file an issue about this, but unfortunately I don't have the hardware to reduce this in a sensible amount of time; would the repro.json be enough by itself to create the problem, or would this need the repo itself uploading?

  • Most of the "public" API functions work on a Context, which inherits from an Arch backend, which (usually) inherits from a BaseArch<R> (where R is something that inherits from ArchRanges), which inherits from ArchAPI<R>, which inherits from BaseCtx. autocxx generates a doc comment about BaseArch/ArchAPI being incomprehensible due to specialisation, which means that functions from BaseCtx (which are generated) are not reachable from a Context. This is something that can be worked around using some FFI shimming, but it's a bit inconvenient. Is this worth filing a bug for, or is this considered a bindgen issue?
  • As mentioned in Implement AsRef for casting to superclasses #592, a big issue here is the lack of being able to convert &mut of a class into an &mut of its base class; since a lot of the important nextpnr API is in Arch rather than Context, this has the side effect of making much of the internal state read-only.
  • Another C++ idiom used in nextpnr is the C++ ForwardIterator interface that involves operator++ and operator*. These functions don't get produced in the autocxx bindings, making it impossible to iterate over C++ items. Again, fixable with some FFI shimming, but inconvenient. I'd file a bug here, except I don't specifically know what these functions would actually get called in Rust-land, so writing a test case is difficult.
@adetaylor
Copy link
Collaborator

Thanks very much for the report!

  • extern_rust_function (calling from C++ to Rust) is less mature than the inverse. I'm doing a lot of cleanup there in Extern rust fun type checking #1168 in response to your test case, thanks! I consider this high priority to fix because extern_rust_function is just very, very bad and needs to be less bad.
  • GraphicElement: yeah a repro.json would be great, I can at least find out whether I can minimize it. Again this seems high priority to investigate - because this is just a bug by the looks of it.
  • Specialization: yes, bindgen has known limitations here, although based on your description I'm not completely sure it's specialization at fault. If you can come up with a standalone test case that reproduces the problem we can have a look. It could, for example, end up being Associated types not resolved to concrete type rust-lang/rust-bindgen#1924 which needs a bit of love to push it over the line and land it (which is not to say that I am ever going to achieve that, I have failed thus far, but it might be a good project for someone else if that turns out to be the cause)
  • Superclass casting: yes this needs sorting out. I don't think this is fundamentally too difficult - just, as noted on Implement AsRef for casting to superclasses #592, it was much easier to get it working for the immutable refs. This is just a matter of spending time on this.
  • Overloaded operators: there is currently no support to expose them in Rust at all, but I'd be happy to consider reasonable proposals to do so. operator++ would probably need to end up being called operator_increment in Rust or something? operator* is tricky - there's an argument that we'd want to implement the Deref trait in Rust but that has implications on method probing so I think for now it would be safer to give it a name like operator_deref or something. I created a placeholder tracking issue for this feature - Support overloaded operators #1169 - but I'm unlikely to work on it any time soon.

I'm going to close this but please do raise new issues for those noted above. Thanks again!

adetaylor added a commit that referenced this issue Oct 20, 2022
Previously extern_rust_function support was highly basic and
didn't usually work at all. This change improevs matters:

* it allows us to emit errors with miette diagnostics from
  this part of the codebase, giving spans in case of trouble
* it validates that the parameters to such a function are
  valid to be used by cxx, as is the return type
* it detects types used as parameters and return type and
  ensures they are not garbage collected elsewhere in autocxx
* it adds a code example.
* we remove a field from RustFun which we didn't actually need
  and replace it with a simple Boolean.

Builds on test case from #1166 (thanks!) and there's a bit of
discussion in #1167.
adetaylor added a commit that referenced this issue Oct 20, 2022
Previously extern_rust_function support was highly basic and
didn't usually work at all. This change improevs matters:

* it allows us to emit errors with miette diagnostics from
  this part of the codebase, giving spans in case of trouble
* it validates that the parameters to such a function are
  valid to be used by cxx, as is the return type
* it detects types used as parameters and return type and
  ensures they are not garbage collected elsewhere in autocxx
* it adds a code example.
* we remove a field from RustFun which we didn't actually need
  and replace it with a simple Boolean.

Builds on test case from #1166 (thanks!) and there's a bit of
discussion in #1167.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants