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

C api #972

Closed
Pajn opened this issue Aug 21, 2019 · 18 comments
Closed

C api #972

Pajn opened this issue Aug 21, 2019 · 18 comments
Labels

Comments

@Pajn
Copy link
Contributor

Pajn commented Aug 21, 2019

Sorry for posting a question as an issue but I don't know where else to turn. Mir does not seem to have an obvious place for discussion.

A lot of the Mir API seems to be in term of C++ classes, is that the only API or are there a C API below that could be used to implement compositors?

If there isn't, do you know how usable the C++ API are for ffi, for example are Mir classes generally relocatable, trivially copyable or otherwise have properties making them safe to use via raw pointers?

@AlanGriffiths
Copy link
Collaborator

This was last discussed on the Mir forum here: https://discourse.ubuntu.com/t/mir-news-17th-august-2018/7618/4

As you observe, the principle server-side API (libmiral-dev) is defined by C++ classes. There is no C API other than the deprecated client-side API (libmirclient-dev) - and that was a C wrapper layer over C++ code.

The majority of classes in the server-side API are not trivially copyable, and the API user is expected to implement interface classes, use lamdas and other C++ features.

IMO it would be possible (but far from trivial) to write a wrapper that hides these details behind opaque pointers. If your interest relates to a specific part of the functionality, and not "everything" that could reduce the scope and make ffi for that sub-domain more achievable.

@Pajn
Copy link
Contributor Author

Pajn commented Aug 21, 2019

I don't know enough about Wayland in general nor Mir specifically to make an educated guess on what I would need at this state, but at a very minimum I would have to implement a custom window manager.

What I want to do is implement a DE and I want to write as much as possible of it in Rust (for memory safety and ergonomics) and as little as possible in C++ (because I don't know it and aren't very interested in learning about it).

The WindowManagerPolicy could just call away to functions but with things like optional_value being a generic class I'm unsure if I could consume a struct directly or would need to copy everything over to a more portable structure first.

I'm basically trying to figure out if building on top of Mir would be feasible in my position. I have played around with wlroots and while it has been wonders for a basic POC I would prefer Mir because of things like Nvidia support and in general not refusing to support common protocols or hardware.

@AlanGriffiths
Copy link
Collaborator

Agreed, you couldn't consume a C++ type directly. I'll take miral::WindowSpecification as an example here.

What you could do is write wrapper functions like:

extern "C" bool get_top_left(struct mir_window_specification const* window_specification, struct mir_point* result)
{
    if (auto const tl = window_specification->top_left())
    {
        result->x = tl.value().x.as_int();
        result->y = tl.value().y.as_int();
        return true;
    }
    else
    {
        return false;
    }
}

Where mir_window_specification is an opaque type in the wrapper API that's internally convertible to WindowSpecification and struct mir_point { int x; int y; };.

There's a lot of repetitive code, so I'd try generating it.

@wmww
Copy link
Contributor

wmww commented Aug 21, 2019

My opinion is that wrapping the MirAL API is probably not worth it. It makes more sense to write the window manager, implementations of any bespoke Wayland protocols and other things that must interface directly with Mir APIs in C++. Then, use the language and toolkit of your choice for other shell components.

Since Mir implements Layer Shell, things like panels, notifications, lockscreens and launchers could be built in Rust + GTK (or any language with GTK bindings).

You don't have to use GTK or Layer Shell. Mir's support for Layer Shell and GTK Layer Shell make it easy, but you could also use a different protocol or create your own protocols and implement them as bespoke protocols for Mir. The point is that most of the bits of the shell run outside of the Mir process.

In the scheme of things, those out-of-process shell components will be a much more significant portion of work than the window manager*. You probably wont need to change the example floating window management policy much.

* Unless you wanted a floating wm or something fundamentally novel. In that case there would be more C++ wm work, and maybe it would be worth wrapping Mir APIs if you really hated C++.

@Pajn
Copy link
Contributor Author

Pajn commented Aug 21, 2019

Thanks for the hints. I managed to create a POC where I forked egmde and added two pieces of logic via rust:

  1. Windows are located at the top left of the monitor
  2. Windows can be moved horizontally via ctrl + left/right

I used bindgen to generate bindings (I did not know that it supported C++) and cbindgen to generate a header file for calling into my Rust functions from the WMPolicy. The C++ bindings are incredibly ugly but kind of work. I did get some problems though, I can't call miral::WindowManagerTools::active_window() from Rust. I can call some methods on WindowManagerTools but not all. It seems to be methods that return a struct value that are problematic. This could possibly be rust-lang/rust-bindgen#778

https://github.com/Pajn/egmde/blob/master/rust_wm/src/lib.rs

wmww, you might be correct in that doesn't make sense, and getting run-time crashes when calling certain methods makes that seem like the case.

@RAOF
Copy link
Contributor

RAOF commented Aug 21, 2019 via email

@wmww
Copy link
Contributor

wmww commented Aug 22, 2019

It is true that some shell features don't work very well when implemented out-of-process. The obvious example of this is exposé, However you try to design it as an out-of-process feature, there's always a serious problem.

That being said, how to do exposé in Mir is still an open question. AFAIK, the current MirAL APIs are not enough to implement it well either, so you'd need to patch Mir or use unstable APIs. Once we decide the correct way to support exposé in C++ shells, it will be more obvious if that's another piece that may need language bindings.

@RAOF
Copy link
Contributor

RAOF commented Aug 22, 2019

Right; this works for qtmir (for Unity8) because qtmir links with non-MirAL parts of Mir.

@AlanGriffiths
Copy link
Collaborator

Useful discussion, but I fear it is splitting into two topics:

The digression: Yes, there are features that are missing in libmiral. We've had several in the wishlist for a while waiting for the right time to address them.

The original topic:. Because languages do not have representations of all of the same abstractions binding generators will always need some help or customization. (We didn't restrict the miral API to a dialect that works with binding generators.)

I agree with Chris that C bindings would be a great feature, but we're not going to be able to spend our time on that for the foreseeable future. In the meantime, generating Rust bindings seems to have got you part way.

To get further you will need to write some, hopefully simple, C++ code to access the features that the bindings don't support (or adjust the generated code to work by hand). I've no experience with the specific generators you mention, so I don't know what the working dialect is but, to take the miral::WindowManagerTools::active_window() example you mentioned maybe something like this:

void mirffi_helpers::WindowManagerTools::active_window(miral::Window& window, miral::WindowManagerTools const& tools) const
{
    window = tools.active_window();
}

Let us know how you get on, and if we can help further.

@Pajn
Copy link
Contributor Author

Pajn commented Aug 23, 2019

Instead of relying completely on MirAls datastructures for the current state I have started to keep track of (part of) the world myself. This gets around problem methods like active_window and also cleans up the Rust side quite a bit because of the reduced need for FFI and with that unsafe code.
So far it actually seems much less of a problem than what is written all around the internet about C++ APIs. Of course I have only used a small part of the API yet and dragons may lie ahead.
I have yet to solve the active_window problem which seems to be a general problem with giving Rust a class instance as a value and not a pointer to a value (and maybe the smart pointers or destructor is somehow related). I will have to ask the Rust community what the problem might be because I have yet to succeed with workarounds like the above either.

The exposé discussion is interesting. While the specifics feels like a far away topic at the moment Mir in general is a bit lacking in open discussions about what is missing, what is to come and things of that nature. I assume most of them is held on internal Canonical channels which make Mir feel a bit more secret than I assume its meant to be.

@RAOF
Copy link
Contributor

RAOF commented Aug 23, 2019

I am also not familiar with binding C++ and Rust, but it wouldn't surprise me if binding references was non-obvious; does the semantically-equivalent modification of Alan's suggestion above work?

void mirffi_helpers::WindowManagerTools::active_window(miral::Window* window, miral::WindowManagerTools const* tools) const
{
    *window = tools->active_window();
}

The miral::Window& in the original is passing a class instance as a pointer rather than by value; it's just using C++'s references (which are basically just sugar around an non-null, automatically-dereferenced pointer).

As for Mir feeling a bit secret - hm. It's not meant to be 😬
Some of this is going to be that there aren't a lot of projects using Mir deeply at the moment; the only fully-featured shell written using Mir that I'm aware of is Unity8, and I don't think the UBports folk feel that we're opaque.

We're reasonably active on the Mir section of the Ubuntu Discourse; looking at mir-server.io I see that we don't actually link there! That's something we should fix! We should also point people at #mir-server on Freenode.

bors bot added a commit that referenced this issue Aug 23, 2019
974: doc: Mention our community resources r=Saviq a=RAOF

Hopefully this, along with some other website changes, will result in fewer people being unable to find a forum to ask us questions! (cf: issue #972)

Co-authored-by: Christopher James Halse Rogers <[email protected]>
@AlanGriffiths
Copy link
Collaborator

looking at mir-server.io I see that we don't actually link there! That's something we should fix! We should also point people at #mir-server on Freenode.

We do link to both: https://github.com/canonical-web-and-design/mir-server.io/blob/5f00eab4a73e7df86a6de264bba2198728f71dfa/index.html#L202

(It's "below the fold", which I don't like, but it is there.)

@AlanGriffiths
Copy link
Collaborator

I have yet to solve the active_window problem which seems to be a general problem with giving Rust a class instance as a value and not a pointer to a value (and maybe the smart pointers or destructor is somehow related). I will have to ask the Rust community what the problem might be because I have yet to succeed with workarounds like the above either.

There are several things that may contribute here. In particular, this member function is returning an class with user defined copy semantics. There are a number of ways around that, but all the ones I can think of involve writing C++ code (ideally with a custom code generator).

(Today's idea for a wrapper is to wrap functions returning a smart pointer with ones that return a raw pointer, cache the smart pointer on the C++ side (so that the pointer remains valid) and clear the cache in advise_end().)

@Pajn
Copy link
Contributor Author

Pajn commented Aug 23, 2019

does the semantically-equivalent modification of Alan's suggestion above work?

I'm pretty sure I tried exactly that as well. In this case my guess (that may be very wrong) is a problem with how I allocated space for the pointer. I first tried allocating with in Rust and then in C but could not get it to work. I can't see why allocating it in C an passing a pointer shouldn't work though so I might have screwed something up.
Receiving a reference via https://github.com/MirServer/mir/blob/master/src/miral/window_info.cpp#L581 (return type miral::WindowInfo&) works fine and I can use for example the top_left method. so there is something up with having ownership of instance. The custom copy semantics sounds like a likely reason as instances of optional_value and mir::geometry::Point haven't caused any problems. I'll experiment and see if I can confirm that and if it might be possible to get that working via the bindings, otherwise AlanGriffiths cache idea sounds reasonable.

As for Mir feeling a bit secret - hm. It's not meant to be

I definitely did not mean for that as a critic! It was just an observation from (as you said) there aren't a lot of projects using Mir. I had overlooked the #mir-server channel, I'll make sure to check that out. The clarification that discourse also is a good place for questions was nice. From the current content I got the feeling that is was more a channel for official communication from the Mir team.

@AlanGriffiths
Copy link
Collaborator

I can't see why allocating it in C an passing a pointer shouldn't work though

I can. It is not expecting raw memory, but an initialized object.

That could be worked around with "placement new", but that simply stores up problems for finalization. So I don't suggest doing that.

@Pajn
Copy link
Contributor Author

Pajn commented Aug 23, 2019

I naively thought that as I had an initialized object I just needed to store it somewhere.

I tried your suggestion with smart pointers and managed to get it working.

Here are the C++ code:

extern "C" void* get_active_window(miral::WindowManagerTools* tools)
{
    Window window = tools->active_window();
    std::shared_ptr<Window> window_ptr = std::make_shared<Window>();
    *window_ptr = window;
    return static_cast<void*>(new std::shared_ptr<Window>(window_ptr));
}

extern "C" Window* rust_get_window(std::shared_ptr<Window> window)
{
    return window.get();
}

extern "C" void rust_drop_window(void* ptr)
{
    std::shared_ptr<Window> *value = static_cast<std::shared_ptr<Window>*>(ptr);
    delete value;
}

and the Rust code

#[derive(Clone)]
#[repr(C)]
struct SharedPtrWindow(*mut miral::Window);

extern "C" {
  fn get_active_window(value: *mut miral::WindowManagerTools) -> SharedPtrWindow;
  fn rust_get_window(value: SharedPtrWindow) -> *mut miral::Window;
  fn rust_drop_window(value: SharedPtrWindow) -> ();
}

impl SharedPtrWindow {
  fn get(&self) -> &mut miral::Window {
    unsafe { &mut *rust_get_window(self.clone()) }
  }
}

impl Drop for SharedPtrWindow {
  fn drop(&mut self) {
    unsafe { rust_drop_window(self.clone()) }; 
  }
}

I'll ask and see if I can get bindgen to work to a degree where I don't need stuff like that but it's nice to see that it's possible to work around if it isn't.

@AlanGriffiths
Copy link
Collaborator

AlanGriffiths commented Aug 23, 2019

Nice.

For the record, I was thinking of something more like:

std::set<miral::Window> window_cache;
...
extern "C" miral::Window* get_active_window(miral::WindowManagerTools* tools)
{
    auto iter = window_cache.insert(tools->active_window());
    return &*iter.first;
}

void ProxyWindowManager::advise_end()
{
    // Invoke the rust code
    window_cache.clear();
}

@Pajn
Copy link
Contributor Author

Pajn commented Aug 24, 2019

I got that, but it felt better making sure it lived for the lifetime of the usage inside the Rust code. It's more binding code and the usage is a bit more awkward as I need to call .get() inside Rust code but I think that's a worthwhile trade of.

After debugging why WindowSpecification.parent() was so unreliable (a lot of times being empty for tooltips) I realized that weak_ptr is problematic but nothing another small FFI helper in form of window_specification_has_parent can't solve.

I'm growing more confident that this actually is a workable path and questions that come up are no longer related to FFI. Hopefully bindgen can be configured or patched to not generate bindings at all for these problematic cases so that one would have to write manual bindings instead of getting runtime problems.

Thanks for the help and insights so far. I'll no doubt come back with other (more Mir in general) questions as I progress.

@Pajn Pajn closed this as completed Aug 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants