Enable "Optimized Debugging" and Enable "Control Flow Guard" in MSVC builds

[ariccio]

Summary

This patch turns on two of VS2015's new features: Optimized Debugging & Control Flow Guard.

Optimized Debugging makes it much easier to debug/profile optimized MSVC binaries, and it is a bit like [[ https://gcc.gnu.org/onlinedocs/gcc/Debugging-Options.html | GCC's -g3 option ]]. I've enabled it for all builds, so that we can enable some optimizations for debug builds in the future without worrying about debuggability.

Control Flow Guard is a security feature that hardens against memory corruption, has negligible performance impact, and is backwards compatible. Enabling it is like enabling DEP, ASLR, or /GS stack checking.

[aaron.ballman]

These seem generally reasonable to me; do you have any performance impact numbers for either of the flags?

[rnk]

llvm/cmake/modules/HandleLLVMOptions.cmake

363

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.

380–381

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.

[aaron.ballman]

llvm/cmake/modules/HandleLLVMOptions.cmake

363

Good catch! I agree.

380–381

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.)

[ariccio]

llvm/cmake/modules/HandleLLVMOptions.cmake

363

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?

380–381

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:

1. The compiler optimizes some calls out
2. All major browsers use CFG
3. All Windows system libraries are built with CFG
4. 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.

[aaron.ballman]

llvm/cmake/modules/HandleLLVMOptions.cmake

380–381

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)?

[rnk]

llvm/cmake/modules/HandleLLVMOptions.cmake

380–381

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.

[ariccio]

llvm/cmake/modules/HandleLLVMOptions.cmake

380–381

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).

I think this is an example of clang's version of CFG detecting incorrect code: https://bugs.chromium.org/p/chromium/issues/detail?id=538952

...It's not exactly the same, but it's the same basic idea.

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)?

How do I boostrap it on Windows? I wish there was a programmatic way to test performance regressions, kinda like chromium has.

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.

We may just have to agree to disagree with our philosophical differences here. I think it's a pretty bad idea to compare LLVM's hardening against a generic other program, because most programs have terrible security. I think that we'd be quite silly to ignore a zero-effort switch like this.

That said, I'd be ok dropping this patch if there isn't enough support for it.

[ariccio]

Total sidenote: here's some interesting reading on CFG's impact on Windows: http://www.alex-ionescu.com/?p=246

[bcraig]

I'm generally in favor of both changes.

For enhanced symbols, I would suggest measuring the size difference in the RelWithDebInfo pdb files before and after this change. That should tell you if explicitly setting the flag does anything. It would also be nice to get a build time impact, but that is much less deterministic.

I would prefer that CF Guard be turned on. If a malicious party sneaks in something on some repo on GitHub, I don't want them to own my machine after compiling it.

[aaron.ballman]

llvm/cmake/modules/HandleLLVMOptions.cmake

380–381

There's no special reason to harden the compiler more than any other application.

I strongly disagree with this assertion. For starters, not everyone thinks about security as much as they should, so if it costs us nothing in terms of performance to turn this flag on, we make a more secure product that can be more safely used everywhere (including on a server) *out of the box*. That's sufficient special reason to harden any application, including our suite of tools.

380–381

I think this is an example of clang's version of CFG detecting incorrect code: https://bugs.chromium.org/p/chromium/issues/detail?id=538952

...It's not exactly the same, but it's the same basic idea.

I will have to study this in more detail but at first glance, I'm not certain this has the same level of utility has /GS. However, that's not to say that this switch doesn't have utility.

How do I boostrap it on Windows? I wish there was a programmatic way to test performance regressions, kinda like chromium has.

Hmm, good point, I don't know if we can bootstrap from MSVC-built Clang. @rnk may know that answer better. If bootstrapping isn't possible, even just timing a large corpus of code (such as our test suite) may provide the answer we need. I know that such a test won't be highly accurate, but I'm not concerned about performance regressions that are in the noise, I'm more worried about performance regressions that are measurable and noticeable. So you could time it with Measure-Command in powershell, or download a utility similar to bash's "time" command to measure the performance of running the suite.

llvm/cmake/modules/HandleLLVMOptions.cmake

if( MSVC )
   # and up.
   if (NOT (MSVC_VERSION LESS 1900))
     # Disable string literal const->non-const type conversion.
     # "When specified, the compiler requires strict const-qualification
     # conformance for pointers initialized by using string literals."
     append("/Zc:strictStrings" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
   endif(NOT (MSVC_VERSION LESS 1900))
 
+  # /Zo (Enhance Optimized Debugging) was introduced with Visual Studio 
+  # 2013 update 3. Instead of only enabling them in VS2013 update 3, we'll just
+  # enable them for Visual Studio 2015 (VS 14, MSVC_VERSION 1900) and up.
+  if (NOT (MSVC_VERSION LESS 1900))
+    # "The /Zo compiler switch generates enhanced debugging information for
+    # optimized code. Optimization may use registers for local variables,
+    # reorder code, vectorize loops, and inline function calls. These
+    # optimizations can obscure the relationship between the source code and
+    # the compiled object code. The /Zo switch tells the compiler to generate
+    # additional debugging information for local variables and inlined
+    # functions. It also enables stack traces to show inlined functions in the
+    # WinDBG debugger.
+    # Debug builds that have disabled optimizations (/Od) do not need the
+    # additional debugging information generated when /Zo is specified."
+    append("/Zo" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+  endif(NOT (MSVC_VERSION LESS 1900))
+
+  # /guard (Enable Control Flow Guard) was introduced with Visual Studio 
+  # 2015.
+  if (NOT (MSVC_VERSION LESS 1900))
+    # "The /guard:cf option causes the compiler to analyze control flow for
+    # indirect call targets at compile time, and then to insert code to
+    # verify the targets at runtime."
+    append("/guard:cf" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+    # "The /guard:cf option must be passed to both the compiler and linker
+    # to build code that uses the CFG exploit mitigation technique."
+    append("/guard:cf"
+      CMAKE_EXE_LINKER_FLAGS
+      CMAKE_MODULE_LINKER_FLAGS
+      CMAKE_SHARED_LINKER_FLAGS)
+  endif(NOT (MSVC_VERSION LESS 1900))
+
   # "Generate Intrinsic Functions".
   append("/Oi" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 
   # "Enforce type conversion rules".
   append("/Zc:rvalueCast" CMAKE_CXX_FLAGS)
 
   if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
     # Find and run MSVC (not clang-cl) and get its version. This will tell