Comments (9)
Hi, thanks for the report!
For MSVC I'm adding /OPT:NOICF
, which disables "identical COMDAT folding", which as far as I understand is an optimization MSVC does to fold functions with identical contents together, so they all end up having the same function pointer.
That's useful in general, especially in template-heavy code, where a lot of different template instantiations may end up being the exact same machine code, just with different types in C++. But for signals, which are distinguished from each other by the value of their function pointer, this is a problem -- let's say, Signal pressed()
and Signal released()
are expected to be treated differently but (apart from emit()
taking their function pointer) they internally do the exact same thing so the compiler merges them into one function, leading to both being treated as the same signal.
I suppose clang-cl is doing something similar possibly, although I'm not aware of this problem on Linux or other platforms. Thus maybe clang-cl is using the MSVC linker and not its own, effectively inheriting the ICF optimization from it? So maybe using /OPT:NOICF
for clang-cl as well would work too? I have a clang-cl build on the CI but it's only a Debug build so I unfortunately didn't catch this myself.
Depending on whether you use Corrade as a CMake subproject or externally installed, it'd mean expanding the logic to add /OPT:NOICF
for clang-cl as well either directly here or in (your copy of) FindCorrade.cmake. Or just add it to linker flags of your project directly, for a quick test.
Just for the full picture -- something similar is happening on GCC with -Bsymbolic
, where (IIRC?) a function pointer has a different value depending on whether it's called from within a shared library that defines it, or from outside (thus connecting to the signal from outside simply didn't work), and in some other case with GCC I remember I had to make the signal definitions non-inline to prevent them from having a different function pointer value in every file that included the header.
It's quite a mess frankly, each new compiler version and every improvement to any optimizer being potentially breaking to this functionality. There are other signal/slot libraries based on the same principle (taking a pointer of the signal function to map them to slots) and all of them are running into the exact same issues. Libraries that don't, such as Qt, have some meta-compiler that circumvents this, but that was exactly the extra complexity I didn't want to deal with. On the other hand, the meta-compiler has the advantage that it can assign unique contiguous indices to the signals, making their lookup significantly faster than with a hashmap.
Maybe something like embedding a compile-time-hashed value of the __PRETTY_FUNCTION__
value could robustly prevent compilers from folding the signals together, but I didn't investigate anything like that so far yet.
from corrade.
Yes, I'm already using /OPT:NOICF
switch for MSVC, but it seems it doesn't have any effect for clang-cl, only __attribute__((optnone))
worked for me. Turning off optimizations only for this SignalData
constructor seems fine to me, but I'm not sure if it doesn't have any other consequences.
I also tried to insert OutputDebugString
call inside the SignalData
constructor to debug it, but with this call it started to work magically. So I'm not sure what's going on there.
I like Interconnect because I can define new events very easily. I would like to avoid taking extra steps to define an event.
from corrade.
with this call it started to work magically
Yeah, I suspect that made the optimizer exclude the function from optimization / merging / folding. Same was happening for me with the broken inline behavior, adding a printf statement made the problem go away.
I'm not on Windows but I tried to reproduce the issue on the CI, and indeed the tests broke when I switched to Release
. But, contrary to what you said, adding /OPT:NOICF
actually fixed it, making the tests pass again. It's commit f76ac49 in the next
branch, can you check it on your side?
I like Interconnect because I can define new events very easily. I would like to avoid taking extra steps to define an event.
Yes, that's exactly why I made the library, because the design seemed very easy to use in theory. But then compilers happened :D
from corrade.
I tried it again, rebuilt everything and it still doesn't work with /OPT:NOICF
. But I see linker knows about the switch, because if I change it to some non-existing /OPT:
, e.g. /OPT:NOICFTEST
, the linker shows error lld-link : error : /opt: unknown option: noicftest
. And that also probably means that clang-cl uses its own LLVM linker.
from corrade.
Ah well. Are you able to reproduce with Corrade's own tests? Enable them with CORRADE_BUILD_TESTS
and run the InterconnectTest
. For me the output with clang-cl and Release looked like this: https://ci.appveyor.com/project/mosra/corrade/build/next-3187/job/t5wk6f3gxvhsivbx?fullLog=true#L2538
I tried on the CI with -fuse-ld=lld
now but that passed the tests too. There's version 17.7.3 but I think such minor version difference alone isn't responsible for any behavior difference.
from corrade.
Yeah, all tests in InterconnectTest
passes in next
branch (emitterIdenticalSignals
fails in master
branch). But in my project, even non-identical signals don't work.
from corrade.
Would you be able to create a minimal repro case I could look at, or change the test in a way that makes it fail too? Do you use any extra compiler or linker flags besides the CMake defaults for Release? There's also one other test that checks sending signals across DLLs, in case a variant of that scenario in particular is what could trigger this behavior.
If everything else fails, I'll go with your original optnone
suggestion. But it feels like a rather imprecise hammer that could have unintended side effects, and also I'd really like to have a regression test case for this :)
from corrade.
I discovered that if I disable exceptions completely (because I have it disabled in my project), following test starts to fail. And optnone
fixes it, so it starts passing again. Interesting is that turning exceptions back on in my project doesn't help. I think clang-cl is a bit cursed. ๐ But I'm using multiple inheritance in my project too, so it may be related.
FAIL [17] emitterMultipleInheritance() at ...\corrade-master\src\Corrade\Interconnect\Test\Test.cpp:602
Containers mailbox.messages and (std::vector<std::string>{"hello", "<>ahoy<>"}) have different size, actual 1 but 2 expected. Actual contents:
{hello}
but expected
{<>ahoy<>, hello}
Actual hello but <>ahoy<> expected on position 0.
from corrade.
But for signals, which are distinguished from each other by the value of their function pointer, this is a problem
Even though the C++ standard says that two pointers to the same function must compare equal, in practice this isn't possible to implement on all platforms - for example, on Windows with imported DLL functions, the first time the function is called might patch the function address, and if you take the address before and after they won't compare equal. I'm not sure how relevant this is to you, but I recommend being careful when it comes to comparing function pointers.
from corrade.
Related Issues (20)
- Corrade's test suite fails under AddressSanitizer HOT 9
- Corrade with BUILD_TESTS=ON compilation error: call to non-โconstexprโ function HOT 5
- Error when installing via HunterGate HOT 7
- Prefix cmake options with CORRADE_ HOT 4
- Windows: inconsistent redefinition of _aligned_malloc HOT 3
- Corrade adds /wd*** warning disablements to "clang.exe" on windows. HOT 1
- Building Corrade with -std=c++20 causes errors inside MinGW <numbers> header HOT 3
- Corrade fails to compile with emscripten 3.1.22 HOT 4
- Opt-in to native UTF-8 support for OS interaction on Windows
- std::tuple_size / tuple_element specializations for Corrade containers HOT 1
- error: cannot initialize a variable of type 'const char *const' with an rvalue of type 'int' HOT 3
- Optimizing compilation time for the test suite -- an analysis HOT 3
- Interconnect - Slots are not called according to their record order HOT 1
- V8::Zone Allocator HOT 1
- NEON code does not build on armv7 HOT 2
- what to set CORRADE_INCLUDE_DIR to for in-source-builds HOT 2
- JsonToken::asObject() odd behavior with empty objects HOT 2
- New Release HOT 1
- Packaging location of GDB script files HOT 8
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. ๐๐๐
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google โค๏ธ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from corrade.