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
Le 06/03/2010 17:32, José Fonseca a écrit :> > > 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) > > > 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. >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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100306/85219dcc/attachment.html>
Whoops, mailing list headers still broken, sending to the list this time: On Sat, Mar 6, 2010 at 3:35 PM, OvermindDL1 <overminddl1 at gmail.com> wrote:> On Sat, Mar 6, 2010 at 11:40 AM, Cédric Venet <cedric.venet at laposte.net> wrote: >> 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)). > > I use this for every bit of my code: > /D_CRT_NONSTDC_NO_DEPRECATE /D_CRT_SECURE_NO_DEPRECATE > /D_CRT_SECURE_NO_WARNINGS /D_SECURE_SCL=0 /D_SCL_SECURE_NO_DEPRECATE > /D_HAS_ITERATOR_DEBUGGING=0 > > In one particular class in one of my libraries, the speed difference > is the difference of it running in about 10ms, compared to it running > in about 12 minutes, and yes that is accurate. That is a lot greater > then just "10x". Whoever was the retard at MS that decided to design > such things, they deserve a quick kick to the privates... I dealt > with that for a freaking week... >
On 03/06/10 17:37, OvermindDL1 wrote:> Whoops, mailing list headers still broken, sending to the list this time: > > On Sat, Mar 6, 2010 at 3:35 PM, OvermindDL1<overminddl1 at gmail.com> wrote: >> On Sat, Mar 6, 2010 at 11:40 AM, Cédric Venet<cedric.venet at laposte.net> wrote: >>> 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)). >> >> I use this for every bit of my code: >> /D_CRT_NONSTDC_NO_DEPRECATE /D_CRT_SECURE_NO_DEPRECATE >> /D_CRT_SECURE_NO_WARNINGS /D_SECURE_SCL=0 /D_SCL_SECURE_NO_DEPRECATE >> /D_HAS_ITERATOR_DEBUGGING=0 >> >> In one particular class in one of my libraries, the speed difference >> is the difference of it running in about 10ms, compared to it running >> in about 12 minutes, and yes that is accurate. That is a lot greater >> then just "10x".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 :) -Isaac
On Sat, Mar 6, 2010 at 4:08 PM, Isaac Dupree <ml at isaac.cedarswampstudios.org> wrote:> On 03/06/10 18:03, OvermindDL1 wrote: >> >> On Sat, Mar 6, 2010 at 3:57 PM, Isaac Dupree >> <ml at isaac.cedarswampstudios.org> wrote: >>> >>> On 03/06/10 17:37, OvermindDL1 wrote: >>>> >>>> Whoops, mailing list headers still broken, sending to the list this >>>> time: >>>> >>>> On Sat, Mar 6, 2010 at 3:35 PM, OvermindDL1<overminddl1 at gmail.com> >>>> wrote: >>>>> >>>>> On Sat, Mar 6, 2010 at 11:40 AM, Cédric Venet<cedric.venet at laposte.net> >>>>> wrote: >>>>>> >>>>>> 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)). >>>>> >>>>> I use this for every bit of my code: >>>>> /D_CRT_NONSTDC_NO_DEPRECATE /D_CRT_SECURE_NO_DEPRECATE >>>>> /D_CRT_SECURE_NO_WARNINGS /D_SECURE_SCL=0 /D_SCL_SECURE_NO_DEPRECATE >>>>> /D_HAS_ITERATOR_DEBUGGING=0 >>>>> >>>>> In one particular class in one of my libraries, the speed difference >>>>> is the difference of it running in about 10ms, compared to it running >>>>> in about 12 minutes, and yes that is accurate. That is a lot greater >>>>> then just "10x". >>> >>> 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 :) >> >> Correct, as stated, whoever at MS designed that code needs a swift >> kick to their family jewels. ;-) >> >> Admittedly, it was a lot of template code, and most of it optimized >> out and inlined beautifully, once those above definitions were made, >> without them *nothing* inlined, and it ran slower then all freaking >> heck. > > (remember to send to the list! I won't mind the duplicate)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. :)