Skip to content

Conversation

@fogInSingularity
Copy link
Collaborator

Implement plugin subsystem:

Plugin subsystem consist of following parts:

Plugin manager(that manages plugins and notify them on events)
Plugin(manages dynamically loaded plugin)
loadPlugin function that returns LoadablePlugin struct with pointers to notify, unload and plugin internal memory.
uml diargram
I need help deciding on the notify function signature. I think it’s worth making a function for each event (due to the fact that the parameters will be different for different events).

@fogInSingularity fogInSingularity self-assigned this Mar 3, 2025
@fogInSingularity fogInSingularity linked an issue Mar 3, 2025 that may be closed by this pull request
@fogInSingularity fogInSingularity force-pushed the add_plugins branch 2 times, most recently from ad60aa5 to d268e3e Compare March 3, 2025 21:00
@derzhavin3016
Copy link
Member

I suggest you to implement PluginAPI as an interface class to avoid C-style type erasure void * and simplify plugin usage. Interface can consist of two virtual function for loading plugin (which will somehow subscribe plugin on some events) and unloading for unsubscribtion.

@fogInSingularity
Copy link
Collaborator Author

I changed the implementation of the plugin subsystem:
image
Now you need to inherit from IPlugin to create your own plugin and supply function IPlugin* loadPlugin(const char* options) that allocates your plugin.

But i this code currently have bug

hsim::SharedLib sharedLib{"./path/to/libconcrete_plugin.so", hsim::kLazy};

auto plugin = hsim::loadPluginFromSO(sharedLib, "option");
machine.addEventConsumer(std::move(plugin));
machine.notify();

~SharedLib called before ~IPlugin(that calls ~ConcretePlugin that is loaded from .so file) so there a use after free
im not sure how to fix it

@fogInSingularity
Copy link
Collaborator Author

I fixed use after free bug
current plugin subsystem:
image

@derzhavin3016
Copy link
Member

In general, design looks nice to me (seems like an Observer pattern). But there is some noticeable point about extensibility: events list could be extended & each event can accept different arguments (types & amount). Also, you need to somehow distinguish events with identical call signatures (e.g. memwrite & memread events arguments would be the same)

@derzhavin3016
Copy link
Member

For example, try to define two events with identical argument list, and two events with different arguments list. Hope this helps to prototype generic solution

@derzhavin3016
Copy link
Member

Example event notification (approximate syntax):

event_model.call<MemWrite>(core, ptr, size);

Copy link
Member

@derzhavin3016 derzhavin3016 left a comment

Choose a reason for hiding this comment

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

Some comments, it's not required to be fixed since they're absolutely minor. It's more like a notice


private:
std::unique_ptr<void, DlCloser> m_handle;
static_assert(sizeof(m_handle) == sizeof(void *));
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK, it is not required by standard

void attach(IEventConsumerHandle consumer) {
m_consumers.push_back(std::move(consumer));
}
void detach(IEventConsumerHandle consumer) { m_consumers.remove(consumer); }
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused a bit with thus function. How could you remove plugin by address, if you've lost it after attach() call?
e.g.:

EventManager ev;
auto consumer = std::make_unique<IntConsumer>(42);
ev.attach(std::move(consumer));
// consumer is in moved-from state hereafter
ev.detach(???); // what to pass here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm confused a bit with thus function. How could you remove plugin by address, if you've lost it after attach() call? e.g.:

EventManager ev;
auto consumer = std::make_unique<IntConsumer>(42);
ev.attach(std::move(consumer));
// consumer is in moved-from state hereafter
ev.detach(???); // what to pass here?

is detach necessary at all?

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this method

@derzhavin3016
Copy link
Member

After personal discussion, it was decided to leave implementation in current state, with optional code deduplication via defines/Jinja codegen.

The last part is to add Unit tests, after which this PR could be merged

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.

Implement plugin subsystem

3 participants