Page MenuHomePhabricator

Follow up to safe API patch
ClosedPublic

Authored by jlpeyton on May 21 2015, 1:16 PM.

Details

Summary

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)

  1. "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.

Diff Detail

Repository
rL LLVM

Event Timeline

jlpeyton updated this revision to Diff 26265.May 21 2015, 1:16 PM
jlpeyton retitled this revision from to Follow up to safe API patch.
jlpeyton updated this object.
jlpeyton edited the test plan for this revision. (Show Details)
jlpeyton added a reviewer: hfinkel.
jlpeyton set the repository for this revision to rL LLVM.
jlpeyton added subscribers: Unknown Object (MLST), Unknown Object (MLST), tlwilmar.
AndreyChurbanov accepted this revision.Jul 3 2015, 4:47 AM
AndreyChurbanov added a reviewer: AndreyChurbanov.
AndreyChurbanov added a subscriber: AndreyChurbanov.

LGTM, as our testing showed no performance differences, the change looks safe.

This revision is now accepted and ready to land.Jul 3 2015, 4:47 AM
This revision was automatically updated to reflect the committed changes.