search

wxWidgets / wxWidgets

Cross-Platform C++ GUI Library
https://www.wxwidgets.org/
5.77k stars 1.7k forks source link

Reworked RTTI in typeinfo.h and fixed potential problems with dll boundaries #24508

Closed MapleLeaf-X closed 1 week ago

MapleLeaf-X commented 2 weeks ago

Previously I've overlooked RTTI's safety across dll boundaries so this pr resolve such potential problem. a few things to note, the code has been reworked and a few internal macros have been removed as they're no longer needed.

PS: I wasn't entirely sure about the naming of wxPrivateTypeId please let me know if that name is suitable or not and I will change it if isn't.

MapleLeaf-X commented 2 weeks ago

Thinking more about this, I'm really not sure if this works correctly when using shared libraries. I don't believe we have tests exercising this case, i.e. having a class defined in a shared library and using its type id in the application, have you tested this, ideally under both Linux and macOS (because ELF and Mac are different in this aspect)?

It would be definitely nice if we could things as simply as this, of course, but this seems suspicious, it's not for nothing that libstdc++ implements C++ standard RTTI by comparing names as strings and not just pointers.

There are indeed no tests for this but at least from my tests on windows it does work correctly and I'm assuming that if exporting via declspec(dllexport) works on windows then it should at least in theory work on other platforms as well. unfortunately I can't put it to a real test on linux or osx, however it shouldn't be worse than how it previously worked. Additionally I'd like to note that ideally RTTI availability could be ignored specifically for typeinfo.h to allow mixed build configurations with RTTI enabled or disabled and that will fix #16431.

MapleLeaf-X commented 2 weeks ago

It appears that under certain conditions it can still fail so I'll be looking into this for now until I'll have it resolved.

vadz commented 2 weeks ago

Please note that both ELF and macOS are very different from Windows in this aspect, so it would be really great to have a test for this case added to our test suite so that it could be run by the CI on all platforms. I realize that it's tricky to add it due to build system complications, but it still would be very nice to have...

MapleLeaf-X commented 2 weeks ago

I did a bit of manual testing with GCC on linux with -O3 and it did not break so it seem to work as expected. I'll try and look into adding more tests and we'll see how that goes but I can't promise anything.

Edit: there is still one problem with this RTTI which unfortunately cannot be resolved due to the nature of the language itself and it plagued the previous implementation as well. two separate but identical instantiations of a template will fail to compare if they're from different binaries, most of the time it shouldn't be a problem but it should be noted. Ideally RTTI should be thrown away in favor of a better solution but currently I don't have a better idea/solution for this.

vadz commented 2 weeks ago

I don't think it's -Ox really changes anything (but I could be wrong), it's having the same symbol in multiple places which does. E.g. consider the application using libfoo where it is defined and also using libbar using libfoo. I'm afraid we could have 2 different symbols, with different addresses, in this case.

MapleLeaf-X commented 2 weeks ago

Optimizations absolutely can affect symbols as in the case of MSVS where /Gy and /OPT:ICF will merge constants but that is technically non conforming behavior (merging functions falls under as-if so as long as semantics aren't broken it will be considered conforming). as for multiple symbols I do believe that it should work as intended at least for wxWidgets since the code is mostly identical to what it originally used to be minus the requirement of default constructors.

Edit: come to think of it, assuming libbar doesn't have libfoo embedded via the likes of /wholearchive and an application links against libfoo in such case there should still be only one symbol defined within libfoo however if libfoo were to be somehow linked more than once within an application (statically) then yes custom RTTI will more than likely fail and it is the problem I've mentioned in an edit in the previous comment.

vadz commented 1 week ago

Thanks, comparing this with the pre-a0d6d1816dd87357a33df7b9dc4fbf04722f3e1a version I don't think this can introduce any regressions, so I'm going to merge it.