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

libtock/ipc: add custom IPC service callback type with buffer pointer #421

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lschuermann
Copy link
Member

This changes the IPC service callback function type to provide the IPC buffer pointer as an actual pointer type. The fact that this is currently an integer is pretty weird, and appears to be an artifact of the implementation sharing the subscribe system call wrapper.

I'm not sure whether what I'm doing here works, or is legal. In fact, there's a chance that it's relying on undefined behavior. However, conveying pointers as int is also not exactly elegant. A proper fix may be to duplicate the subscribe system call wrapper implementation and declare it with a different type that accepts this upcall function signature.

@ppannuto
Copy link
Member

ppannuto commented May 9, 2024

Shoving integers into pointers is a time-honored tradition in C. From the example I'm most familiar with (GLib), however:

Warning: You may not store pointers in integers. This is not portable in any way, shape or form. These macros only allow storing integers in pointers, and only preserve 32 bits of the integer; values outside the range of a 32-bit integer will be mangled.

If I understand correctly, the material part of the question boils down to:

typedef void (ipc_service_upcall)(int, int, void*, void*);
typedef void (subscribe_upcall)(int, int, int, void*);

subscribe_return_t sval = subscribe(IPC_DRIVER_NUM, svc_id, *((subscribe_upcall*) &callback), ud);
                                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Which is functionally stuffing a void* into an int (though hidden behind some typedefs).

For the current, specific use cases of Tock, I believe our suite of hardware platforms and pointer models is sufficiently limited that this will work. I'm less sure whether all this means we should, or shouldn't, merge this change [which, really isn't changing any functionality; we're already doing the bad thing IIUC].

@bradjc
Copy link
Contributor

bradjc commented May 10, 2024

Do we need this now? Can you make these changes at least consistent with the guide?

@lschuermann
Copy link
Member Author

I don't know whether we need to change it, but if we do, I can make it consistent with the guide.

Copy link
Contributor

@bradjc bradjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Namespacing and fn typdef format.

@lschuermann
Copy link
Member Author

Let's postpone this change until after the tutorial.

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