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

Added ProcPtr to IvoryStore, enabling 'store' to work on ProcPtr. #23

Closed
wants to merge 3 commits into from
Closed

Conversation

Hodapp87
Copy link
Contributor

@Hodapp87 Hodapp87 commented Jan 5, 2015

This commit message is Literate Haskell to explain the commit's purpose.

{-# LANGUAGE TypeOperators #-}
{-# LANGUAGE DataKinds #-}
import Ivory.Language
import Ivory.Compile.C.CmdlineFrontend
m = package "StoreProcPtr" $ do
incl setFnExample
defMemArea fnExtn
main = runCompiler [m] [] initialOpts

The specific use case that brought this to my attention was the need to
interface Ivory with some APIs that relied on callbacks. Due to some other
limitations, I had to write by hand C wrappers for these callbacks.

However, I wanted Ivory to have the ability to still set this callback at
runtime somehow, and so I had the C wrapper make a call to a function pointer,
and I tried to set this function pointer's value from within Ivory.

Suppose that 'fnExtn' below is that function pointer.

type Fn = '[Uint16] :-> ()
fnExtn :: MemArea (Stored (ProcPtr Fn))
fnExtn = area "fnExtn" Nothing

I have an Ivory procedure 'fnExample' that I would like to assign to 'fnExtn'
sometime at runtime:

fnExample :: Def Fn
fnExample = proc "fnExample" $ \ _ -> body $ retVoid

And so I also have a function 'setFn' which, given a procedure name and
another Ivory procedure, generates an Ivory procedure that sets that global
function pointer 'fnExtn' to that first procedure.

setFn :: String -> Def Fn -> Def('[] :-> ())
setFn name fn = proc name $ body $
store (addrOf fnExtn) (procPtr fn)

...which I've instantiated thus with fnExample as 'setFnExample':

setFnExample = setFn "setFnExample" fnExample

But there is a problem. 'setFn' relies on 'store' (Ivory.Language.Ref) which
has the signature: IvoryStore a => Ref s (Stored a) -> a -> Ivory eff ()

Predictably, 'store' does not work on anything that is not in IvoryStore,
"Things that can be safely stored in references." ProcPtr, despite being
something that (as far as I understand it) can safely be stored in a reference,
is not in IvoryStore. (While IvoryArea a => IvoryStore (Ptr Global a), I am
unaware of this working with Def or ProcPtr and the minimal compliant type
I could construct was Ptr Global (Stored (ProcPtr proc)) which is not a
correct type for what I am doing.)

This commit simply adds ProcPtr to IvoryStore, and enables the above code to
run.

This commit message is Literate Haskell to explain the commit's purpose.

> {-# LANGUAGE TypeOperators #-}
> {-# LANGUAGE DataKinds #-}
> import Ivory.Language
> import Ivory.Compile.C.CmdlineFrontend
> m = package "StoreProcPtr" $ do
>     incl setFnExample
>     defMemArea fnExtn
> main = runCompiler [m] [] initialOpts

The specific use case that brought this to my attention was the need to
interface Ivory with some APIs that relied on callbacks.  Due to some other
limitations, I had to write by hand C wrappers for these callbacks.

However, I wanted Ivory to have the ability to still set this callback at
runtime somehow, and so I had the C wrapper make a call to a function pointer,
and I tried to set this function pointer's value from within Ivory.

Suppose that 'fnExtn' below is that function pointer.

> type Fn = '[Uint16] :-> ()
> fnExtn :: MemArea (Stored (ProcPtr Fn))
> fnExtn = area "fnExtn" Nothing

I have an Ivory procedure 'fnExample' that I would like to assign to 'fnExtn'
sometime at runtime:

> fnExample :: Def Fn
> fnExample = proc "fnExample" $ \ _ -> body $ retVoid

And so I also have a function 'setFn' which, given a procedure name and
another Ivory procedure, generates an Ivory procedure that sets that global
function pointer 'fnExtn' to that first procedure.

> setFn :: String -> Def Fn -> Def('[] :-> ())
> setFn name fn = proc name $ body $
>                      store (addrOf fnExtn) (procPtr fn)

...which I've instantiated thus with fnExample as 'setFnExample':

> setFnExample = setFn "setFnExample" fnExample

But there is a problem.  'setFn' relies on 'store' (Ivory.Language.Ref) which
has the signature:  IvoryStore a => Ref s (Stored a) -> a -> Ivory eff ()

Predictably, 'store' does not work on anything that is not in IvoryStore,
"Things that can be safely stored in references."  ProcPtr, despite being
something that (as far as I understand it) can safely be stored in a reference,
is not in IvoryStore.  (While IvoryArea a => IvoryStore (Ptr Global a), I am
unaware of this working with Def or ProcPtr and the minimal compliant type
I could construct was Ptr Global (Stored (ProcPtr proc)) which is not a
correct type for what I am doing.)

This commit simply adds ProcPtr to IvoryStore, and enables the above code to
run.
@jameysharp
Copy link
Contributor

Awesome—I wanted this a few weeks ago but didn't take the time to figure out how to fix it. I'll ask Lee to review.

@Hodapp87
Copy link
Contributor Author

Hodapp87 commented Jan 5, 2015

Is there any point in extending this also to Def, or is it good enough to just have ProcPtr?

@leepike
Copy link
Contributor

leepike commented Jan 5, 2015

I asked @Hodapp87 to make the pull request as we were talking about Ivory over email and he suggested the change. But now I wonder if this is actually safe. A global memory area containing a value is safe to dereference because it is guaranteed to exist and be initialized to 0. But for function pointers, this isn't sufficient, since it can be used before being pointed to a valid function, resulting in a seg fault or similar runtime error.

@Hodapp87
Copy link
Contributor Author

Hodapp87 commented Jan 5, 2015

This is a good point that I had not thought of. I'll have to think some about what ways exist to remedy this.

At the same time, ProcPtr has Ptr in the name (as well as "pointer" in the documentation), and Ptr elsewhere in Ivory explicitly is nullable (vs. Ref which is not). I was looking at it as more akin to IvoryArea a => IvoryStore (Ptr Global a) - one still admits nullable pointers there inside of IvoryStore.

Admitting a nullable ProcPtr, though, means indirect and indirect_ need to change (perhaps to be more like withRef). If ProcPtr explicitly cannot be nullable, then perhaps ProcRef is a more proper name.

@leepike
Copy link
Contributor

leepike commented Jan 6, 2015

If ProcRef is implementable (I don't see why not), then that is likely preferable to ProcPtr anyway. @jameysharp, @pchickey, @elliottt: have you needed ProcPtr?

@leepike
Copy link
Contributor

leepike commented Jan 8, 2015

@Hodapp87, again, thanks for bring this long-standing Ivory bug to our attention: your use-case points out that we could always store a ProcPtr into a MemArea and use it without initializing it.

Here are a few commits that should both fix the bug and provide you with the functionality you require.

  • Commit bf56763 fixes the bug by placing an IvoryZero class constraint on storing into a MemArea. This means things stored into a MemArea must have an initializer. (However, currently, we are not actually using the default initializer, so that globals go into the .bss section. Thus, IvoryZero instances must be compatible with C semantics---i.e., initialized to 0.)
  • Commit 89129ad generalizes the store smart constructor to work on both Refs and Ptrs, since both are safe to store into.

With that, I think we have enough machinery to support your use-case. Consider the examples in Commit 5df8459. Instead of creating a memory area containing a function pointer directly, we create one containing a pointer to a function pointer (procArea). Because Ptr may be null, we can't use it directly from Ivory, which is good.

However, we can copy the function pointer into a local function reference and use it in Ivory (see function foo()). We can also store a new function pointer into the memory area (also see foo()).

Also, since the memory area is global, so you can use it, albeit unsafely, from raw C, as is your use case.

Finally, the following are not possible, which is good:

procArea2 :: MemArea (Stored (Ref Global (Stored (ProcPtr Fn'))))
procArea2 = area "procArea2" Nothing
procArea3 :: MemArea (Stored (ProcPtr Fn'))
procArea3 = area "procArea3" Nothing

They cannot be constructed due to a lack of IvoryZeroVal instances.

I believe the commits in Branch proc-ref fix the bug and give you the functionality you need without requiring any significant change to the language. I should get a code-review from the rest of the team, however, before merging into master.

@Hodapp87
Copy link
Contributor Author

Hodapp87 commented Jan 9, 2015

@leepike, thanks for pointing me to some examples. I will take a look at the branches and commits you noted. If I understand you right, then the proc-ref branch should have all I need without my commit, and I can close this pull request.

@leepike
Copy link
Contributor

leepike commented Jan 9, 2015

If I understand you right, then the proc-ref branch should have all I need without my commit, and I can close this pull request.

Yes, and then I'll merge into master, so long as nobody finds additional bugs. :)

@leepike
Copy link
Contributor

leepike commented Jan 9, 2015

Sorry, I spoke too soon. This doesn't do what you want, since it doesn't initialize the global pointer. Let me rework this. (I just checked safety, that you can't use the null pointer from Ivory.)

@leepike
Copy link
Contributor

leepike commented Jan 9, 2015

@Hodapp87, Ok, sorry about that. Checkout branch procptr and look at the examples in FunPtr.hs (procArea and function foo) and see if that works for you.

@Hodapp87
Copy link
Contributor Author

I've ignored this, so I'm just going to close it since it's probably irrelevant at this point.

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

Successfully merging this pull request may close these issues.

3 participants