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

Fix issues with Windows and the Interconnect library #51

Closed
mosra opened this issue Sep 16, 2018 · 7 comments

Comments

2 participants
@mosra
Copy link
Owner

commented Sep 16, 2018

Currently, in case two signals have the same signature (such as the StateMachine::stepped() templated signal or Ui::UserInterface::inputWidgetFocused() and inputWidgetBlurred() pair of signals, MSVC merges them in Release mode when the /OPT:ICF flag is enabled as their codegen is the same. That's very unfortunate and leads to nasty hard-to-prevent bugs.

Presently, there are the following options, none of which is ideal:

  1. Manually make each signal generate a different codegen by writing a different value to an internal member. That's how it's done now, is eww and doable only in library code -- we can't force the users to do the same with their emitter implementations.
  2. Globally enable /OPT:NOICF. That'll prevent some useful optimizations elsewhere and again needs to be done by the users as CMake < 3.13 doesn't support INTERFACE_LINK_OPTIONS. Even with CMake 3.13, users with custom buildsystems will have issues because this flag won't get enabled for them automatically. (Note: it might be useful to say /OPT:REF,NOICF instead (source).)
  3. Work around this issue by changing the emit() API. One idea I have is to replace it with a CORRADE_EMIT() macro that has __COUNTER__ inside, which then will get saved to an internal member of the Emitter class. This won't solve it for templated signals, though -- the user would again need to create a unique numeric identifier for these (typeid? something?).
  4. Maybe we can trick the optimizer into thinking the functions are different. Maybe sneaking a printf() inside emit() would work? Some inline assembly?
  5. Maybe some special attributes could cause the function to not be affected by this optimization? (I couldn't find any.)
  6. Hope MSVC fixes this in the linker somehow (they know about it). Though that might not ever happen and even after that we need to stay compatible with previous releases.

In total, there are the following Windows-specific issues:

  • Signals on classes with virtual inheritance have unexpectedly large size on 32-bit (fixed in b83c116)
  • The above /OPT:ICF issue
  • Signals in classes with multiple inheritance work only when the bases are virtual (might help casting the signal pointer to an unknown type to expand it to its full size)
  • Signal with multiple inheritance don't seem to work at all on MinGW, even with the virtual workaround can't reproduce

@mosra mosra added this to TODO in Interconnect via automation Sep 16, 2018

@mosra mosra moved this from TODO to Important TODO in Interconnect Sep 16, 2018

@mosra mosra referenced this issue Oct 15, 2018

Closed

2018.10 release #265

56 of 56 tasks complete

@mosra mosra added this to the 2018.1d milestone Oct 15, 2018

@mosra mosra changed the title Fix /OPT:ICF issues with MSVC and the Interconnect library Fix issues with MSVC and the Interconnect library Oct 15, 2018

@mosra mosra changed the title Fix issues with MSVC and the Interconnect library Fix issues with Windows and the Interconnect library Oct 15, 2018

@mosra mosra self-assigned this Oct 15, 2018

@mosra mosra modified the milestones: 2019.01, 2019.0b Jan 29, 2019

@foodchaining

This comment has been minimized.

Copy link

commented Feb 27, 2019

That's very unfortunate and leads to nasty hard-to-prevent bugs.

Do you mean bugs in Corrade itself or in an application using Corrade::Interconnect? What a typical bug might look like with regard to this issue?

@mosra

This comment has been minimized.

Copy link
Owner Author

commented Feb 27, 2019

In the application. It'll result in a differently named signal being called, triggering completely different slots from what you'd expect.

@foodchaining

This comment has been minimized.

Copy link

commented Mar 9, 2019

What do you think about CORRADE_EMIT() macro workaround, but with a __FUNCSIG__ as a source of uniqueness?

__FUNCSIG__ (as well as __FUNCDNAME__) gives a full function signature, with substituted template arguments - if a function is templated.

@mosra

This comment has been minimized.

Copy link
Owner Author

commented Mar 9, 2019

Interesting, didn't know about __FUNCSIG__ / __FUNCDNAME__, thanks! Yes, this could probably work, however it probably will cause binary bloat as the function signatures will get embedded in the binary (mangled signatures are slightly shorter, but still), unless they get hashed or something, in which case it'll be a big compile-time disadvantage, I'm afraid.

The main problem is I don't like the macro :( If I compare to /OPT:NOICF (which also might cause binary bloat due to duplicates being present), forcing /OPT:NOICF on library users seems like a better idea.

@foodchaining

This comment has been minimized.

Copy link

commented Mar 9, 2019

The main problem is I don't like the macro :(

Who likes macros? But what if a macro had a bonus - by making signal declarations less verbose, something like CORRADE_TSIGNAL(Class, Signal, TemplateType, (int, std::string...))? I actually don't know whether such thing is possible, may be Boost Preprocessor has a recipe...

… compare to /OPT:NOICF (which also might cause binary bloat due to duplicates being present) …

Which variant takes more memory cannot be said for sure until measured and most likely the result will vary from application to application. Also, probably the macro can be optimized to use __FUNCDNAME__ only for templated signals and __COUNTER__ otherwise.

… forcing /OPT:NOICF on library users seems like a better idea.

Some will interpret such a restriction as a library defect!

@mosra

This comment has been minimized.

Copy link
Owner Author

commented Mar 10, 2019

I don't think the current signal declarations are verbose, quite the contrary. Using a macro like you suggest, in my opinion, would make things overly opaque and prevent signal name/signature autocompletion in "less gifted" IDEs. Moreover, the current design allows for extra flexibility -- turning references into copies, having default arguments for signals etc.

I see /OPT:NOICF as a known-to-be-working workaround for a non-conforming compiler (as per the reddit link above) -- which, in my book, is a defect on the compiler side, not on the library side. I still hope the workaround can be done in a less intrusive way. The reddit thread suggests using a function-local static variable. Another solution, similar to __FUNCDNAME__ but without the macro requirement, could be using std::source_location as a default argument to emit() (also see #55).

@mosra

This comment has been minimized.

Copy link
Owner Author

commented Mar 16, 2019

Finally got around to fixing this:

  • 0ce8cb4 extends the test to verify the actually problematic cases
  • f8b7e1c and a5b0dcb fixes issues with signals implemented in classes with multiple inheritance
  • and 5bfa63a explicitly disables /OPT:ICF for all users of the Interconnect library. I tried really hard to find some other solution that would be both reliable and not extremely annoying to use, but failed, so this has to do. Related docs.

Regarding MinGW, everything in the tests was "just working" with the current state, so I assume all works correctly there. If not, that'll get handled in a separate issue, since problems described here are fairly MSVC-specific.

@mosra mosra closed this Mar 16, 2019

Interconnect automation moved this from Important TODO to Done Mar 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.