[1.35] System: error_code.cpp #defines macros without checking if already defined
Hi Beman, error_code.cpp contains the following #defines: #define _CRT_SECURE_NO_DEPRECATE #define _SCL_SECURE_NO_DEPRECATE It should first check that these macros are not defined already. Otherwise users who #define these macros on the command line will received duplicate definition errors, as I am. Also, you might consider adding: #define _SECURE_SCL 0 for VC 9. - Mat
error_code.cpp contains the following #defines:
#define _CRT_SECURE_NO_DEPRECATE #define _SCL_SECURE_NO_DEPRECATE
It should first check that these macros are not defined already. Otherwise users who #define these macros on the command line will received duplicate definition errors, as I am. Also, you might consider adding:
#define _SECURE_SCL 0
for VC 9.
I'ld rather have it somewhere in Boost.Config. Every library has to deal with these, so why not silence the warnings once and for all. Regards Hartmut
Hartmut Kaiser wrote:
error_code.cpp contains the following #defines:
#define _CRT_SECURE_NO_DEPRECATE #define _SCL_SECURE_NO_DEPRECATE
It should first check that these macros are not defined already. Otherwise users who #define these macros on the command line will received duplicate definition errors, as I am. Also, you might consider adding:
#define _SECURE_SCL 0
for VC 9.
I'ld rather have it somewhere in Boost.Config. Every library has to deal with these, so why not silence the warnings once and for all.
I thought we'd been through this: defining those in a header is *too late* to do any good, these global macros have to be set either by the build system, or at the top of a .cpp file *before* any headers are included, in the latter case there should also be a check that they weren't previously set on the command line. John.
John,
error_code.cpp contains the following #defines:
#define _CRT_SECURE_NO_DEPRECATE #define _SCL_SECURE_NO_DEPRECATE
It should first check that these macros are not defined already. Otherwise users who #define these macros on the command line will received duplicate definition errors, as I am. Also, you might consider adding:
#define _SECURE_SCL 0
for VC 9.
I'ld rather have it somewhere in Boost.Config. Every library has to deal with these, so why not silence the warnings once and for all.
I thought we'd been through this: defining those in a header is *too late* to do any good, these global macros have to be set either by the build system, or at the top of a .cpp file *before* any headers are included, in the latter case there should also be a check that they weren't previously set on the command line.
Sorry, I missed that discussion. Is the build system expected to support these anytime soon? Regards Hartmut
Mat Marcus wrote:
Also, you might consider adding:
#define _SECURE_SCL 0
for VC 9.
And make the users chase subtle ODR violations? Not very polite. http://forums.microsoft.com/MSDN/ShowPost.aspx?PostID=863481&SiteID=1
"Peter Dimov" <pdimov@pdimov.com> writes:
Mat Marcus wrote:
Also, you might consider adding:
#define _SECURE_SCL 0
for VC 9.
And make the users chase subtle ODR violations? Not very polite.
http://forums.microsoft.com/MSDN/ShowPost.aspx?PostID=863481&SiteID=1
Ouch, good point. I guess one has to consider _SECURE_SCL=0 as a global setting. Perhaps the boost build system might want to make this the default, given the past performance issues of MS checked iterators, etc. even in release builds. (I haven't measured the performance imapct for vc9 yet, but I did notice that it is not a debug-only switch). - Mat
Mat wrote:
"Peter Dimov" <pdimov@pdimov.com> writes:
Mat Marcus wrote:
Also, you might consider adding:
#define _SECURE_SCL 0
for VC 9.
And make the users chase subtle ODR violations? Not very polite.
http://forums.microsoft.com/MSDN/ShowPost.aspx?PostID=863481&SiteID=1
Ouch, good point. I guess one has to consider _SECURE_SCL=0 as a global setting. Perhaps the boost build system might want to make this the default, given the past performance issues of MS checked iterators, etc. even in release builds. (I haven't measured the performance imapct for vc9 yet, but I did notice that it is not a debug-only switch).
- Mat
In additions to John Maddock's concerns about whether disabling the macro would even work in practice, I think it would be (kind of) rude to even try. Suppose somebody actually wants to deprecate code and warning 4995 is disabled? Or supposed somebody actually wants to use these "checked iterators", or does not want to use boost functions that access this code. I think boost should respect these deprecations as a valid way for the user to configure things, boost headers should not muck with them. I think it is kind of rude that the vendor's stl deprecates standard function *by default*, but in order to preserve the *option* to deprecate them I think boost should just let it be. In the boost build files they should be defined, as John Maddock said. In user code those of us who choose msvc should just deal. (I put them at the top of the precompiled header in vc8) -- John Femiani
Mat Marcus:
Ouch, good point. I guess one has to consider _SECURE_SCL=0 as a global setting. Perhaps the boost build system might want to make this the default, given the past performance issues of MS checked iterators, etc. even in release builds. (I haven't measured the performance imapct for vc9 yet, but I did notice that it is not a debug-only switch).
I've found _SECURE_SCL less evil than claimed. It does decrease performance, but in my experience, it does so in situations where the code is already suboptimal (repeated calls to vector<>::operator[] and use of vector<T>::iterator instead of T*). On the plus side, it does catch bugs. On balance, I don't believe that we should override the default, although we probably should provide a way for the user to set it to 0 when building Boost.
I've found _SECURE_SCL less evil than claimed. It does decrease performance, but in my experience, it does so in situations where the code is already suboptimal (repeated calls to vector<>::operator[] and use of vector<T>::iterator instead of T*). On the plus side, it does catch bugs.
On balance, I don't believe that we should override the default, although we probably should provide a way for the user to set it to 0 when building Boost.
I'm surprised to hear you say this, as my experience is so different. To see whether this might be because things changed with in VC 9, I ran one of our benchmark suites to measure the _SECURE_SCL penalty for release builds. The benchmarks attempt, amongst other things, to measure the abstraction penalty by comparing the performance of different algorithms when using a vendor's vector with associated iterator type vs. an array with pointers. When reading the results below, double pointer means an array of doubles (pointer iterators), where double vector means a vector of doubles (vector<T>::iterator). Note that sometimes the penalty is as much as an order of magnitude. This strengthens my view that _SECURE_SCL for release builds as a misfeature, and I still am of the opinion that, at the least, boost build should disable 'secure' STL for release variations in the default case. (I won't go into detail with the source code for the test or the exact build settings at this point in the discussion. If you are interested in the source for these tests, let me know and I'll see whether they are ready for public release.) - Mat ### ### release build with _SECURE_SCL enabled ### c:\boxer\performance\projects\msvc8\Release\stepanov_vector.exe test description absolute operations ratio with number time per second test0 0 "double pointer verify2" 3.03 sec 989.77 M 1.00 1 "DoubleClass pointer verify2" 3.05 sec 984.57 M 1.01 2 "double vector" 16.14 sec 185.86 M 5.33 3 "DoubleClass vector" 18.19 sec 164.95 M 6.00 4 "double vector reverse" 26.23 sec 114.35 M 8.66 5 "DoubleClass vector reverse" 26.20 sec 114.49 M 8.65 6 "double vector reverse reverse" 31.25 sec 96.00 M 10.31 7 "DoubleClass vector reverse reverse" 31.25 sec 96.00 M 10.31 Total absolute time for Vector accumulate: 155.34 sec Vector accumulate Penalty: 5.92 test description absolute operations ratio with number time per second test0 0 "insertion_sort double pointer verify2" 1.16 sec 2.60 M 1.00 1 "insertion_sort DoubleClass pointer verify2" 1.58 sec 1.90 M 1.37 2 "insertion_sort double vector" 19.58 sec 0.15 M 16.94 3 "insertion_sort DoubleClass vector" 18.00 sec 0.17 M 15.57 4 "insertion_sort double vector reverse" 30.56 sec 0.10 M 26.44 5 "insertion_sort DoubleClass vector reverse" 29.06 sec 0.10 M 25.14 6 "insertion_sort double vector reverse reverse" 43.03 sec 0.07 M 37.22 7 "insertion_sort DoubleClass vector reverse reverse" 43.05 sec 0.07 M 37.24 Total absolute time for Vector Insertion Sort: 186.02 sec Vector Insertion Sort Penalty: 16.49 test description absolute operations rati o with number time per second test 0 0 "quicksort double pointer verify2" 1.64 sec 14.63 M 1.00 1 "quicksort DoubleClass pointer verify2" 1.95 sec 12.29 M 1.19 2 "quicksort double vector" 5.03 sec 4.77 M 3.07 3 "quicksort DoubleClass vector" 5.22 sec 4.60 M 3.18 4 "quicksort double vector reverse" 7.16 sec 3.35 M 4.36 5 "quicksort DoubleClass vector reverse" 7.34 sec 3.27 M 4.48 6 "quicksort double vector reverse reverse" 8.84 sec 2.71 M 5.39 7 "quicksort DoubleClass vector reverse reverse" 9.06 sec 2.65 M 5.52 Total absolute time for Vector Quicksort: 46.25 sec Vector Quicksort Penalty: 3.52 test description absolute operations rati o with number time per second test 0 0 "heap_sort double pointer verify2" 1.81 sec 13.24 M 1.00 1 "heap_sort DoubleClass pointer verify2" 1.88 sec 12.80 M 1.03 2 "heap_sort double vector" 7.84 sec 3.06 M 4.33 3 "heap_sort DoubleClass vector" 7.89 sec 3.04 M 4.35 4 "heap_sort double vector reverse" 10.80 sec 2.22 M 5.96 5 "heap_sort DoubleClass vector reverse" 10.31 sec 2.33 M 5.69 6 "heap_sort double vector reverse reverse" 13.23 sec 1.81 M 7.30 7 "heap_sort DoubleClass vector reverse reverse" 13.22 sec 1.82 M 7.29 Total absolute time for Vector Heap Sort: 66.98 sec Vector Heap Sort Penalty: 4.46 ### ### release build with _SECURE_SCL disabled ### c:\boxer\performance\projects\msvc8\Release\stepanov_vector.exe test description absolute operations ratio with number time per second test0 0 "double pointer verify2" 3.03 sec 989.45 M 1.00 1 "DoubleClass pointer verify2" 3.05 sec 984.57 M 1.00 2 "double vector" 3.06 sec 979.75 M 1.01 3 "DoubleClass vector" 3.05 sec 984.57 M 1.00 4 "double vector reverse" 3.06 sec 979.75 M 1.01 5 "DoubleClass vector reverse" 3.05 sec 984.57 M 1.00 6 "double vector reverse reverse" 3.09 sec 969.62 M 1.02 7 "DoubleClass vector reverse reverse" 3.06 sec 979.43 M 1.01 Total absolute time for Vector accumulate: 24.45 sec Vector accumulate Penalty: 1.01 test description absolute operations ratio with number time per second test0 0 "insertion_sort double pointer verify2" 1.16 sec 2.60 M 1.00 1 "insertion_sort DoubleClass pointer verify2" 1.58 sec 1.90 M 1.37 2 "insertion_sort double vector" 1.58 sec 1.90 M 1.37 3 "insertion_sort DoubleClass vector" 2.08 sec 1.44 M 1.80 4 "insertion_sort double vector reverse" 1.55 sec 1.94 M 1.34 5 "insertion_sort DoubleClass vector reverse" 2.03 sec 1.48 M 1.76 6 "insertion_sort double vector reverse reverse" 1.59 sec 1.88 M 1.38 7 "insertion_sort DoubleClass vector reverse reverse" 2.09 sec 1.43 M 1.81 Total absolute time for Vector Insertion Sort: 13.66 sec Vector Insertion Sort Penalty: 1.53 test description absolute operations rati o with number time per second test 0 0 "quicksort double pointer verify2" 1.75 sec 13.71 M 1.00 1 "quicksort DoubleClass pointer verify2" 2.06 sec 11.64 M 1.18 2 "quicksort double vector" 1.80 sec 13.36 M 1.03 3 "quicksort DoubleClass vector" 2.14 sec 11.21 M 1.22 4 "quicksort double vector reverse" 1.78 sec 13.48 M 1.02 5 "quicksort DoubleClass vector reverse" 2.13 sec 11.29 M 1.21 6 "quicksort double vector reverse reverse" 1.78 sec 13.48 M 1.02 7 "quicksort DoubleClass vector reverse reverse" 2.16 sec 11.13 M 1.23 Total absolute time for Vector Quicksort: 15.59 sec Vector Quicksort Penalty: 1.13 test description absolute operations rati o with number time per second test 0 0 "heap_sort double pointer verify2" 1.80 sec 13.36 M 1.00 1 "heap_sort DoubleClass pointer verify2" 1.83 sec 13.12 M 1.02 2 "heap_sort double vector" 1.88 sec 12.80 M 1.04 3 "heap_sort DoubleClass vector" 2.05 sec 11.73 M 1.14 4 "heap_sort double vector reverse" 2.14 sec 11.21 M 1.19 5 "heap_sort DoubleClass vector reverse" 2.28 sec 10.52 M 1.27 6 "heap_sort double vector reverse reverse" 1.88 sec 12.80 M 1.04 7 "heap_sort DoubleClass vector reverse reverse" 2.03 sec 11.81 M 1.13 Total absolute time for Vector Heap Sort: 15.88 sec Vector Heap Sort Penalty: 1.12
Mat Marcus: ...
I'm surprised to hear you say this, as my experience is so different. To see whether this might be because things changed with in VC 9, I ran one of our benchmark suites to measure the _SECURE_SCL penalty for release builds.
A compiler that is good at removing the iterator abstraction penalty will show a bigger difference between _SECURE_SCL=0 and =1, so your results aren't surprising. I wasn't disputing that. My point was that if one habitually uses pointers to traverse performance-sensitive vectors, then _SECURE_SCL only affects the non-performance-sensitive vector accesses, where one presumably doesn't mind the extra safety. (One reason for avoiding vector::iterator is fear of abstraction penalty that may now be obsolete and unfounded; another legitimate one, however, is that pointers imply contiguous storage, and algorithms can take advantage of that.)
This strengthens my view that _SECURE_SCL for release builds as a misfeature, and I still am of the opinion that, at the least, boost build should disable 'secure' STL for release variations in the default case.
If we do that, we'll probably have to encode the _SECURE_SCL setting in the .lib names, to avoid mismatches.
On Sat, Apr 5, 2008 at 5:53 PM, Peter Dimov <pdimov@pdimov.com> wrote:
Mat Marcus: [snip]
This strengthens my view that _SECURE_SCL for release builds as a misfeature, and I still am of the opinion that, at the least, boost build should disable 'secure' STL for release variations in the default case.
If we do that, we'll probably have to encode the _SECURE_SCL setting in the .lib names, to avoid mismatches.
Yes, agreed. I was envisioning a link-incompatible boost-build feature, e.g. <safe-stl-misfeature>on/off :-) which defaulted to off for release builds. - Mat
On Sat, Apr 5, 2008 at 8:53 PM, Peter Dimov <pdimov@pdimov.com> wrote:
A compiler that is good at removing the iterator abstraction penalty will show a bigger difference between _SECURE_SCL=0 and =1, so your results aren't surprising. I wasn't disputing that. My point was that if one habitually uses pointers to traverse performance-sensitive vectors, then _SECURE_SCL only affects the non-performance-sensitive vector accesses, where one presumably doesn't mind the extra safety.
(One reason for avoiding vector::iterator is fear of abstraction penalty that may now be obsolete and unfounded; another legitimate one, however, is that pointers imply contiguous storage, and algorithms can take advantage of that.)
I really dislike this. It's been hard enough to convince people that yes, templates are just as fast as doing it manually, so anything that prevents that from being true is, in my opinion, a misfeature --- and contrary to the "spirit" of C++, according to my reading of D&E and the HOPL paper. And vector iterators also imply contiguous storage. Any good implementation ought to specialize for that if they use sequential-memory optimizations. As far as I'm concerned, the implementation shouldn't be doing anything to hurt my performance by default if I have NDEBUG defined. If I want my vectors and such checked, I'll ask for a debug STL. The OS will already prevent me from screwing up some other program. YMMV, ~ Scott
Mat Marcus schrieb:
I'm surprised to hear you say this, as my experience is so different. To see whether this might be because things changed with in VC 9, I ran one of our benchmark suites to measure the _SECURE_SCL penalty for release builds. The benchmarks attempt, amongst other things, to measure the abstraction penalty by comparing the performance of different algorithms when using a vendor's vector with associated iterator type vs. an array with pointers.
Weird, tested the following snippet: double* cdata = new double [100000]; for (int i = 0; i < 100000; ++i) cdata [i] = std::rand () / static_cast<float> (RAND_MAX); std::vector<double> data (cdata, cdata + 100000); Timer t; std::sort (cdata, cdata + 100000); std::cout << t.getElapsedTime () << std::endl; t.reset (); std::sort (data.begin (), data.end ()); std::cout << t.getElapsedTime () << std::endl; delete [] cdata; with Timer being a high-resolution timer on Windows. Results: Debug/x64 0.0856235 2.25998 Release/x64 0.0164568 0.0163389 <-- The std::vector version is *faster* For std::accumulate, its Release/x64 0.000354514 0.000479391 in Release (ok, 50% slower but no way an order of magnitude) and Debug/x64 0.000603987 0.0168896 in Debug. Does not sound too bad to me actually? Not sure, maybe the x64 exception handling is just vastly better for non-thrown exceptions. I'm keeping it on, just in case, and I hope Boost won't override the compiler default. Cheers, Anteru
On Fri, Apr 11, 2008 at 7:11 AM, Anteru <newsgroups@catchall.shelter13.net> wrote:
Mat Marcus schrieb:
I'm surprised to hear you say this, as my experience is so different. To see whether this might be because things changed with in VC 9, I ran one of our benchmark suites to measure the _SECURE_SCL penalty for release builds. The benchmarks attempt, amongst other things, to measure the abstraction penalty by comparing the performance of different algorithms when using a vendor's vector with associated iterator type vs. an array with pointers.
Weird, tested the following snippet: [snip]
Does not sound too bad to me actually? Not sure, maybe the x64 exception handling is just vastly better for non-thrown exceptions. I'm keeping it on, just in case, and I hope Boost won't override the compiler default.
Cheers, Anteru
For my reply, I have received permission to post a pre-release version of some code from a colleague's benchmark suite. This should indicate how to reproduce my results. I will begin a new thread since the current subject line doesn't really reflect the topic under discussion. I will begin the threas: High Performance Cost of MS 'secure' STL for Release Builds. - Mat
Crossposting to Boost.Build...
error_code.cpp contains the following #defines:
#define _CRT_SECURE_NO_DEPRECATE #define _SCL_SECURE_NO_DEPRECATE
It should first check that these macros are not defined already. Otherwise users who #define these macros on the command line will received duplicate definition errors, as I am. Also, you might consider adding:
#define _SECURE_SCL 0
for VC 9.
I'ld rather have it somewhere in Boost.Config. Every library has to deal with these, so why not silence the warnings once and for all.
In fact I'ld suggest to add: // // silence 'secure' and 'deprecate' warnings // #if (_MSC_VER >= 1400) && !defined(BOOST_ENABLE_DEPRECATE_WARNINGS) #define _CRT_SECURE_NO_DEPRECATE #define _SCL_SECURE_NO_DEPRECATE #endif #if (_MSC_VER >= 1400) && !defined(BOOST_ENABLE_SECURE_WARNINGS) #define _CRT_SECURE_NO_WARNINGS #define _SCL_SECURE_NO_WARNINGS #endif to the file boost/config/compiler/visual.hpp, which should solve the issue for all libraries. I'm not sure about the _SECURE_SCL for VC9 (I don't use this compiler). Any comments? Regards Hartmut
"Hartmut Kaiser" <hartmut.kaiser@gmail.com> writes:
Crossposting to Boost.Build...
error_code.cpp contains the following #defines:
#define _CRT_SECURE_NO_DEPRECATE #define _SCL_SECURE_NO_DEPRECATE
It should first check that these macros are not defined already. Otherwise users who #define these macros on the command line will received duplicate definition errors, as I am. Also, you might consider adding:
#define _SECURE_SCL 0
for VC 9.
I'ld rather have it somewhere in Boost.Config. Every library has to deal with these, so why not silence the warnings once and for all.
In fact I'ld suggest to add:
// // silence 'secure' and 'deprecate' warnings // #if (_MSC_VER >= 1400) && !defined(BOOST_ENABLE_DEPRECATE_WARNINGS) #define _CRT_SECURE_NO_DEPRECATE #define _SCL_SECURE_NO_DEPRECATE #endif
#if (_MSC_VER >= 1400) && !defined(BOOST_ENABLE_SECURE_WARNINGS) #define _CRT_SECURE_NO_WARNINGS #define _SCL_SECURE_NO_WARNINGS #endif [snip]
Wouldn't #define _CRT_SECURE_NO_DEPRECATE 1 etc., be better than #define _CRT_SECURE_NO_DEPRECATE so as not to clash with users who define_CRT_SECURE_NO_DEPRECATE on the command line?
Mat Marcus wrote:
Hi Beman,
error_code.cpp contains the following #defines:
#define _CRT_SECURE_NO_DEPRECATE #define _SCL_SECURE_NO_DEPRECATE
It should first check that these macros are not defined already. Otherwise users who #define these macros on the command line will received duplicate definition errors, as I am. Also, you might consider adding:
#define _SECURE_SCL 0
for VC 9.
Leaving aside the _SECURE_SCL 0 question, I think we need a boost header file that wraps up the needed code. This header should not be included in Boost headers, since doing so gets in user's way. But it would be useful in library sources like error_code.cpp and test programs. Building on Hartmut's suggestion, the header might look like this: // // silence 'secure' and 'deprecate' warnings // #if (_MSC_VER >= 1400) && !defined(BOOST_ENABLE_DEPRECATE_WARNINGS) # ifndef _CRT_SECURE_NO_DEPRECATE # define _CRT_SECURE_NO_DEPRECATE # endif # ifndef _SCL_SECURE_NO_DEPRECATE # define _SCL_SECURE_NO_DEPRECATE # endif #endif #if (_MSC_VER >= 1400) && !defined(BOOST_ENABLE_SECURE_WARNINGS) # ifndef _CRT_SECURE_NO_WARNINGS # define _CRT_SECURE_NO_WARNINGS # endif # ifndef _SCL_SECURE_NO_WARNINGS # define _SCL_SECURE_NO_WARNINGS # endif #endif Perhaps name it <boost/config/silence_misfeatures.hpp> or similar. I don't want a Microsoft specific name since it might be useful later for other compiler misfeatures. I'd add a comment explaining not to use it in user-visible code like boost/ headers. --Beman
Beman Dawes wrote:
Leaving aside the _SECURE_SCL 0 question, I think we need a boost header file that wraps up the needed code. This header should not be included in Boost headers, since doing so gets in user's way. But it would be useful in library sources like error_code.cpp and test programs.
Building on Hartmut's suggestion, the header might look like this:
// // silence 'secure' and 'deprecate' warnings // #if (_MSC_VER >= 1400) && !defined(BOOST_ENABLE_DEPRECATE_WARNINGS) # ifndef _CRT_SECURE_NO_DEPRECATE # define _CRT_SECURE_NO_DEPRECATE # endif # ifndef _SCL_SECURE_NO_DEPRECATE # define _SCL_SECURE_NO_DEPRECATE # endif #endif
#if (_MSC_VER >= 1400) && !defined(BOOST_ENABLE_SECURE_WARNINGS) # ifndef _CRT_SECURE_NO_WARNINGS # define _CRT_SECURE_NO_WARNINGS # endif # ifndef _SCL_SECURE_NO_WARNINGS # define _SCL_SECURE_NO_WARNINGS # endif #endif
Perhaps name it <boost/config/silence_misfeatures.hpp> or similar. I don't want a Microsoft specific name since it might be useful later for other compiler misfeatures.
I'd add a comment explaining not to use it in user-visible code like boost/ headers.
Unfortunately - and as already noted by Peter Dimov - defining these macros has an impact on the ABI comparibity of the code, so I don't believe we can do that :-( We might be able to use #pragma's to just turn the warnings unconditionally off though - so in effect the header would have the same effect. Let me experiment with that, John.
participants (8)
-
Anteru -
Beman Dawes -
Hartmut Kaiser -
John Femiani -
John Maddock -
Mat Marcus -
Peter Dimov -
Scott McMurray