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

CMake code to run ASAN for Windows tests #463

Merged
merged 17 commits into from
Feb 19, 2023

Conversation

JoergAtGithub
Copy link
Contributor

This PR extends the CMake lists to run ASAN address sanitzer for tests under Windows (requires Win10 or 11).

  • It builds a standalone (no dependencies to local VisualStudio installation pathes) executable with ASAN code integrated.
  • In case of an address sanitizer failure, an ASAN Dump file is generated under out\build\x64-Debug\src\windows with a name like AsanDump_046D_C52F_0001_000C.dmp

CMakeLists.txt Outdated Show resolved Hide resolved
windows/CMakeLists.txt Outdated Show resolved Hide resolved
HIDAPI_ENABLE_ASAN is no set platform independent
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Comment on lines 13 to 16
if(HIDAPI_ENABLE_ASAN)
# Option /MTd needed to embedd ASAN library
add_compile_options(/MTd)
endif()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW: in my other project(s) I didn't it. for ASAN

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static linking is not needed in general, you could als add the ASAN DLL, that comes in various flavors with VisualStudio, to your DLL search path.
But it's placed in a directory with a name like this:
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.28.29333\bin\Hostx64\x86\clang_rt.asan_dbg_dynamic-i386.dll
I'm not able to determine this path using CMake. It depends on many things like VisualStudio installation path, Visual Studio version and the build type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is rather easy to get this path:

  • it is MSVC-specitic (so unser if(MSVC));
  • as per documentation - on MSVC the support is limited to x86 and x64 on Windows 10, so only one of two possible suffixes: i386 or x86_64;
  • the location is documented and it matches the path to CMAKE_C_COMPILER directory;

But in any case - I don't believe we need to handle that in our CMake. ASAN runs are designed to be executed only in development builds (e.g. no "production" scenarios).
The runtime is always available from MSVC development sonsole, so I believe that is just enough.

D:\a\hidapi\hidapi\hidapisrc\windows\hid.c : error C2220: the following warning is treated as an error
D:\a\hidapi\hidapi\hidapisrc\windows\hid.c : warning C5072: ASAN enabled without debug information emission. Enable debug info for better ASAN error reporting
@JoergAtGithub
Copy link
Contributor Author

This setup works for me on my local system. If I inject an memory access error, it reports it, otherwise every test is "Passed":
grafik
But on GitHub CI, at least the --output-on-error functionality of ctest seems not to work. But this seems to be a general ctest issue, not related to this ASAN PR.

@JoergAtGithub JoergAtGithub marked this pull request as ready for review October 22, 2022 06:55
@mcuee mcuee added the build system/CI Anything related to building the project or running on CI label Oct 28, 2022
@JoergAtGithub JoergAtGithub requested a review from Youw February 2, 2023 18:14
@Youw
Copy link
Member

Youw commented Feb 5, 2023

But on GitHub CI, at least the --output-on-error functionality of ctest seems not to work. But this seems to be a general ctest issue, not related to this ASAN PR.

Agree

@Youw
Copy link
Member

Youw commented Feb 5, 2023

warning C5072: ASAN enabled without debug information emission. Enable debug info for better ASAN error reporting

So we better make RelWithDebInfo builds on CI for ASAN.


I'm working in this right now. Will post an update shortly.

@JoergAtGithub
Copy link
Contributor Author

This looks much better than my attemp! Works well for me on local VS2022 as on CI.
But I get Linker warnings:

Run cmake --build . --config RelWithDebInfo --target install
MSBuild version 17.4.1+9a89d02ff for .NET Framework
  Checking Build System
  Building Custom Rule D:/a/hidapi/hidapi/hidapisrc/windows/CMakeLists.txt
  hid.c
  hidapi_descriptor_reconstruct.c
  Generating Code...
LINK : warning LNK4044: unrecognized option '/fsanitize=address'; ignored [D:\a\hidapi\hidapi\build\msvc\src\windows\hidapi_winapi.vcxproj]
LINK : warning LNK4300: ignoring '/INCREMENTAL' because input module contains ASAN metadata [D:\a\hidapi\hidapi\build\msvc\src\windows\hidapi_winapi.vcxproj]
LINK : warning LNK4044: unrecognized option '/fsanitize=address'; ignored [D:\a\hidapi\hidapi\build\msvc\src\windows\hidapi_winapi.vcxproj]
     Creating library D:/a/hidapi/hidapi/build/msvc/src/windows/RelWithDebInfo/hidapi.lib and object D:/a/hidapi/hidapi/build/msvc/src/windows/RelWithDebInfo/hidapi.exp

@Youw
Copy link
Member

Youw commented Feb 8, 2023

Hm, interesting. I haven't seen this warning locally. But I use MSVC 2019 though.

With gcc this linker option even required (otherwise the ASAN library is not linked).
I guess we need another MSVC workaround.

CMakeLists.txt Outdated Show resolved Hide resolved
@Youw
Copy link
Member

Youw commented Feb 8, 2023

As for:

warning LNK4300: ignoring '/INCREMENTAL' because input module contains ASAN metadata

I don't have a quick solution for it right now, and I'll probably keep it as is.

@JoergAtGithub
Copy link
Contributor Author

Why not setting /INCREMENTAL:NO ?

@Youw
Copy link
Member

Youw commented Feb 9, 2023

AFAIK MSVC compiler/linker usually complains if the same option is passed multiple times, and /INCREMENTAL is already present in the link option list, so it is not about setting it, but rather replacing which is not as trivial as one might think

and I like simple/elegant solutions

but OK, I'll try

@Youw
Copy link
Member

Youw commented Feb 19, 2023

probably time to merge it, before it gets ugly again

@Youw Youw merged commit 624ae35 into libusb:get-descriptor Feb 19, 2023
@JoergAtGithub JoergAtGithub deleted the ASAN3 branch February 19, 2023 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build system/CI Anything related to building the project or running on CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants