Dynamic Loader (dyld) by georgemorgan · Pull Request #137 · flipper-io/flipper

georgemorgan · 2018-02-18T05:04:42Z

Not ready to merge.

Implement a dynamic loader across libflipper and the device. Goal: Resolve symbols from higher level languages to make it easier to write bindings.

Changes

This PR basically removes the need to worry about a struct _lf_module * in user-land. Although this data type still exists, it is internal to libflipper and shouldn't be touched from the outside.

The signatures of `lf_invoke`, `lf_push`, `lf_pull` are now as follows:

int lf_invoke(struct _lf_device *device, char *module, lf_function function, lf_type ret, lf_return_t *retval, struct _lf_ll *args);
int lf_push(struct _lf_device *device, void *dst, void *src, size_t len);
int lf_pull(struct _lf_device *device, void *dst, void *src, size_t len);

All of these functions accept a struct _lf_device * as the first parameter.
Instead of accepting a struct _lf_module *, they now accept a char * for the module parameter. This is the name of the module that is to be invoked, pushed, or pulled on.

`lf_bind` has gone bye-bye

There is no need for lf_bind anymore! When an invoke is performed, libflipper will automatically do what lf_bind did behind the scenes. The user doesn't have to worry about it anymore.

So to change the language bindings:

Remove all concept of struct _lf_module *.
- The bindings should no longer have a concept of a user module or a standard module. To them a module is now just a string literal.
Remove lf_bind.
- There is no need to bind to modules anymore.

nicholastmosher · 2018-03-13T22:51:47Z

console/src/bin/modules_cli.rs

-        let flipper = flipper::Flipper::attach();
-        let led = flipper::fsm::led::Led::new(&flipper);
-        led.rgb(red, green, blue);
+        let flipper = flipper::Flipper::attach_hostname("localhost");


Please make sure you change this back to regular attach, and let's both try to be more mindful of this, it's a mistake I find myself making a lot too :)

nicholastmosher · 2018-03-13T22:57:24Z

languages/rust/src/lf.rs

-        }
-        let ret = libflipper::lf_invoke(flipper.device, module.as_ptr(), index, T::lf_type(), arglist);
+        args.map(|args|
+            for arg in args.iter() {


"Map" functions are generally not intended to have side effects, you can get rid of that outer loop and just use:

for arg in args.iter() { // ... }

nicholastmosher · 2018-03-13T22:58:58Z

languages/rust/src/lf.rs

+/// let output: u8 = invoke(&flipper, &module, 0, args);
 /// ```
-pub fn lf_invoke<T: LfReturnable>(flipper: &Flipper, module: &ModuleFFI, index: u8, args: Args) -> T {
+pub fn invoke<T: LfReturnable>(module: &str, index: u8, args: Option<Args>) -> T {


With this API, the rust bindings have no means of specifying which Flipper device they would like to use, only the "default" one specified by libflipper will ever be used. Before we wrap this up I would like to add in the capability of creating module instances which are paired to a specific Flipper device.

nicholastmosher · 2018-03-13T22:59:43Z

languages/rust/src/lf.rs

-        }
-        let ret = libflipper::lf_push(flipper.device, module.as_ptr(), index, data.as_ptr() as *const c_void, data.len() as u32, arglist);
+        args.map(|args|
+            for arg in args.iter() {


Same comment as above, should remove the outer "args.map" block and just use

for args in args.iter() { // ... }

nicholastmosher · 2018-03-13T22:59:49Z

languages/rust/src/lf.rs

-        }
-        let ret = libflipper::lf_pull(flipper.device, module.as_ptr(), index, buffer.as_mut_ptr() as *mut c_void, buffer.len() as u32, arglist);
+        args.map(|args|
+            for arg in args.iter() {


Same comment as above, should remove the outer "args.map" block and just use

for args in args.iter() { // ... }

nicholastmosher · 2018-03-13T23:00:13Z

languages/rust/src/lib.rs

 #![deny(unused_import_braces)]
 #![deny(unused_qualifications)]
-#![deny(warnings)]
+//#![deny(warnings)]


Please make sure you put this back when you're done :)

nicholastmosher · 2018-03-13T23:06:45Z

languages/rust/src/lib.rs

    /// is used when communicating with libflipper to specify which
    /// device functions should be executed on.
-    device: _lf_device,
+    device: _lf_device


I tend to like keeping the trailing commas on multi line lists like structs. If you remember to set this back that'd be cool.

nicholastmosher · 2018-03-13T23:18:57Z

languages/rust/src/fsm/led.rs

-    ModuleFFI,
-    _lf_module,
-};
+pub struct Led;


I think we need to find some way to provide both an instance-level API and a module-level (similar to Java static) API such that the module level Led::rgb uses the default device and the instance-level led.rgb will use a specific device.

nicholastmosher · 2018-03-13T23:29:29Z

languages/rust/src/lf.rs

-use ::{ModuleFFI, _lf_module};
+use std::ffi::CString;
+
+pub type _lf_device = *const c_void;


I'm wondering whether the _lf_device type should just be c_void so that in the lf_invoke signature it's clearer that it's a pointer by having lf_invoke(device: *const _lf_device, ...)

That is a good point. Should I change that?

nicholastmosher · 2018-03-13T23:40:49Z

languages/rust/src/lf.rs

    fn from(ret: LfReturn) -> Self { ret.0 as u64 }
 }

+pub fn current_device() -> _lf_device {


I don't think we should have a public function which returns a raw pointer. This should either be pub(crate) or we should find a way to do without this entirely.

nicholastmosher · 2018-03-13T23:47:35Z

languages/rust/src/lib.rs


 impl Flipper {
-    pub fn attach() -> Self {
+    pub fn attach() -> _lf_device {


The _lf_device type shouldn't appear in the public API at all. If it needs to exist, it should be stored as a private field in a struct.

harlanhaskins · 2018-04-23T15:59:28Z

runtime/include/flipper/types.h

 #define LF_WEAK __attribute__((weak))

+#ifdef __clang__
+#define LF_FUNC(MODULE) __attribute__((section("__TEXT,.lm."MODULE), used))


Can you add a space between "__TEXT,.lm." and MODULE? When this header is imported in C++, this breaks, because MODULE is interpreted as a literal suffix

Fixed in 9757f9b

Necessary for compatibility with gcc.

georgemorgan · 2018-11-03T22:07:56Z

ASF time! Soon!

Fixes compatibility issues with dyld on Ubuntu 18.04.

georgemorgan force-pushed the dyld branch from 8d3ceb8 to 605f0dc Compare February 21, 2018 05:45

nicholastmosher force-pushed the dyld branch from 605f0dc to 8866e0d Compare March 8, 2018 16:09

nicholastmosher mentioned this pull request Mar 8, 2018

Javascript #139

Open

5 tasks

georgemorgan force-pushed the dyld branch 2 times, most recently from e2a560c to c07e4fa Compare March 11, 2018 01:42

nicholastmosher force-pushed the dyld branch 3 times, most recently from 937a8ae to 27b0f69 Compare March 13, 2018 15:22

nicholastmosher reviewed Mar 13, 2018

View reviewed changes

georgemorgan force-pushed the dyld branch from 386f9fd to f32d926 Compare March 21, 2018 22:20

nicholastmosher changed the title ~~Dyld~~ Dynamic Loader (dyld) Apr 20, 2018

harlanhaskins reviewed Apr 23, 2018

View reviewed changes

georgemorgan force-pushed the dyld branch from 813655d to bc5228b Compare May 24, 2018 08:22

georgemorgan mentioned this pull request May 24, 2018

Comb through source with -Wpedantic #152

Open

nicholastmosher force-pushed the dyld branch from bc5228b to c793145 Compare June 10, 2018 19:10

georgemorgan force-pushed the dyld branch 2 times, most recently from bc5228b to 861a0cf Compare June 10, 2018 20:24

georgemorgan and others added 10 commits August 16, 2018 10:37

Transition lf_* funcs to accept a device pointer.

2f5240f

Fix Python bindings

d25ea63

Pass explicit device pointer to lf_invoke in rust bindings

b79390d

Update makefile, clean up files

9027646

Clean up makefile

402bf01

pip2 -> pip

995a119

Move modules to api folder.

dd694de

Add new LF_FUNC macro to builtins

d7eff0a

Generate the standard modules!

b1f70d1

Generate top level carbon.c temporarily

301c6f4

georgemorgan and others added 22 commits September 9, 2018 18:09

fix pin namespace collision

aa6a7e2

fix atmegau2 ram overflow

9a4477e

fix undefined behavior + pack structs again + no access to fmr_call

ea22590

fix cross platform error logic w/ atmegau2

e91f0bb

restore the old bulk

1e3d688

Clean up lf_dyld

d6c8d41

Convert tabs to spaces

b0685c5

Convert null statement while loops to empty block while loops

98f664a

Simplify strings in error assertions

2f3df26

Run clang-format over whole codebase

3d2fd94

address signedness and pointer comparison warnings

cdb41f2

Update Swift bindings to work with dyld_wip + rebase

39993ae

try to fix bulk

06a64a6

update swift bindings, fix bulk on atmega

9da9410

move swift to library

1f89b74

fix atsam4s build

3cb1181

fix pointer size issue

20b32f1

restore fdfu

d2bb729

move carbon IO defs

96c0903

Move rust API and examples to library/rust

0fa8da2

Add Rust macro for generating invocation implementations

6dcd399

Fix lf_invoke wrapper for Rust

3cfacec

nicholastmosher force-pushed the dyld branch from bd71c2d to cfce0bd Compare September 23, 2018 20:38

Split Rust bindings into core and api

36e0be8

nicholastmosher force-pushed the dyld branch from cfce0bd to 36e0be8 Compare September 23, 2018 20:46

wasv added 2 commits October 27, 2018 11:35

Adds py3 packages and pkg-config to dependencies

4d3d8fd

Moves LD_FLAGS to end of linking command.

559f6bd

Necessary for compatibility with gcc.

wasv and others added 2 commits November 10, 2018 15:07

Only link asan if debug flag is defined

b317d8b

Merge pull request #156 from wastevensv/wasv_dyld

8efbcda

Fixes compatibility issues with dyld on Ubuntu 18.04.

Comments

Conversation

georgemorgan commented Feb 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

The signatures of lf_invoke, lf_push, lf_pull are now as follows:

lf_bind has gone bye-bye

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgemorgan Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgemorgan commented Nov 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

georgemorgan commented Feb 18, 2018 •

edited

Loading

The signatures of `lf_invoke`, `lf_push`, `lf_pull` are now as follows:

`lf_bind` has gone bye-bye

georgemorgan Mar 14, 2018 •

edited

Loading