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 -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Allow-to-build-against-static-MSVC-runtime.patch Type: text/x-patch Size: 2055 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100306/69a65259/attachment.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0002-Include-stdint.h-when-HAVE_STDINT_H-is-defined-on-MS.patch Type: text/x-patch Size: 1244 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100306/69a65259/attachment-0001.bin>
Le 06/03/2010 11:43, José Fonseca a écrit :> 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 > >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
On Sat, Mar 6, 2010 at 11:33 AM, Cédric Venet <cedric.venet at laposte.net> wrote:> Le 06/03/2010 11:43, José Fonseca a écrit : >> >> 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 >> > > 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)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
José Fonseca <jose.r.fonseca at 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?> typedef __int64 int64_t; > typedef unsigned __int64 uint64_t; > typedef signed int int32_t; > @@ -145,6 +148,7 @@ typedef signed int ssize_t; > #ifndef UINT64_C > # define UINT64_C(C) ((uint64_t) C ## ULL) > #endif > +#endif /* !HAVE_STDINT_H */ > #endif /* _MSC_VER */ > > /* Set defaults for constants which we cannot find. */
On Sat, Mar 6, 2010 at 9:49 PM, Óscar Fuentes <ofv at wanadoo.es> wrote:> José Fonseca <jose.r.fonseca at 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... On Sat, Mar 6, 2010 at 4:26 PM, OvermindDL1 <overminddl1 at gmail.com> wrote:> On Sat, Mar 6, 2010 at 4:19 PM, José Fonseca <jose.r.fonseca at gmail.com> wrote: >>>> --- 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 > > Boost has such an stdint compatibility header in their TR1 section. > Why not take it and put it in LLVM itself, the boost license > encourages that use and it interoperates properly with every known > compiler, adding the missing things on those compilers if necessary. >
On Sat, Mar 6, 2010 at 11:27 PM, OvermindDL1 <overminddl1 at gmail.com> wrote:> To the mailing list this time... > > On Sat, Mar 6, 2010 at 4:26 PM, OvermindDL1 <overminddl1 at gmail.com> wrote: >> On Sat, Mar 6, 2010 at 4:19 PM, José Fonseca <jose.r.fonseca at gmail.com> wrote: >>>>> --- 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 >> >> Boost has such an stdint compatibility header in their TR1 section. >> Why not take it and put it in LLVM itself, the boost license >> encourages that use and it interoperates properly with every known >> compiler, adding the missing things on those compilers if necessary.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