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

[Alexander Riccio via llvm-commits]

ariccio added inline comments.

================
Comment at: llvm/cmake/modules/HandleLLVMOptions.cmake:363
@@ -362,1 +362,3 @@
 
+  # /Zo (Enhance Optimized Debugging) was introduced with Visual Studio 
+  # 2013 update 3. Instead of only enabling them in VS2013 update 3, we'll just
----------------
aaron.ballman wrote:
> rnk wrote:
> > According to MSDN: "The /Zo option is enabled by default in Visual Studio 2015 when you specify debugging information with /Zi or /Z7." I think we can just leave this flag alone.
> Good catch! I agree.
The way that I (possibly incorrectly) interpreted this is that this means that `/Zo` is enabled when you select `/Zi` or `/Z7` in the UI. Should I try to test that hypothesis, or should I ask around?

================
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:
> 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.


http://reviews.llvm.org/D19519