[PATCH]: MSVC build enhancements

Attached are two patches with MSVC build enchancements.

They are quite trivial, but were necessary to correctly link LLVM
libraries with Mesa3D on Windows.

Jose

0001-Allow-to-build-against-static-MSVC-runtime.patch (2.01 KB)

0002-Include-stdint.h-when-HAVE_STDINT_H-is-defined-on-MS.patch (1.21 KB)

Are you volontary trying to break everyone build (just to build your own project), or have you no idea of the effect of this change:

+add_llvm_definitions( -D_SECURE_SCL=0 )

While I personnaly use this flag in all my projects, it should not be silently and sneakily imposed to all llvm user. You should make it an option, and keep the default as it is currently. I.e. make this an opt-in choice.

While I may seem harsh, this flag change the ABI !!! and its effects are very hard to understand and debug. For me it is criminal to try to pass this as a "trivial" patch. (already been bitten here)

regards,
Cédric

I had these changes locally for many months now and thought it might
be useful to others so decided to submit them before 2.7. I have no
vested interested in them for my project as I always build llvm
myself.

That flag was suggested in some website to solve issues when
statically linking release LLVM libraries in a debug build. I had no
idea of the full implications, nor can I really deduct from your reply
how deep they are, but I'll investigate it further.

It was not my intention to break things. To be honest the whole
process of statically linking libraries in MSVC is full of flaws and
is not a little bit robust, so I don't get surprised with anything
anymore.

So feel free to ignore the chunk or the whole patch, or the whole set.

Jose

This flag change the ABI of the M$ STL (by removing debug member variable). So all the C++ lib linked (statically or not) must have the same setting for this flag or must not exchange STL structure (and for static linking, there is probably ODR problems lurking).
The default is _SECURE_SCL=1 on current version of VS (they changed for VS2010 it seems).

So adding an option for adding this flag would be great but not changing the default. (The flag is interesting because it can leads to great performance improvement in STL heavy code (up to x10 and more on particular code)).

regards,

Cédric

José Fonseca <jose.r.fonseca@gmail.com> writes:

Attached are two patches with MSVC build enchancements.

They are quite trivial, but were necessary to correctly link LLVM
libraries with Mesa3D on Windows.

[snip]

   add_llvm_definitions( -D_SCL_SECURE_NO_DEPRECATE )
+ add_llvm_definitions( -D_SECURE_SCL=0 )

With this setting the default LLVM build becomes incompatible with
libraries compiled with _SECURE_SCL=1 (which is the default
setting). The right thing here is to use an option.

- add_llvm_definitions("/${LLVM_USE_CRT}")
+ # http://www.cmake.org/Wiki/CMake_FAQ#How_can_I_build_my_MSVC_application_with_a_static_runtime.3F
+ foreach(flag_var
+ CMAKE_C_FLAGS CMAKE_C_FLAGS_DEBUG CMAKE_C_FLAGS_RELEASE CMAKE_C_FLAGS_MINSIZEREL CMAKE_C_FLAGS_RELWITHDEBINFO
+ CMAKE_CXX_FLAGS CMAKE_CXX_FLAGS_DEBUG CMAKE_CXX_FLAGS_RELEASE CMAKE_CXX_FLAGS_MINSIZEREL CMAKE_CXX_FLAGS_RELWITHDEBINFO)
+ if(${flag_var} MATCHES "/MD")
+ string(REGEX REPLACE "/MD" "/${LLVM_USE_CRT}" ${flag_var} "${${flag_var}}")
+ endif(${flag_var} MATCHES "/MD")
+ endforeach(flag_var)
     message(STATUS "Using VC++ CRT: ${LLVM_USE_CRT}")
   endif (NOT ${LLVM_USE_CRT} STREQUAL "")

Ugh! Well, if this is the only way... However, AFAIK the last flag is
the one that prevails, and that's the one introduced by
add_llvm_definitions. What's the problem if you don't replace the switch
everywhere? Does the replacement method above works well with debug
builds? (On debug builds, /MDd is used by default instead of /MD, so
replacing /MD with /MTd, for instance, would give /MTdd)

--- a/include/llvm/System/DataTypes.h.cmake
+++ b/include/llvm/System/DataTypes.h.cmake
@@ -100,6 +100,9 @@ typedef u_int64_t uint64_t;
#else
#include <math.h>
#endif
+#ifdef HAVE_STDINT_H
+#include <stdint.h>
+#else /* !HAVE_STDINT_H */

This looks incorrect to me. stdint.h is already included below for
non-MSVC systems. Maybe you inteded to #include it for MSVC only? Or do
intend to correct some problem with non-MSVC builds too?

Whoops, mailing list headers still broken, sending to the list this time:

you mean the faster time *was* 10 ms, not that
there was a 10 ms difference...

12 minutes = 720 seconds = 720000 ms. You describe a 72000x difference :slight_smile:

-Isaac

Bah, freaking list headers still broken!

But yes, my whole definition line above, I would love it if all of
those were added to CMake (non-default I guess) so I did not need to
add it to the compile line manually. :slight_smile:

José Fonseca <jose.r.fonseca@gmail.com> writes:

Attached are two patches with MSVC build enchancements.

They are quite trivial, but were necessary to correctly link LLVM
libraries with Mesa3D on Windows.

[snip]

add_llvm_definitions( -D_SCL_SECURE_NO_DEPRECATE )
+ add_llvm_definitions( -D_SECURE_SCL=0 )

With this setting the default LLVM build becomes incompatible with
libraries compiled with _SECURE_SCL=1 (which is the default
setting). The right thing here is to use an option.

OK.

- add_llvm_definitions("/${LLVM_USE_CRT}")
+ # http://www.cmake.org/Wiki/CMake_FAQ#How_can_I_build_my_MSVC_application_with_a_static_runtime.3F
+ foreach(flag_var
+ CMAKE_C_FLAGS CMAKE_C_FLAGS_DEBUG CMAKE_C_FLAGS_RELEASE CMAKE_C_FLAGS_MINSIZEREL CMAKE_C_FLAGS_RELWITHDEBINFO
+ CMAKE_CXX_FLAGS CMAKE_CXX_FLAGS_DEBUG CMAKE_CXX_FLAGS_RELEASE CMAKE_CXX_FLAGS_MINSIZEREL CMAKE_CXX_FLAGS_RELWITHDEBINFO)
+ if(${flag_var} MATCHES "/MD")
+ string(REGEX REPLACE "/MD" "/${LLVM_USE_CRT}" ${flag_var} "${${flag_var}}")
+ endif(${flag_var} MATCHES "/MD")
+ endforeach(flag_var)
message(STATUS "Using VC++ CRT: ${LLVM_USE_CRT}")
endif (NOT ${LLVM_USE_CRT} STREQUAL "")

Ugh! Well, if this is the only way... However, AFAIK the last flag is
the one that prevails, and that's the one introduced by
add_llvm_definitions. What's the problem if you don't replace the switch
everywhere?

It didn't work. I was still getting libraries with the wrong CRT.

I think it's because these flags don't really override each other. My
understanding is that these options work by defining _DEBUG _MT and
_DLL preprocessing symbols
(http://msdn.microsoft.com/en-us/library/2kzt1wy3.aspx) which are
recognized by the CRT headers and used to select the right functions
propotypes and the right CRT .LIB via #pragmas defaultlib. And appears
that defining multiple flags gets you a mixed behavior, instead of
overriding earlier flags.

Does the replacement method above works well with debug
builds? (On debug builds, /MDd is used by default instead of /MD, so
replacing /MD with /MTd, for instance, would give /MTdd)

I've used that snippet from the CMake in several projects and it
appears to work well. But it might be worth checking it to make sure.

--- a/include/llvm/System/DataTypes.h.cmake
+++ b/include/llvm/System/DataTypes.h.cmake
@@ -100,6 +100,9 @@ typedef u_int64_t uint64_t;
#else
#include <math.h>
#endif
+#ifdef HAVE_STDINT_H
+#include <stdint.h>
+#else /* !HAVE_STDINT_H */

This looks incorrect to me. stdint.h is already included below for
non-MSVC systems. Maybe you inteded to #include it for MSVC only? Or do
intend to correct some problem with non-MSVC builds too?

The idea here is indeed to use an external stdint.h in MSVC builds.

The problem is that everybody except Microsoft recognized the
usefulness of stdint.h types [1], and they work around it for MSVC by
defining the stdint.h types themselves. LLVM does it. Mesa3D did it
too. But that won't work when two such projects try to interoperate --
you'll get errors that uint8_t/uint16_t/uint32_t/etc has been
redefined.

There is more than one way to address this. The solution I came up
with was to make all projects respect HAVE_STDINT_H, and only define
the stdint types if HAVE_STDINT_H is not defined. Then all projects
can use the same definition, without symbol clashes. We actually ship
a tiny stdint.h in Mesa3D,
http://cgit.freedesktop.org/mesa/mesa/tree/include/c99/stdint.h , but
there are many free replacements out there.

This is not a problem specific to Mesa and LLVM -- it is a problem
faced by any pair of projects that use stdint types and want to build
on MSVC.

Jose

[1] I'm being unfair. Microsoft certainly recognized by now how useful
the stdint.h types are to write portable code, which is probably the
reason why they haven't been implemented yet. It doesn't appear C99
will be implemented soon either:
https://connect.microsoft.com/VisualStudio/feedback/details/485416/support-c99

To the mailing list this time...

You mean https://svn.boost.org/trac/boost/browser/trunk/boost/cstdint.hpp .

Levering their definitions sounds a good idea in general. There are
some tweaks necessary: boost defines only cstdint.hpp, not stdint.h,
and it actually includes system's stdint.h. This won't work with the
LLVM-C bindings.

OK. It seems my changes are far from trivial. Apologies for naively
assuming they were.

I think it's better not apply any of this for LLVM 2.7. I'll workout
individual patches based on the feedback I got and re-submit at a time
it can receive more testing without fear of breaking stuff.

Jose