[PATCH] D19519: Enable "Optimized Debugging" and Enable "Control Flow Guard" in MSVC builds

[Reid Kleckner via llvm-commits]

rnk added inline comments.

================
Comment at: llvm/cmake/modules/HandleLLVMOptions.cmake:380-381
@@ +379,4 @@
+
+  # /guard (Enable Control Flow Guard) was introduced with Visual Studio 
+  # 2015.
+  if (NOT (MSVC_VERSION LESS 1900))
----------------
aaron.ballman wrote:
> ariccio wrote:
> > aaron.ballman wrote:
> > > rnk wrote:
> > > > I don't think we should enable this by default. Unless you are deploying clang online in a place like https://gcc.godbolt.org/, you should generally consider it to be exploitable and shouldn't run it on attacker controlled source code. Users like godbolt can enable this flag manually the old fashioned way with CMAKE_C_FLAGS.
> > > I don't disagree with this logic, but would also ask: if there's no impact to performance, is it harmful to enable? (That's why I was wondering about performance measurements.)
> > I actually //do// disagree with this logic. /GS stack checking is a similar hardening measure, but we always use it (despite the small perf loss) because code should //always// be hardened. I think of it like a seatbelt: I always wear a seatbelt; not just when I distrust the driver; even when I do trust the driver.
> > 
> > Performance wise, I have some indirect evidence of an only very small effect:
> > 
> > 
> >   # [[ https://blogs.msdn.microsoft.com/vcblog/2014/12/08/visual-studio-2015-preview-work-in-progress-security-feature/ | The compiler optimizes some calls out ]]
> >   # [[ http://blog.trendmicro.com/trendlabs-security-intelligence/windows-10s-new-browser-microsoft-edge-improved-but-also-new-risks/ | All major browsers use CFG ]]
> >   # [[ https://labs.bromium.com/2015/09/28/an-interesting-detail-about-control-flow-guard/ | All Windows system libraries are built with CFG ]]
> >   # The generated code isn't much different than `/GS` stack checking code, which inserts a call to `__security_check_cookie`, amd CFG calls `___guard_check_icall_fptr`.
> > 
> > 
> > ...if there's a negligible impact to performance, then there's no harm to be done. Like `/GS`, it also protects against some bugs that are not being exploited.
> The logic I don't disagree with is that this hardening is of far greater interest to scenarios where clang is being used on a server. The stack checking has some extra correctness benefits (such as alerting you to when you've stomped the stack) which is why I think it makes sense to always enable /GS regardless of its (negligible) performance impact. My understanding of /guard:cf is that this is *only* useful against purposeful hijacking of the binary and it will not give extra protection for incorrect code (unlike /GS). That's why I think it's good to discuss the actual impact on Clang and LLVM itself. If the impact is negligible, then I think we should enable it because it is a safety belt of sorts. However, if it has noticeable impact, I am less convinced of its utility for our general use case.
> 
> So to be more precise, before we enable this flag, I would like to understand how it impacts the performance of Clang and LLVM (I'm less interested in how it affects web browsers or the OS, though those make me hopeful that this has no real performance impact). Can you run some timing tests to see the comparison between a build with this flag set and one without it set, for instance, by bootstrapping the compiler (or building some other large code base)?
I think we should just do whatever the CMake default is. There's no special reason to harden the compiler more than any other application. Eventually, /GS appeared everywhere and MSVC made it the VS default. CMake followed suit. If and when that happens for CF guard, then we'll get it for free.


http://reviews.llvm.org/D19519