[Openmp-commits] [PATCH] Follow up to safe API patch
jonathan.l.peyton at intel.com
Thu May 21 13:16:14 PDT 2015
A while back, we made an initial change where dangerous C API functions were replaced with macros that translated the dangerous API function calls to safer function calls
e.g., sprintf() replaced with KMP_SPRINTF() which translates to sprintf_s() on Windows. Currently, the only operating system where this is applicable is Windows. Unix-like systems are still using the dangerous API e.g., KMP_SPRINTF() translates to sprintf(). Hal had brought up a couple concerns over performance:
> +// _malloca was suggested, but it is not a drop-in replacement for _alloca
> +# define KMP_ALLOCA _alloca
> 1) "Is the only difference the fact that the size is capped at _ALLOCA_S_THRESHOLD? Would you hit this limit? Are there performance implications to making the change regardless?"
> -#define ngo_store_go(dst, src) memcpy((dst), (src), CACHE_LINE)
> +#define ngo_store_go(dst, src) KMP_MEMCPY((dst), (src), CACHE_LINE)
> 2) "The other changes look fine, but this could have performance implications, no? The compiler can optimize away the memcpy, but likely not the memcpy_s, and the associated validation is likely expensive. Should we avoid this here?"
For 1) Yes, it hits the limit, and the runtime should not allocate memory from the heap. Currently performance is only affected on Windows because it is the only operating system using real safe API function calls.
For 2) That particular instance of memcpy only affects hierarchical barrier which is primarily used on Intel(R) MIC architectures. For memcpy() in general, it currently is only affected on Windows and will not affect performance for Unix users.
If users are worried about performance, a CMake options can be added which always maps the macros to the unsafe C function calls.
@@ -32,7 +32,7 @@
#define ngo_load(src) ((void)0)
#define ngo_store_icvs(dst, src) copy_icvs((dst), (src))
-#define ngo_store_go(dst, src) memcpy((dst), (src), CACHE_LINE)
+#define ngo_store_go(dst, src) KMP_MEMCPY((dst), (src), CACHE_LINE)
#define ngo_sync() ((void)0)
#endif /* KMP_MIC && USE_NGO_STORES */
@@ -23,9 +23,7 @@
# define RSIZE_MAX_STR ( 4UL << 10 ) // 4KB
// _malloca was suggested, but it is not a drop-in replacement for _alloca
-// TODO: test performance and replace with _alloca (as below)
-# define KMP_ALLOCA alloca
-//# define KMP_ALLOCA _alloca
+# define KMP_ALLOCA _alloca
# define KMP_MEMCPY_S memcpy_s
# define KMP_SNPRINTF sprintf_s
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 1128 bytes
Desc: not available
More information about the Openmp-commits