update_methods segfault if I forget a register_class

[paul-d-rose]

I enjoyed your cppcon video, and I'm having fun playing with your library - thanks!

If I forget a define_method, I can get an error callback with set_method_call_error_handler.

If I forget a register_class, I get a segfault during update_methods.

I understand that it can't just magically work, and I like that it fails early ( during update_methods, which hopefully I'm calling right from the top of main ).

Is there any way to throw, or at least abort with relavent stderr text in this case ( without significant additional expense )?

I can provide a sample if necessary, but just removing a register_class from an example should do it.

[jll63]

I enjoyed your cppcon video, and I'm having fun playing with your library - thanks!

Glad to hear that!

Is there any way to throw, or at least abort with relavent stderr text in this case ( without significant additional expense )?

In C++ there is no good, general solution to this, because run time reflection does not make it possible to enumerate all the type_infos in the program (like what's done in the Dlang version).

I can add checks for null in places like this when building in debug mode, but that is only a partial solution.

I don't know if you have looked at the implementation, or thought about the implementation notes...Yomm2 builds a perfect, collision free hash function that maps the address of the type_info for all known classes (as per register_class) to a pointer to a method table (i.e. yomm2's equivalent of the vptr). Entering an unknown type_info* in that function will result in a crash if you are lucky, i.e. if the hash returns an index corresponding to an unused entry (the hash function is not minimal, so there are probably a few unused entries). However, if the runtime type of a virtual argument has not been registered, its type_info hash may yield the same index as another, registered class, because the hash function is perfect only for the values it is aware of.

Hmmm, this is quite annoying in fact. Maybe in debug mode, I should keep track of the known classes in an unordered_map and check the typeids of all the virtual arguments against it. It would be slow, but if it's debug mode...Or I could add an additional paranoid mode.

[paul-d-rose]

Thank you for the detailed reply.

I had not not looked into the implementation notes, but I will.

Certainly a debug build only solution to this issue is enough.

Even a solution that only solved the case where the crash happens during update_methods() would be nice. That is the case that has caught me twice so far (I defined all of the methods, but forgot to register the one of the classes).

The library is useful without any extra checking. I just need to have good coverage directed tests. It is merely inconvenient that it is hard to find the class that I forgot to register.

perfect, collision free hash function

Could you build (for debug only) a vector of type-id that was indexed by the output of your hash function? It would have only the expected result for that index. Then have an debug only check that you found what you were looking for. This check would never fail in a well formed program (because the hash is perfect), and could (and probably should) be suppressed for a release build.

[jll63]

Could you build (for debug only) a vector of type-id that was indexed by the output of your hash function? It would have only the expected result for that index.

I just implemented something similar.

Now in debug mode, you should get a better error message if:

• a class used as a virtual parameter in a method declaration has not been registered (during update_methods)
• a class used as a parameter in a method specialization corresponding to a virtual parameter in the method declaration has not been registered (during update_methods)
• the dynamic type of an argument has not been registered (during method call)

[paul-d-rose]

I tried it out, and it worked very well. I commented out a single register_class and got an error message naming the unregistered class. Thank you.

You may already know this, but just in case you don't:

Now I can get a crash if the NDEBUG flag for the yomm2 library build does not match the NDEBUG flag for my project. Even in an otherwise well formed program.

It is not a problem for me, but I wanted to make sure that it wasn't an unintended consequence for you.

[jll63]

Now I can get a crash if the NDEBUG flag for the yomm2 library build does not match the NDEBUG flag for my project. Even in an otherwise well formed program.

That's because hash table entries are now 2 words (type_info* and mptr) in debug mode.

It is not a problem for me, but I wanted to make sure that it wasn't an unintended consequence for you.

Actually it's the other way around: I never realized that it was possible to mix debug and non debug. I did not expect this to work because some data structures have extra members in the debug version. Now that you attracted my attention on this, this looks like a nice feature to have. I will try to modify the library to support this.

[paul-d-rose]

Having extra validation code called by assert() is OK, and #ifdef-ing out checking code that supports assert() calls is OK. But NDEBUG changes that break separate compilation (i.e. break ABI) is usually considered bad.

Having hash_entry_size be defined in your cpp and "extern" for your headers might work, but the generated code would not be quite as good because it would not be a compile time constant for the inlined code in the user's translation units. Maybe it wouldn't be noticeable.

Header only libraries are another way people avoid this problem ( because there is no ABI ).

[jll63]

This should take care of it: https://github.com/jll63/yomm2/tree/better-registered-class-check

[paul-d-rose]

Thanks for the update.

I checked out the new better-registered-class-check branch and did a clean build of both it and my project.

With a well formed program ( all classes registered and all methods defined) everything continues to work when both your library and my program have matching NDEBUG.

If I use NDEBUG for my project and not for your library, I get a segfault during update_methods.

If I do the opposite, NDEBUG for your library and not for my project, I get link errors such as "undefined reference to `yorel::yomm2::detail::log"

I'm using g++ 7.3 on ubuntu linux 18.08, if it matters.

The procedure I use to reproduce:

Case 1 - yomm2 debug with example release 1) follow directions to install yomm2:

mkdir build cd build cmake .. make

sudo make install

2) make example accept_no_visitor "by hand" with NDEBUG

cd examples g++ -DNDEBUG -std=c++17 -o accept_no_visitors accept_no_visitors.cpp -lyomm2

3) run

./accept_no_visitors

4) segfault

Case 2 - yomm2 release example debug 1) make and install yomm2 release

mkdir build cd build cmake -DCMAKE_BUILD_TYPE=Release .. make sudo make install

2) make example by hand without NDEBUG

cd examples g++ -std=c++17 -o accept_no_visitors accept_no_visitors.cpp -lyomm2

3) link errors

Normally one wouldn't build the your supplied example by hand, but it represents a likely way somebody might try out the library with their own first program

On Mon, May 20, 2019 at 8:22 PM Jean-Louis Leroy notifications@github.com wrote:

This should take care of it: https://github.com/jll63/yomm2/tree/better-registered-class-check

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jll63/yomm2/issues/4?email_source=notifications&email_token=AJPRRUBY53R7ME56M45FDSLPWNFD5A5CNFSM4HMI55SKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV2PJUA#issuecomment-494204112, or mute the thread https://github.com/notifications/unsubscribe-auth/AJPRRUDOCAZM7NAZYVU5AYLPWNFD5ANCNFSM4HMI55SA .

[jll63]

I think this should do it. I added support for testing mixed builds. Can you double-check?

[paul-d-rose]

Thanks! I’ll try it tonight

On Mon, Jun 3, 2019 at 7:57 PM Jean-Louis Leroy notifications@github.com wrote:

I think this should do it. I added support for testing mixed build. Can you double-check?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jll63/yomm2/issues/4?email_source=notifications&email_token=AJPRRUH25E6N452WB4QUUSTPYW4V3A5CNFSM4HMI55SKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW3C4FQ#issuecomment-498478614, or mute the thread https://github.com/notifications/unsubscribe-auth/AJPRRUBRP4NJBOA5X2HF56LPYW4V3ANCNFSM4HMI55SA .

[paul-d-rose]

Looks good.

All four combinations of debug/release for app/library work properly with a well formed program.

debug/debug gives very nice error output

debug lib/release app still gives quite useful error output.

and release/release behaves as expected

Thank you.

[jll63]

Great!