This is an archive of the discontinued LLVM Phabricator instance.

[Sanitizer Coverage] Fix Instrumentation to work on Windows.
ClosedPublic

Authored by mpividori on Jan 6 2017, 8:18 PM.

Details

Summary

Hi,

I modified Sanitizer Coverage Instrumentation's code, to work on Windows.
In Windows, the symbols "___stop___sancov_guards" and "___start___sancov_guards" are not defined automatically.
So, we need to take a different approach.

As suggested in: https://msdn.microsoft.com/en-us/library/7977wcck.aspx
I define 3 sections: ".SCOV$A", ".SCOV$M" and ".SCOV$Z".

  • Section ".SCOV$A" will only hold a variable ___start___sancov_guard.
  • Section ".SCOV$M" will hold the main data.
  • Section ".SCOV$Z" will only hold a variable ___stop___sancov_guards.

When linking, they will be merged sorted by the characters after the $, so we can use the pointers of the variables ___[start|stop]___sancov_guard to know the actual range of addresses of that section.

___[start|stop]___sancov_guard should be defined only once per module, because of that, I include them inside the static asan runtime: "clang_rt.asan_dynamic_runtime_thunk", that will be included in both, dlls and executables (that changes are in a different diff).

In this diff, I updated the instrumentation to include all the guard arrays in section ".SCOV$M".

Diff Detail

Repository
rL LLVM

Event Timeline

mpividori updated this revision to Diff 83493.Jan 6 2017, 8:18 PM
mpividori retitled this revision from to [Sanitizer Coverage] Fix Instrumentation to work on Windows..
mpividori updated this object.
mpividori added reviewers: kcc, aizatsky, rnk, zturner.
mpividori set the repository for this revision to rL LLVM.
mpividori added a subscriber: llvm-commits.
rnk added inline comments.Jan 9 2017, 1:18 PM
lib/Transforms/Instrumentation/SanitizerCoverage.cpp
372–387 ↗(On Diff #83493)

I don't think we need this dynamic initialization in every TU on Windows. If ___start___sancov_guard is linked statically into every DLL, we can add code to that object file to call __sanitizer_cov_trace_pc_guard_init on Windows.

mpividori added inline comments.Jan 9 2017, 2:42 PM
lib/Transforms/Instrumentation/SanitizerCoverage.cpp
372–387 ↗(On Diff #83493)

@rnk, do you mean register this function in the section ".CRT$XCU" , like you do with register_dso_globals? Yes, I agree.
Here, I just updated the code to do the same than on linux. I think we can improve this as you suggest, also for linux.

mpividori added inline comments.Jan 23 2017, 3:30 PM
lib/Transforms/Instrumentation/SanitizerCoverage.cpp
372–387 ↗(On Diff #83493)

@rnk I think this approach is ok for now, so we do the same than for linux, and we simplify the code.

rnk accepted this revision.Jan 23 2017, 3:37 PM

lgtm

lib/Transforms/Instrumentation/SanitizerCoverage.cpp
372–387 ↗(On Diff #83493)

Yeah, definitely OK for now. Eventually we should do the .CRT$XCU thing, though. It's nicer.

This revision is now accepted and ready to land.Jan 23 2017, 3:37 PM
mpividori updated this revision to Diff 86908.Feb 2 2017, 3:37 PM

Updated diff after rebase. It is slightly different because of some new commits.

This revision was automatically updated to reflect the committed changes.