Re: [Mingw-w64-public] InterlockedIncrement boost (yes, again) -What's the right answer here?
Jacek Caban schreef op ma 22-07-2013 om 11:50 [+0200]: On 07/21/13 23:24, dw wrote: Attached is the patch I came up with to fix the build issue. You are checking for defined(__MINGW64_VERSION_MAJOR). Would it make sense to do (__MINGW64_VERSION_MAJOR = 3)? IMO, if the change works with older mingw-w64 release, not checking version is better. If it doesn't, then yeah, something like that may be needed. All my tests were done against trunk snapshots (so major version 3). I could add such a conditional, but what benefit would it have? The mingw-w64 v1 branch also has an intrin.h with declarations for the Interlocked symbols, so the current patch should work just fine on the v1 branch. Regards, Erik -- See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831iu=/4140/ostg.clktrk ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] InterlockedIncrement boost (yes, again) -What's the right answer here?
On 7/23/13 10:53 PM, Erik van Pienbroek wrote: Jacek Caban schreef op ma 22-07-2013 om 11:50 [+0200]: On 07/21/13 23:24, dw wrote: Attached is the patch I came up with to fix the build issue. You are checking for defined(__MINGW64_VERSION_MAJOR). Would it make sense to do (__MINGW64_VERSION_MAJOR = 3)? IMO, if the change works with older mingw-w64 release, not checking version is better. If it doesn't, then yeah, something like that may be needed. All my tests were done against trunk snapshots (so major version 3). I could add such a conditional, but what benefit would it have? The mingw-w64 v1 branch also has an intrin.h with declarations for the Interlocked symbols, so the current patch should work just fine on the v1 branch. I think that what you check should be enough to expect it to work with older mingw-w64, so it's better to no have an additional version check, IMO. Thanks, Jacek -- See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831iu=/4140/ostg.clktrk ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] InterlockedIncrement boost (yes, again) -What's the right answer here?
On 07/21/13 17:02, Erik van Pienbroek wrote: Erik van Pienbroek schreef op zo 21-07-2013 om 14:49 [+0200]: So now I think it's up to us to come up with the most proper fix which we can then try to get upstreamed. Attached is the patch I came up with to fix the build issue. It is basically method 2 in dw's original mail. The header in question already contained an #include intrin.h, but it was only part of an #ifdef _MSC_VER section (which doesn't get triggered for mingw-w64). I've changed this #ifdef clause so that it also includes intrin.h when the mingw-w64 compiler is used. With this patch I've done the following build tests: using an old mingw-w64 snapshot (20121016): success! using a recent mingw-w64 snapshot (20130713, r5949): success! using a bleeding edge mingw-w64 snapshot (20130721, r5969): success! The qpid-cpp build failure is also resolved with this patch as well. Unless somebody objects I think this patch is good enough to be proposed upstream. Looks good to me. Thanks, Jacek -- See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831iu=/4140/ostg.clktrk ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] InterlockedIncrement boost (yes, again) -What's the right answer here?
On 07/21/13 23:24, dw wrote: Attached is the patch I came up with to fix the build issue. You are checking for defined(__MINGW64_VERSION_MAJOR). Would it make sense to do (__MINGW64_VERSION_MAJOR = 3)? IMO, if the change works with older mingw-w64 release, not checking version is better. If it doesn't, then yeah, something like that may be needed. Jacek -- See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831iu=/4140/ostg.clktrk ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] InterlockedIncrement boost (yes, again) -What's the right answer here?
I vote for this. Boost can always be fixed, and it contains lots of ugly hacks around various platform obscurities. I think MSVC intrinsics combined with GCC are a valid obscurity. Granted, if Boost is to change, you might as well give them the best performance while we're at it :) It's (apparently) a pretty trivial change and one that people are already recommending. Doesn't seem that significant an issue (to me). Why not explain it to the Boost mailing list? I'm rather sure they'd be inclined to accomodate us, if there's no fundamental problems with the changes. While I could write the email, I'm not sure they'd listen to me. It's not like they know who I am. Also, I've been told that my posts are sometimes perceived as confrontational. And that's not a good thing for this kind of effort. dw -- See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831iu=/4140/ostg.clktrk ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] InterlockedIncrement boost (yes, again) -What's the right answer here?
With this specific define set the boost package can indeed be compiled without issues (for both the x86 and x64 targets). Yay! However, there's a catch! The boost build system doesn't embed this specific define in its installed headers. It expects that all boost-using projects (like qpid-cpp) also have this BOOST_USE_WINDOWS_H define set otherwise it will also cause build failures there. I don't know squat about boost. But it seems like building boost with one set of options, then building downstream projects that use boost with a different set of options is a bad idea. If defining BOOST_USE_WINDOWS_H is what's needed to work with our v3, this doesn't seem to me like an unreasonable burden. Especially when it also gains them performance improvements. dw -- See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831iu=/4140/ostg.clktrk ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] InterlockedIncrement boost (yes, again) -What's the right answer here?
Still, the question if we want those in our crt is a separated issue. That's a question of being backward compatible, which is important IMO. Too bad those were introduced in the first place... To me, this is the key question. I agree that breaking backward compatibility is a bad thing. And worse, we don't have any idea how many projects would be affected. And really, the fix is pretty trivial (like I say, probably just an alias). On the other hand, if someone were to post a message to some project saying I'm using your header files to access someone else's lib files and it doesn't always work right, who would you say is at fault? The header people? The lib people? Or the crazy person who is trying to mix the two? I'm still prepared to add these definitions back to the library file if people feel strongly. I just want to point out that if I do, there will be no way to access these symbols using our headers. You must be using boost (or some similar project) to reference them. And that just feels odd. In summary, I've got Ruben who (I think) is saying to leave them out and work with boost (and others) to deal with the consequences. And Jacek who (I think) is saying we should put them back to maintain backward compatibility. If this were just a technical question, I'd vote to leave them out. But really it's more of a political question. And that's something I'm really bad at. So, who decides? If it's me, I'm probably going to wimp out and add the defs back to avoid the conflict. dw -- See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831iu=/4140/ostg.clktrk ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] InterlockedIncrement boost (yes, again) -What's the right answer here?
dw schreef op za 20-07-2013 om 23:48 [-0700]: So, who decides? If it's me, I'm probably going to wimp out and add the defs back to avoid the conflict. I've just forwarded all our information to the Fedora maintainer of the mingw-boost package - Thomas Sailer - and asked him if he could provide his opinion about the situation and perhaps get in contact with upstream boost devs to discuss this issue. Erik -- See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831iu=/4140/ostg.clktrk ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] InterlockedIncrement boost (yes, again) -What's the right answer here?
Erik van Pienbroek schreef op zo 21-07-2013 om 12:22 [+0200]: dw schreef op za 20-07-2013 om 23:48 [-0700]: So, who decides? If it's me, I'm probably going to wimp out and add the defs back to avoid the conflict. I've just forwarded all our information to the Fedora maintainer of the mingw-boost package - Thomas Sailer - and asked him if he could provide his opinion about the situation and perhaps get in contact with upstream boost devs to discuss this issue. I've just got a response from Thomas. He has indicated that he doesn't really have a relationship with the upstream boost devs, but he's willing to try to get any patches upstreamed which we can come up with. So now I think it's up to us to come up with the most proper fix which we can then try to get upstreamed. Regards, Erik -- See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831iu=/4140/ostg.clktrk ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] InterlockedIncrement boost (yes, again) -What's the right answer here?
Erik van Pienbroek schreef op zo 21-07-2013 om 14:49 [+0200]: So now I think it's up to us to come up with the most proper fix which we can then try to get upstreamed. Attached is the patch I came up with to fix the build issue. It is basically method 2 in dw's original mail. The header in question already contained an #include intrin.h, but it was only part of an #ifdef _MSC_VER section (which doesn't get triggered for mingw-w64). I've changed this #ifdef clause so that it also includes intrin.h when the mingw-w64 compiler is used. With this patch I've done the following build tests: using an old mingw-w64 snapshot (20121016): success! using a recent mingw-w64 snapshot (20130713, r5949): success! using a bleeding edge mingw-w64 snapshot (20130721, r5969): success! The qpid-cpp build failure is also resolved with this patch as well. Unless somebody objects I think this patch is good enough to be proposed upstream. Regards, Erik --- boost/detail/interlocked.hpp.interlocked 2012-12-11 15:42:26.0 +0100 +++ boost/detail/interlocked.hpp 2013-07-21 15:22:56.082346444 +0200 @@ -69,9 +69,9 @@ # define BOOST_INTERLOCKED_EXCHANGE_POINTER(dest,exchange) \ ((void*)BOOST_INTERLOCKED_EXCHANGE((long*)(dest),(long)(exchange))) -#elif defined( BOOST_MSVC ) || defined( BOOST_INTEL_WIN ) +#elif defined( BOOST_MSVC ) || defined( BOOST_INTEL_WIN ) || defined( __MINGW64_VERSION_MAJOR ) -#if defined( BOOST_MSVC ) BOOST_MSVC = 1600 +#if ( defined( BOOST_MSVC ) BOOST_MSVC = 1600 ) || defined( __MINGW64_VERSION_MAJOR ) #include intrin.h @@ -93,12 +93,16 @@ #endif +# if defined( BOOST_MSVC ) + # pragma intrinsic( _InterlockedIncrement ) # pragma intrinsic( _InterlockedDecrement ) # pragma intrinsic( _InterlockedCompareExchange ) # pragma intrinsic( _InterlockedExchange ) # pragma intrinsic( _InterlockedExchangeAdd ) +# endif + # if defined(_M_IA64) || defined(_M_AMD64) extern C void* __cdecl _InterlockedCompareExchangePointer( void* volatile *, void*, void* ); -- See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831iu=/4140/ostg.clktrk___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] InterlockedIncrement boost (yes, again) -What's the right answer here?
Attached is the patch I came up with to fix the build issue. You are checking for defined(__MINGW64_VERSION_MAJOR). Would it make sense to do (__MINGW64_VERSION_MAJOR = 3)? dw -- See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831iu=/4140/ostg.clktrk ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] InterlockedIncrement boost (yes, again) -What's the right answer here?
dw schreef op za 20-07-2013 om 02:07 [-0700]: An argument could be made that we have broken backward compatibility and it's our responsibility to fix it. On the other hand, one could say they are using our library incorrectly (by not including any of our headers), and the fact that it worked at all was a fluke and inconsistent with the MSDN docs. I would say we have to mimic the MSVC behavior as close as possible. This bring us to the question whether boost can be compiled successfully for x86_64 using MSVC. If the upstream boost devs have added workarounds for mingw-specific toolchain bugs we could try to persuade them to drop or loosen these workarounds. Regards, Erik van Pienbroek -- See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831iu=/4140/ostg.clktrk ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] InterlockedIncrement boost (yes, again) -What's the right answer here?
On 07/20/13 12:22, Erik van Pienbroek wrote: dw schreef op za 20-07-2013 om 02:07 [-0700]: An argument could be made that we have broken backward compatibility and it's our responsibility to fix it. On the other hand, one could say they are using our library incorrectly (by not including any of our headers), and the fact that it worked at all was a fluke and inconsistent with the MSDN docs. I would say we have to mimic the MSVC behavior as close as possible. This bring us to the question whether boost can be compiled successfully for x86_64 using MSVC. If the upstream boost devs have added workarounds for mingw-specific toolchain bugs we could try to persuade them to drop or loosen these workarounds. Agreed, they should use intrin.h like they do on recent MSVC and everything should be fine. This applies to both 32-bit and 64-bit version (on 32-bit version, this will give them perf boost). Still, the question if we want those in our crt is a separated issue. That's a question of being backward compatible, which is important IMO. Too bad those were introduced in the first place... Jacek -- See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831iu=/4140/ostg.clktrk ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] InterlockedIncrement boost (yes, again) -What's the right answer here?
2013/7/20 dw limegreenso...@yahoo.com So, Erik was kind enough to try re-running some of his builds with the latest patches to winbase.h. With a bit of tweaking to the patch, x86 now builds. While I haven't checked it in yet, these DLLIMPORT things are fixed. Unfortunately, x64 does not build correctly. If you want to see the raw details: Error log: http://fpaste.org/26679/42789171/raw/ -E output from one of the failing files: http://vanpienbroek.nl/SaslFactory.obj The important source file is http://svn.boost.org/svn/boost/trunk/boost/detail/interlocked.hpp. It is currently using the definitions from line 143. To save you the time, the problem is that for x64, they are NOT including winbase.h (which MSDNhttp://msdn.microsoft.com/en-us/library/ms683614%28v=VS.85%29.aspxsays is where the def exists). They are (as before) using their own def. Since they are not using our headers, the intrinsics are not available, and the mapping from the PSDK function to the intrinsic (which is what MS does for x64) isn't done. This used to work before my patch because intrincs/ilockinc.c used to (incorrectly, in my opinion) have definitions for both the intrinsic (_InterlockedIncrement) and the PSDK (InterlockedIncrement). Now it only has _InterlockedIncrement. FYI, while MSDN says that this function is in kernel32.lib, that is only true for x86. So, that's what's happening. Now, what do we do about it? A few alternatives that occur to me (in no particular order). Boost could: 1) Use winbase.h (via windows.h) like the MSDN docs say they should. In fact, I wonder if defining BOOST_USE_WINDOWS_H would work. 2) In the #if defined(__MINGW64__) block just above, they could add these underscore defines along with #include intrin.h. 3) They could probably just add the defines for the underscores to the __MINGW64__ block. Unlike 1 2, this would NOT get them the inlines, but would resolve from the library like it did before. Or, we could: 4) Add InterlockedIncrement back to ilockinc.c (likewise for the other half dozen or so functions here), probably using alias. An argument could be made that we have broken backward compatibility and it's our responsibility to fix it. On the other hand, one could say they are using our library incorrectly (by not including any of our headers), and the fact that it worked at all was a fluke and inconsistent with the MSDN docs. I vote for this. Boost can always be fixed, and it contains lots of ugly hacks around various platform obscurities. I think MSVC intrinsics combined with GCC are a valid obscurity. Granted, if Boost is to change, you might as well give them the best performance while we're at it :) Why not explain it to the Boost mailing list? I'm rather sure they'd be inclined to accomodate us, if there's no fundamental problems with the changes. Ruben Making things work like they did before (#4) causes the fewest problems for boost. 1 2 are cleaner fixes that will help boost produce better code, and keep our own code cleaner, but might irritate the boost folks (and possibly others). I'm (reluctantly) prepared to add the defs back to the library if that seems like the right thing to do. But I could use some guidance about what the right thing to do is. dw -- See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831iu=/4140/ostg.clktrk ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public -- See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831iu=/4140/ostg.clktrk___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] InterlockedIncrement boost (yes, again) -What's the right answer here?
dw schreef op za 20-07-2013 om 02:07 [-0700]: Boost could: 1) Use winbase.h (via windows.h) like the MSDN docs say they should. In fact, I wonder if defining BOOST_USE_WINDOWS_H would work. I just did some more testing. According to http://sligt.wordpress.com/2011/03/05/compiling-boost-thread-using-mingw64/ people are recommended to define BOOST_USE_WINDOWS_H when building boost using mingw-w64 for the x64 target. With this specific define set the boost package can indeed be compiled without issues (for both the x86 and x64 targets). However, there's a catch! The boost build system doesn't embed this specific define in its installed headers. It expects that all boost-using projects (like qpid-cpp) also have this BOOST_USE_WINDOWS_H define set otherwise it will also cause build failures there. So I guess that changes to our package build scripts alone aren't enough and that code patches really are needed.. Regards, Erik van Pienbroek -- See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831iu=/4140/ostg.clktrk ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
