diagnostic_updtater windows crash fix

[micmzr]

The example.cpp file from the diagnostic_updater package compiles correctly but causes the application to crash on Windows. It appears that the updater module is not being imported properly due to linking issues. The following fix resolves this problem by removing the problematic CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=ON flag.

This issue also causes the controller-manager component to crash on Windows, as it relies on diagnostic_updater. I believe this affects all versions newer than Humble.

[traversaro]

Thanks for the fix, but please restore all the changes in vinca.yaml, and bump the rebuild packages build number to 26, not 1.

[traversaro]

The example.cpp file from the diagnostic_updater package compiles correctly but causes the application to crash on Windows. It appears that the updater module is not being imported properly due to linking issues. The following fix resolves this problem by removing the problematic CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=ON flag.

This issue also causes the controller-manager component to crash on Windows, as it relies on diagnostic_updater. I believe this affects all versions newer than Humble.

Can you tell us more about the crash? CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=ON by itself is not particularly problematic.

[micmzr]

I restored the original vinca.yaml file.
Actually, CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=ON might not be the cause of the problem, but I don't think it's necessary anymore. I added visibility definitions for the DLL, and it looks like that solves the issue with diagnostic_updater.

Unfortunately, I cannot say much about this error. The example.cpp file crashes upon the first use of the updater.add() function:

updater.add("Function updater", dummy_diagnostic);

By examining the original version using cdb, we obtain the following information:

(9ab4.9484): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
353425ff`00023822 ?? ???
0:000> k
Child-SP          RetAddr               Call Site
00000097`290fda78 00000000`00000000     0x353425ff`00023822
00000097`290fda78 00000000`00000000     0x0

It looks like the stack is corrupted. Every call to the updater object's methods causes a crash. I suspected linking issues, so I changed the visibility for the DLL—and the problem went away.

[traversaro]

fyi @gftabor

[gftabor]

Ya jazzy already had this
https://github.com/RoboStack/ros-jazzy/blob/main/patch/ros-jazzy-diagnostic-updater.win.patch

[traversaro]

Ya jazzy already had this https://github.com/RoboStack/ros-jazzy/blob/main/patch/ros-jazzy-diagnostic-updater.win.patch

Actually this PR suggest that that patch is broken/problematic!

[micmzr]

The Jazzy version (4.2.7) actually works correctly, but diagnostics_updater causes a crash in the Lyrical and Rolling versions.

[micmzr]

Honestly, I don't see any significant difference when comparing the code for versions 4.2.7 and 4.5.7 of the diagnostic_updater package. Yet, it doesn't work in Lyrical and Rolling, causing the controller_manager to crash—which is a bit more annoying.

[traversaro]

Honestly, I don't see any significant difference when comparing the code for versions 4.2.7 and 4.5.7 of the diagnostic_updater package. Yet, it doesn't work in Lyrical and Rolling, causing the controller_manager to crash—which is a bit more annoying.

I think jazzy works as there is also https://github.com/RoboStack/ros-jazzy/blob/main/patch/ros-jazzy-diagnostic-updater.patch . So somehow CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS is not sufficient. Given that, I think once you add the visibility headers, probably it is better to always use those, instead of mixing with CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS. In general I have a preference for CMake's GenerateExportHeader, but whatever upstream prefer is fine for me.

[traversaro]

This issue also causes the controller-manager component to crash on Windows, as it relies on diagnostic_updater. I believe this affects all versions newer than Humble.

Do you have any idea on how to reproduce this in a launch-style tests, something like what we added in RoboStack/ros-lyrical#54 ? As we do not understand the kind of problem, having a regression test would be quite useful.

I added visibility definitions for the DLL, and it looks like that solves the issue with diagnostic_updater.

Are you sure you added all the required visibility definitions? Note that CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS is still set by additional_cmake_args in pkg_additional_info.yaml .

[traversaro]

By the way, the exact same issue was debugged by @gftabor in RoboStack/ros-jazzy#81 (comment) . I think my original PR ros/diagnostics#426 was wrong, as we are in the case (quoting from https://cmake.org/cmake/help/latest/prop_tgt/WINDOWS_EXPORT_ALL_SYMBOLS.html?utm_source=chatgpt.com):

In cases that the compiler generates references to the virtual function table, such as in a delegating constructor of a class with virtual functions, the whole class must be marked with __declspec(dllimport) in order to link to the vftable in the .dll.

so in this case we can't (only) use CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS. Interestingly, in this case the upstream actually added a Windows CI, so it would probably make sense to add a fail test in the upstream CI to detect the problem.

[micmzr]

I left CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS in place because I don't see it having any negative impact.

I'm afraid not. I tested it like this:

ros2 run diagnostic_updater example

git clone https://github.com/ros-controls/ros2_control_demos.git
cd ros2_control_demos
colcon build --packages-select ros2_control_demo_example_1 ros2_control_demo_description ros2_control_demo_test_utils
call install\setup.bat
ros2 launch ros2_control_demo_example_1 rrbot.launch.py

[traversaro]

Ok, that is clear. Can you fix the sort CI?

[micmzr]

I think I've fixed the sorting issue.

[traversaro]

Thanks! If you want to port the same PR in lyrical, that is very welcome!