MSVC support #35
MSVC support #35
Conversation
Where GCC and Clang issue warnings such as "empty struct is a GNU extension" (with `-Wgnu-empty-struct`) or "empty struct has size 0 in C, size 1 in C++" (with `-Wc++-compat`), MSVC issues an error C2016 "C requires that a struct or union has at least one member".
This fixes MSVC warning C4013 "'type cast': conversion from 'unsigned int' to 'void *' of greater size".
On Windows, `Sleep` is used. It's not as precise as `usleep`: it accepts milliseconds instead of microseconds, and even then its precision is limited to about 16ms. This is a test code though so it doesn't matter much.
Since we're already using `vsnprintf` and other C99 features, it's only natural to use `snprintf` as well. Moreover, `sprintf` is "deprecated" in Microsoft's CRT in favor of more secure (Microsoft-specific) alternatives with its use resulting in a warning, while `snprintf` isn't (although requires recent enough CRT).
This doesn't include changes to support `SIGSEGV` as even its support in its current form seems questionable to me. This means that to use ctest with MSVC you need to leave `CTEST_SIGSEGV` undefined.
|
I've merged some commits, but not all:
|
| @@ -79,14 +79,19 @@ CTEST_IMPL_DIAG_POP() | |||
| #define CTEST_IMPL_TEARDOWN_FPNAME(sname) CTEST_IMPL_NAME(sname##_teardown_ptr) | |||
|
|
|||
| #define CTEST_IMPL_MAGIC (0xdeadbeef) | |||
| #ifdef __APPLE__ | |||
| #if defined(_MSC_VER) | |||
There was a problem hiding this comment.
Do you know the minimum version of MSVC that these changes work for? Can we check for it in the code?
There was a problem hiding this comment.
IMHO there is no point in checking for a particular version since MSVC only started properly supporting C99 (think designated initializers and snprintf) quite recently (with VS 2013/2015).
There was a problem hiding this comment.
Okay, so you are saying that, since the code assumes C99 features are present, any MSVC version that supports C99 will work. If we ever decide to support C89 compilation though, we may want to revisit this.
| #pragma data_seg(push) | ||
| #pragma data_seg(".ctest$u") | ||
| #pragma data_seg(pop) | ||
| #define CTEST_IMPL_SECTION __declspec(allocate(".ctest$u")) __declspec(align(1)) |
There was a problem hiding this comment.
Style nit: we can combine these __declspecs into one:
__declspec(allocate(".ctest$u"), align(1))
Apparently some compilers even will complain if you don't.
There was a problem hiding this comment.
While works with MSVC, this comma-separated format is not documented AFAICT. MSDN article on __declspec only suggests that space-separated attributes are supported, and so does Clang test case on __declspec. I'd rather not change anything here at all, and silence corresponding Borland's warning/error (which is quite stupid if you ask me) if it comes to that.
There was a problem hiding this comment.
Totally reasonable, I'm fine with leaving it as-is.
| #ifdef __APPLE__ | ||
| #if defined(_MSC_VER) | ||
| #pragma data_seg(push) | ||
| #pragma data_seg(".ctest$u") |
There was a problem hiding this comment.
What is the significance of $u here? I cannot find any documentation online about it.
There was a problem hiding this comment.
Copied blindly from #29. I agree that "subsection" is not really needed in this case, will remove upon rebase.
| #pragma data_seg(".ctest$u") | ||
| #pragma data_seg(pop) | ||
| #define CTEST_IMPL_SECTION __declspec(allocate(".ctest$u")) __declspec(align(1)) | ||
| #elif defined(__APPLE__) | ||
| #define CTEST_IMPL_SECTION __attribute__ ((used, section ("__DATA, .ctest"), aligned(1))) |
There was a problem hiding this comment.
In the gcc/clang definitions of CTEST_IMPL_SECTION, we use the attribute "used" to prevent an optimizing compiler from stripping the symbol from the resulting object file for never being referenced in the code. Does MSVC provide an analogous __declspec or #pragma for this sort of thing?
There was a problem hiding this comment.
I didn't find anything like used right away. Also don't recall whether I tested the optimized build or not, will need to get back to you on this.
There was a problem hiding this comment.
A simple and portable workaround for the symbol-stripping problem would be to remove static from the definitions. Compilers cannot strip non-static symbols even if they appear unused since another compilation unit may be externing them. This would also remove the need for the used attribute in the __GNUC__ case.
|
@bvdberg I agree on both points regarding the example code. If you would change the cases with casts yourself that would be great. As for the |
|
Hi. We have been using ctest (nice library!) on a Mac box without problems, but because master does not currently compile on Windows, we have been trying out this PR. After applying it, it compiles flawlessly on VS 2017; however, it does not detect any test from the suite: As you see, no test is detected. After some debugging, we see that the culprit is the https://github.com/bvdberg/ctest/blob/master/ctest.h#L475 line, that breaks immediately, so not increasing the |
|
@FrancescAlted, thanks for testing this out. As I mentioned in one of the comments above, I didn't try using these changes on optimized builds. Is that something you're doing? And if so, could you try it on a debug build instead? Also, both loops there (including the one above one you're pointing to) need to break right away for ctest to be unable to find any tests. Early break in just one of them is totally fine. |
|
Hi Mike. Thanks for the response. Yes, we were trying a debug build (I did not try a release one yet). And yes, I confirm that both loops break right away, so this is the reason why no tests are detected. Using VS 2017 and Windows 10 here. |
|
@FrancescAlted Could you please provide a minimal code example exhibiting the problem, along with the compile and link commands used to build? |
|
I am using the Below is a transcript of how we build and run the Thanks in advance for any hint you may provide. |
|
The culprit seems to be with debugging information generation. CMake passes This worked for me: cmake_minimum_required(VERSION 3.0)
project(ctest C)
foreach(T DEBUG RELWITHDEBINFO)
string(REPLACE "/Zi" "" CMAKE_C_FLAGS_${T} "${CMAKE_C_FLAGS_${T}}")
string(REPLACE "/debug" "" CMAKE_EXE_LINKER_FLAGS_${T} "${CMAKE_EXE_LINKER_FLAGS_${T}}")
string(REPLACE "/INCREMENTAL" "" CMAKE_EXE_LINKER_FLAGS_${T} "${CMAKE_EXE_LINKER_FLAGS_${T}}")
endforeach()
add_executable(mytests main.c mytests.c)We'll need to dig into this, but the workaround above looks sensible in the meantime (you could adjust the flags for test projects only and see if that works). |
|
I have had a try at your CMakeLists file, but no luck. I have updated it slightly so as to use the existing test_example.c in our repo: And the output with this new cmake file is: |
|
I still see |
|
Urgh, this seems fragile indeed. It is unfortunate that macros are so little portable among different compilers. Thanks for the hint anyway; we will spend a bit more time on this and see. |
|
After fiddling with it a bit more it became clear that the incremental linking is in fact the real problem. Try this and see if it works for you: cmake_minimum_required(VERSION 3.0)
project(ctest C)
add_executable(mytests main.c mytests.c)
set_property(TARGET mytests APPEND_STRING PROPERTY LINK_FLAGS " /INCREMENTAL:NO")Note that instead of removing presumably present mytests.exe: CMakeFiles\mytests.dir\main.c.obj
mytests.exe: CMakeFiles\mytests.dir\mytests.c.obj
mytests.exe: CMakeFiles\mytests.dir\build.make
mytests.exe: CMakeFiles\mytests.dir\objects1.rsp
@$(CMAKE_COMMAND) -E cmake_echo_color --switch=$(COLOR) --green --bold --progress-dir=C:\Repo\ctest\obj\CMakeFiles --progress-num=$(CMAKE_PROGRESS_3) "Linking C executable mytests.exe"
"C:\Program Files\CMake\bin\cmake.exe" -E vs_link_exe --intdir=CMakeFiles\mytests.dir --manifests -- C:\PROGRA~2\MICROS~1\2017\BUILDT~1\VC\Tools\MSVC\1415~1.267\bin\Hostx86\x86\link.exe /nologo @CMakeFiles\mytests.dir\objects1.rsp @<<
/out:mytests.exe /implib:mytests.lib /pdb:C:\Repo\ctest\obj\mytests.pdb /version:0.0 /machine:X86 /debug /INCREMENTAL /subsystem:console /INCREMENTAL:NO kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib
<< |
|
Oh yeah, Just for reference, here it is a log on the new behaviour: Oh man, the black magic of compiler flags. Anyway, thanks a lot! |
|
@mikedld In light of this discussion on compiler flags, perhaps we should replace the Makefile in this repo with a CMakeLists.txt that has special handling for the MSVC case? |
|
@aceckel, I'm not strictly against it, but adding a section to README.md is okay with me too. Swithing to CMake may also seem a bit unfriendly since it's quite a requirement (well, not really if you think about it...) for a small project like this. I'd like to investigate this further though and maybe come up with a solution that works regardless of compiler/linker flags. |
|
@mikedld So I think the issue with incremental linking is that the linker inserts zero-padding around definitions, so we cannot rely on Not sure if Another option would be to add a static constructor to the the windows CRT (with |
|
That might look something like this (also untested): |
|
@mikedld I've confirmed that the above approach works correctly on MSVC 2017 with @FrancescAlted's original configuration. Additionally, I tried it with several combinations of optimization flags (including link-time optimization) and wasn't able to break it. Should I open a pull request to |
|
@aceckel Greatly appreciate you working on this. Linked list approach was something I had in mind when posting the last time. Using constructors is a nice trick but I'm a bit worried as to its support when non-MS linker/CRT is used on Windows in that we'll have to support two mechanisms for test cases discovery... I know that at least GCC and Clang have support for similar .ctors section too, but I also feel uneasy when reading phrases such as the following (re. C++), which may not be that horrible but I have no idea:
In any case, I'll need to rebase this branch first before you make any PRs to it. Will find time for it next couple of days. @bvdberg Anything you'd like to add here? If not for the recent discussion, maybe you have an idea on what to replace those sleep calls with? :) |
|
I never develop/work under any MS platform. Better for my bloodpressure ;) |
Instead of supporting multiple mechanisms for test discovery, we can use the linked-list approach for all platforms. For GNU-compatible compilers, we can use the
What do you mean by "non-MS on Windows" here? If you mean Cygwin or MinGW, I believe the
I believe the documentation tells the reader to compile as a C++ file and not a C file because the example code snippet they give to demonstrate CRT initialization is valid C++ but not valid C (a "initializer is not a constant" error occurs when compiling as a C file). AFAICT, the function pointers in
It looks like the sleep calls were removed in master so I don't think we need to worry about this incompatibility anymore.
Sounds good @mikedld, thanks! |
The changes here are based in part on #29 for which I thank @uael. This is a minimal set of changes needed to make use of ctest with recent versions of MSVC, without any warnings even on
/W4.Despite the fact that I have omitted Travis/AppVeyor stuff here (compared to #29), I do second the need in some kind of CI to make sure all the supported platforms are in fact supported.
Tested on Windows/MSVC, Linux/GCC, and Mac/Clang.