This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] Define delimiters for sanitizer coverage's binary section on Windows.
ClosedPublic

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

Details

Summary

Related to the changes explained in: https://reviews.llvm.org/D28434
___[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.

Diff Detail

Repository
rL LLVM

Event Timeline

mpividori updated this revision to Diff 83495.Jan 6 2017, 8:25 PM
mpividori retitled this revision from to [compiler-rt] Define delimiters for sanitizer coverage's binary section 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:20 PM
lib/asan/asan_win_coverage_sections.cc
18 ↗(On Diff #83495)

You can use the same technique as asan_globals_win.cc to avoid having a dynamic initializer in every TU with coverage.

mpividori added inline comments.Jan 23 2017, 3:31 PM
lib/asan/asan_win_coverage_sections.cc
18 ↗(On Diff #83495)

@rnk Would you agree if we consider that in the future? I think this diff is OK for now, so we take the same approach than for linux and we simplify the code.
Thanks.

kcc edited edge metadata.Jan 23 2017, 3:33 PM

why is this in asan? what about the other sanitizers that should work with coverage?

@kcc this is because all the support for Windows, auxiliary static libraries and dlls, are defined in asan directory. This is how code is organized now.
I think this ok for now. We can consider moving it in the future when all sanitizer coverage's code is moved to the new sancov folder.

kcc added a comment.Jan 23 2017, 3:48 PM

This sounds like accumulating more technical debt.
None of this code belongs to asan (right??) and we should not be adding it to asan.

@kcc Yes it belongs to asan, since that is the only place where we build the dlls for windows.
All the hooks for windows, interception, etc, are implemented inside asan directory, even though some of that code is related to sanitizer coverage. For example, you can see the file: asan_win_dll_thunk.cc, that we re using interception for all the functions of sanitizer coverage. And that code is in asan directory. I think this was coded that way to simplify the code, and avoid many files in different places.
I would like to reorganize all the files, but maybe in a different diff, in the future, after finishing with this?

kcc added a comment.Jan 23 2017, 4:28 PM

I'd like to avoid changes that are known to require large reorg in future.
can we do such reorg right away?

@kcc Ok! I will work on this. Thanks.

mpividori updated this revision to Diff 85840.Jan 25 2017, 4:51 PM

I update this diff to include the changes only for sanitizer coverage, independent of asan.

Hi @alekseyshl @kcc,

Related to the changes explained in: https://reviews.llvm.org/D28434 ___[start|stop]___sancov_guard should be defined only once per module.
In Windows, we have 2 different cases:

+ When considering MD:

All the modules, main executable and dlls, are linked to an auxiliary static library "asan_dynamic_runtime_thunk.lib". Because of that, I include `sanitizer_coverage_win_sections.cc` to `SancovDynamicRuntimeThunk`.

+ When considering MT:

The main executable in linked to the static library: "asan.lib".
All the dlls are linked to an auxiliary static library "asan_dll_thunk.lib".
Because of that, I include `sanitizer_coverage_win_sections.cc` to both `SancovDllThunk` and `SANITIZER_LIBCDEP_SOURCES` (which is included in `asan.lib`).

Would you agree?

@kcc all the reorganization required was included in these diffs:
+ https://reviews.llvm.org/D29148
+ https://reviews.llvm.org/D29154
+ https://reviews.llvm.org/D29158

zturner edited edge metadata.Feb 2 2017, 1:42 PM

@rnk, does this look good to you?

This revision is now accepted and ready to land.Feb 2 2017, 2:39 PM
This revision was automatically updated to reflect the committed changes.