[SanitizerCoverage] Prevent /OPT:REF from stripping constructors

[metzman]

Summary

Linking with the /OPT:REF linker flag when building COFF files causes
the linker to strip SanitizerCoverage's constructors. Prevent this by
giving the constructors WeakODR linkage and by passing the linker a
directive to include sancov.module_ctor.

Include a test in compiler-rt to verify libFuzzer can be linked using
/OPT:REF

[metzman]

• Improve comments.
• Add test to compiler-rt

[metzman]

• fix spacing

[metzman]

@morehouse @rnk
Please take a look.
This patch allows libFuzzer targets to be compiled with /OPT:REF and adds a test to ensure this.

[morehouse]

Change is fine with me since Linux behavior is unchanged. @rnk can comment on whether this is the best approach for Windows or not.

[metzman]

@rnk what do you think of this?
This change does cause many constructor calls to happen (thousands of times for some fuzzers) but there doesn't seem to be a problem with this as libFuzzer ignores the redundant calls.
Also, I'm not worried about a performance penalty from the redundant calls since they are cheap and only happen on program startup.

[rnk]

I think we need to back up and think about what we're doing where and why.

If the application were fully statically linked, we wouldn't need to do anything in instrumentation code at all, we could just reference __start_*/__stop_* directly from the libfuzzer runtime, and that would contain all of sanitizer coverage data. The reason we have these module constructor calls is to deal with the case where the application is composed of multiple DSOs/DLLs. Each DSO will contain its own guards, counters, and pc arrays, and we can't arrange for them all to be contiguous.

To deal with this, during instrumentation, we inject an initializer (these sancov.module_ctor functions) into every module compiled with coverage instrumentation. We do it instrumentation and not in a compiler-rt library, because we only link the fuzzer RTL into the main executable (I think). By adding this initializer in instrumentation, we can discover every .SCOV table in the whole application.

We only need one registration call per DSO, so the existing code uses comdats to deduplicate them. However, we are using comdats in a non-portable way. We give the symbol internal linkage, and then put it in a comdat group. For that to really be meaningful on COFF, the function needs to have some externally visible linkage, like weak_odr or linkonce_odr. I remember that doesn't work, but can you remind me why? It would require adding /include:sancov.module_ctor (and maybe renaming it to remove the .) to ensure it doesn't get stripped.

But, our whole fancy instrumentation-based comdat-deduplicating registration thing doesn't work because we only provide start_sancov_guards etc in the libfuzzer runtime. We can probably fix that by emitting the __start/__stop symbols in the instrumentation pass, but that starts to feel like a lot of complexity.

If we only care about statically linked apps, I think it would be much simpler to bail out of SanitizerCoverageModule::CreateInitCallsForSections early on COFF targets, and put an initializer in libfuzzer that calls __sanitizer_cov_trace_pc_guard_init(&__start___sancov_guards, ...). Then we don't need to worry about /OPT:REF, comdats, and we can remove the COFF-only logic to add sizeof(uint64_t) __start symbols.

Basically, move the complexity to the runtime, and then we have looser coupling between instrumentation and runtime. The runtime will know the __start symbol is a uint64_t because that's where we define it.

Does that make sense?

[metzman]

In D52119#1237797, @rnk wrote:

I think we need to back up and think about what we're doing where and why.

If the application were fully statically linked, we wouldn't need to do anything in instrumentation code at all, we could just reference __start_*/__stop_* directly from the libfuzzer runtime, and that would contain all of sanitizer coverage data. The reason we have these module constructor calls is to deal with the case where the application is composed of multiple DSOs/DLLs. Each DSO will contain its own guards, counters, and pc arrays, and we can't arrange for them all to be contiguous.

Thank you for pointing this out. I didn't think of the benefits for dynamic linking.

To deal with this, during instrumentation, we inject an initializer (these sancov.module_ctor functions) into every module compiled with coverage instrumentation. We do it instrumentation and not in a compiler-rt library, because we only link the fuzzer RTL into the main executable (I think). By adding this initializer in instrumentation, we can discover every .SCOV table in the whole application.

I'm not sure the part about "the main executable" is correct.
I think we link the fuzzer RTL against everything.
Isn't this is how the __sanitizer_coverage_trace_* functions (such as [[ https://cs.chromium.org/chromium/src/third_party/libFuzzer/src/FuzzerTracePC.cpp?q=__sanitizer&sq=package:chromium&g=0&l=473 | __sanitizer_cov_trace_pc_guard ]]) which are defined in the fuzzer RTL, are used in all DSO/modules?
I also think this is why -fsanitize=fuzzer-no-link (which I think is confusingly/incorrectly named, since even when using this, we link against the fuzzer RTL, just not main) exists, without it the linker would be upset because 2 main functions exist, one defined by libFuzzer and one by the application.

We only need one registration call per DSO, so the existing code uses comdats to deduplicate them. However, we are using comdats in a non-portable way. We give the symbol internal linkage, and then put it in a comdat group. For that to really be meaningful on COFF, the function needs to have some externally visible linkage, like weak_odr or linkonce_odr. I remember that doesn't work, but can you remind me why? It would require adding /include:sancov.module_ctor (and maybe renaming it to remove the .) to ensure it doesn't get stripped.

I tried changing the linkage to every other possible value, but each time I compiled a target with /OPT:REF the constructors didn't execute.
I did also separately try adding a #pragma to include sancov.module_ctor in sanitizer_coverage_win_sections.cc but this caused a link failure.

If I recall correctly, I think manually adding !{!"/include:sancov.module_ctor"} to llvm.linker.options in the LLVM IR file of the target I was compiling worked. I assume that injecting this in SanitizerCoverage.cpp would work too.

But, our whole fancy instrumentation-based comdat-deduplicating registration thing doesn't work because we only provide start_sancov_guards etc in the libfuzzer runtime. We can probably fix that by emitting the __start/__stop symbols in the instrumentation pass, but that starts to feel like a lot of complexity.

Based on my possibly wrong understanding, I don't think we can emit these symbols in the pass. I think by having the linker define __start_* and __stop_* we don't need to keep PCs in the PC table for functions that have been stripped by the linker (ie: when the linker strips a function, it removes the elements associated with the function from the PC table and counter array).

If we only care about statically linked apps, I think it would be much simpler to bail out of SanitizerCoverageModule::CreateInitCallsForSections early on COFF targets, and put an initializer in libfuzzer that calls __sanitizer_cov_trace_pc_guard_init(&__start___sancov_guards, ...). Then we don't need to worry about /OPT:REF, comdats, and we can remove the COFF-only logic to add sizeof(uint64_t) __start symbols.

Now that I think about it, I probably do want to support for non-statically linked apps in the future (I think even in static chrome builds some libraries, such as libEGL which provides WebGL, are loaded dynamically).
I think it is fine to ignore them for now, but I'm not sure we should use solutions that will never work for them.

Basically, move the complexity to the runtime, and then we have looser coupling between instrumentation and runtime. The runtime will know the __start symbol is a uint64_t because that's where we define it.

Does that make sense?

I think I understand what you want me to do (eliminate constructors for msvc, emit the symbols in the pass, and then reference them directly from libFuzzer), but I think there are some alternatives that still allow the symbols to be linker defined and don't break dynamic linking further:

1. My current solution
2. A solution that allows deduplication but still forces the linker to not strip all of the constructor, possibly as my solution with llvm.linker.options (which I'm actually not sure would allow duplication even if it does make the constructor comdat).

Do you prefer one or the other (or is my understanding wrong and I should make your suggested change of moving the complexity to the runtime)?

[rnk]

In D52119#1242076, @metzman wrote:

I think I understand what you want me to do (eliminate constructors for msvc, emit the symbols in the pass, and then reference them directly from libFuzzer), but I think there are some alternatives that still allow the symbols to be linker defined and don't break dynamic linking further:

1. My current solution
2. A solution that allows deduplication but still forces the linker to not strip all of the constructor, possibly as my solution with llvm.linker.options (which I'm actually not sure would allow duplication even if it does make the constructor comdat).

Do you prefer one or the other (or is my understanding wrong and I should make your suggested change of moving the complexity to the runtime)?

I think we should aim for 2. Adding /include: to llvm.linker.options should work if we also change the linkage of sancov.module_ctor to anything other than internal. The most reasonable linkage is probably weak_odr.

My concern with the first solution, making the constructor not comdat, is that it's going to slow down process startup, which mattered enough on ELF to make it worth doing the constructor deduplication.

If we are linking the sanitizer runtime into every DLL that uses sanitizer coverage, then we could do it in the runtime, but it would diverge significantly from the Linux implementation. If we could make 2 work it's probably best.

[metzman]

• Improve comment.
• Add test to compiler-rt
• improve comment
• fix spacing
• Use COMDAT but force include of constructors and give them weak ODR linking

[metzman]

• clang-format

[metzman]

• clang-format

[metzman]

@rnk
Please take another look.

I think we should aim for 2. Adding /include: to llvm.linker.options should work if we also change the linkage of sancov.module_ctor to anything other than internal. The most reasonable linkage is probably weak_odr.

I've changed the code to go with "2". You were right that the linkage needed to change for this to work so I changed it to WeakODR

My concern with the first solution, making the constructor not comdat, is that it's going to slow down process startup, which mattered enough on ELF to make it worth doing the constructor deduplication.

I don't think this is a huge concern (I think startup difference wasn't noticeable to human), but I think my current solution does not have this issue even though the previous one did. I confirmed using print statements that there is only one call to the sancov.module_ctor in targets that had thousands with the previous solution.

If we are linking the sanitizer runtime into every DLL that uses sanitizer coverage, then we could do it in the runtime, but it would diverge significantly from the Linux implementation. If we could make 2 work it's probably best.

I don't actually know what affect this has on dynamic linking, since we are only using static in Chrome right now. I definitely want to support it soon so that others can use LibFuzzer on Windows, but it's certainly a lower priority than getting static builds working perfectly.
I observed the problems with OPT:REF (that this patch fixes) in static builds.

[rnk]

Thanks! I think this will work well.

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

309

Unfortunately, I think the names passed to /include: on 32-bit need to be underscore prefixed. I'd use this code to deal with it:

SmallString<20> IncDirective = "/include:";
Mangler().getNameWithPrefix(PrefixedName, CtorFunc, true);
...MDString::get(*C, IncDirective)..

[metzman]

• Add support for i386.

[metzman]

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

309

I believe I have fixed this, please let me know what you think.

I haven't tested it because libFuzzer doesn't support builds for i386 (not really sure why).

[metzman]

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

309

I haven't tested it

By this I mean, I haven't tested it works for x86, I did test that it still works for x64.

[rnk]

lgtm

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

309

That's fine.

[pcc]

llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

311 ↗

(On Diff #169465)

Instead of this you could have done appendToUsed(M, CtorFunc);, we automatically emit /include directives for symbols in llvm.used.
http://llvm-cs.pcc.me.uk/lib/CodeGen/AsmPrinter/AsmPrinter.cpp#1553

[metzman]

llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

311 ↗

(On Diff #169465)

That definitely seems cleaner. Thanks!

I'll try this out when I get a chance.

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

if (TargetTriple.getObjectFormat() == Triple::COFF) {
     SecStartPtr = IRB.CreatePointerCast(GEP, Ty);
   } else {
     SecStartPtr = IRB.CreatePointerCast(SecStart, Ty);
   }
   std::tie(CtorFunc, std::ignore) = createSanitizerCtorAndInitFunctions(
       M, SanCovModuleCtorName, InitFunctionName, {Ty, Ty},
       {SecStartPtr, IRB.CreatePointerCast(SecEnd, Ty)});
 
-  if (TargetTriple.supportsCOMDAT()) {
-    // Use comdat to dedup CtorFunc.
+  if (TargetTriple.supportsCOMDAT()&&
+      TargetTriple.getObjectFormat() != Triple::COFF) {
+    // Use comdat to dedup CtorFunc. Don't do it for COFF files since it breaks
+    // the linker flag: "/OPT:REF".
     CtorFunc->setComdat(M.getOrInsertComdat(SanCovModuleCtorName));
     appendToGlobalCtors(M, CtorFunc, SanCtorAndDtorPriority, CtorFunc);
   } else {
     appendToGlobalCtors(M, CtorFunc, SanCtorAndDtorPriority);
   }
   return CtorFunc;
 }
 
 bool SanitizerCoverageModule::runOnModule(Module &M) {
   if (Options.CoverageType == SanitizerCoverageOptions::SCK_None)
     return false;
   C = &(M.getContext());
   DL = &M.getDataLayout();
   CurModule = &M;
   TargetTriple = Triple(M.getTargetTriple());
   FunctionGuardArray = nullptr;
   Function8bitCounterArray = nullptr;
   FunctionPCsArray = nullptr;
   IntptrTy = Type::getIntNTy(*C, DL->getPointerSizeInBits());
   IntptrPtrTy = PointerType::getUnqual(IntptrTy);
   Type *VoidTy = Type::getVoidTy(*C);
   IRBuilder<> IRB(*C);
   Int64PtrTy = PointerType::getUnqual(IRB.getInt64Ty());