cr_main isn't invoked with operation CR_LOAD and the context passed to cr_plugin_load

[skypjack]

I'm not sure if this is the intended behavior and I haven't found anything in the doc nor the old issues. I'm using this library on a debian sid, clang 9 for development, the resulting code is meant to be deployed and tested on all the supported platforms.

This is an hypothetical header shared by the main executable and the plugin:

struct foo { int value{0}; };
struct bar { int value{0}; };

Consider the following snippet for main.cpp:

foo load{};
bar update{};

cr_plugin ctx;

ctx.userdata = &load;
cr_plugin_load(ctx, PLUGIN);

ctx.userdata = &update;
cr_plugin_update(ctx);

This is instead the code for the plugin:

CR_EXPORT int cr_main(cr_plugin *ctx, cr_op operation) {
    switch (operation) {
    case CR_LOAD:
        static_cast<foo *>(ctx->userdata)->value = 42;
        break;
    case CR_STEP:
        static_cast<baro *>(ctx->userdata)->value = 42;
        break;
    case CR_UNLOAD:
    case CR_CLOSE:
        // nothing to do here, this is only a test.
        break;
    }

    return 0;
}

I thought that it was possible to pass a given context during the load and a different one during the update. However, it seems that the call to cr_main is delayed until I invoke cr_plugin_update. At this point, cr_main is invoked two times with the same context (the one containing a pointer to update) and with different operation (CR_LOAD, then CR_STEP).

It's not a big problem. I introduced a wrapper that has two data members for the given types and made it work. However, this is unexpected and somehow counterintuitive. Is this the case and is there any way I can pass a different pointer through userdata to the different phases?

[skypjack]

I looked at your code.

Actually, it seems that the first call to the plugin takes place when I invoke cr_plugin_update. In this case, cr_plugin_reload is invoked with the second value I attached to the context because reloadCheck is defaulted to true. This confirms that I cannot really pass a different value by means of userdata to cr_plugin_load and the one I attached is just ignored.

if I can give you an advice, I would make this thing explicit in the documentation because, at a first reading, I definitely missed it and thought I could use different values for userdata in the different phases. I apologize for the noise. I close the issue myself having already given an answer. :+1:

[fungos]

Yes, you're right it is confusing. Basically CR_LOAD has nothing to do with cr_plugin_load (which is basically initializer, which was originally named cr_plugin_init). It is a bit confusing and I'll try to improve the documentation soon, thanks for reporting anyway. This issue may still help others.

[skypjack]

The fact that both rely on the word load is a bit misleading, I agree. Fortunately your code is well written and pretty simple to follow and understand, so not a big problem at the end of the day. :+1:

May I ask you another thing? If I got it right, every time I invoke update, it checks if there exists a more updated version of the plugin and eventually the latter is reloaded (unless I pass false to force reload ofc), am I wrong?

[fungos]

Yes, that is exact. That flag was introduced so you can control the frequency of the check and reduce performance impact. It was introduced here #13 by @datgame :)

[skypjack]

I see, good catch actually. Put aside the dev loop when I iterate continuously, it makes sense to inhibit the test to reload a module in production most of the times.

Thank you for the quick responses. Really appreciated. :+1:

[skypjack]

Oh, I've hit another confusing point due to this line:

r = cr_plugin_main(ctx, close ? CR_CLOSE : CR_UNLOAD);

Long story short, when I close a plugin I never see CR_UNLOAD.

My two cents: CR_UNLOAD is a very misleading name if I consider for what it's used. When I close a library (whatever it means), the library itself is necessarily unloaded. In this case instead, closing and unloading are very different operations and the latter takes part to the game only during a reload. This isn't clear from the documentation and the intuition I've from the computer science tells me quite the opposite.

If I got it, with cr I can have only this chains:

• load -> close
• load -> (unload -> load)* -> close

Where (unload -> load) means that the plugin has been reloaded. Right? I think it's pretty clear if put in this terms.

[fungos]

Yes, that is also correct! I can see the confusion, thanks a lot for bringing it up and really sorry about this oversight causing you any trouble.

I think my initial rationale was that in the context of hot-reloading, loading/unloading were specific to the hot-reloading part as these are specific steps required for data lifetime cross-instance. The open and closing of plugin does not incur in load and unload, but unfortunately I did not name the function cr_plugin_open (the one cr_plugin_load).

Also in the case of a cr_plugin_open, a new cr_op::CR_OPEN would be required for consistency (avoiding your first issue).

I think it is a bit late to rename these (if we can even find a better naming), but I'll also put that into the documentation (once I get back from vacancies next year).

I may consider further on renaming and deprecation steps.

[skypjack]

Don't worry. You've nothing to apologize for!! I see it's late for changing the names of these steps and I agree on this. Honestly, I think it would be enough to make it clear in the documentation. The hot-reload context is just one of the use cases for your library after all and even in this case I could expect a CR_UNLOAD on close because... well, I'm also unloading the plugin. :)

Enjoy your vacancies and sorry for the noise. :+1: Thank you for your response, really appreciated.

[fungos]

Note that you can use CR_CLOSE as CR_UNLOAD in your situation, in the docs (at least?) we have this that hint at the difference:

• CR_CLOSE Like CR_UNLOAD but no CR_LOAD should be expected;

[skypjack]

Yeah, sure, I'm using it actually. It took me just a few minutes to realize that CR_UNLOAD doesn't arrive in this case. As I said, the code of this library is easy to follow and understand. :+1:

[fungos]

Hello, in the commit above I did some rewording in the doc and added a cr_plugin_open function to replace the old load. Take a look there please.If you have any suggestion to still improve the wording, I can change it more before merging.

[skypjack]

Oh, wow, thank you for taking in consideration my comments. It looks good, open sounds definitely the right term and the doc now is clear about load/reload.

Just one question for curiosity: why did cr_set_temporary_path become a non-static function?

[fungos]

Yeah, that one is a bit out of place right now. It was triggering a warning as it is unused and is exposed in the doc as a public API. On the other hand, it is not extern neither a valid C function. So I just removed the static to silence the compiler, no other good reason. Not sure what to do with that one yet :)

[skypjack]

I see, yeah, I've those warning as well actually. Another one comes from a shadowing variable and I thought more than once to open a PR to suppress it too. :smile:

[fungos]

Which one? :) Feel free to open PR for anything that helps improve the quality, even typos in doc are worth it.

[skypjack]

Sure, thanks for inviting. It's a pleasure to work on your code actually, it's well written. :+1: The warning comes from this line:

auto ehdr = (Elf64_Ehdr *)p; // shadow

I've never opened an issue because I thought it was on purpose since you put there that comment. The fact is that this triggers a warning when you instruct the compiler to detect shadow variables.

[fungos]

Oh I see, that was self-inflicted as a protection against typo due the way the docs recommend using the ehdr. I'll try to clean this up better. (I should set a proper compiler compatibility tests on CI at some time.)

Fixed.

[fungos]

Thank you for all your help in here, if there is anything more don't worry reopening this or creating new issues.