This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] Provide a list of functions from sanitizers's interfaces.
ClosedPublic

Authored by mpividori on Jan 25 2017, 2:25 PM.

Details

Summary

In this diff I propose to add a new auxiliary file to each sanitizer: sanitizer_interface.inc, listing all the functions exported, with the macros: INTERFACE_FUNCTION() and INTERFACE_WEAK_FUNCTION().

So, when we need to define/do something for each function in the sanitizer's interface, we can define the macros and include that header, like:

#define INTERFACE_FUNCTION(Name)   //Do something with Name.
#define INTERFACE_WEAK_FUNCTION(Name)   //Do something with Name, maybe different because Name is a weak function.
#include "sanitizer_interface.inc"

In particular, I need these files for Windows, in the nexts diffs.
Also, this files could replace the existing files: weak_symbols.txt for Apple. Instead of reading weak_symbols.txt to get the list of weak symbols, we can read the file sanitizer_interface.inc and consider all the symbols included with the macro INTERFACE_WEAK_FUNCTION(Name).

In this diff, I only include these files to the sanitizers that work on Windows. We could do the same for the rest of the sanitizers and remove the weak_symbols.txt files.

Diff Detail

Repository
rL LLVM

Event Timeline

mpividori created this revision.Jan 25 2017, 2:25 PM
kcc edited edge metadata.Jan 25 2017, 2:28 PM

Please don't. This is yet another thing to maintain -- it will be broken by every change that changes any of the API functions.

@kcc, please wait to see the other diffs. I only move code from Windows files to a general file.

kubamracek edited edge metadata.Jan 25 2017, 2:30 PM
In D29148#656808, @kcc wrote:

Please don't. This is yet another thing to maintain -- it will be broken by every change that changes any of the API functions.

What if a lit test is added that verifies the list matches the built libraries?

kcc added a comment.Jan 25 2017, 2:32 PM
In D29148#656808, @kcc wrote:

Please don't. This is yet another thing to maintain -- it will be broken by every change that changes any of the API functions.

What if a lit test is added that verifies the list matches the built libraries?

If we can have a lit test that works on linux and if this is just moving stuff from win-specific to generic -- might be ok.
Right now the code is often broken by changes that work on linux and don't work on windows.

@kcc @kubamracek This interface list, really simplify the code for Windows, as you can see in:

Otherwise, I need to repeat this list many times. I will think about a lit tests.

mpividori updated this revision to Diff 86097.Jan 27 2017, 12:28 PM

Add missing functions.

@kubamracek @kcc

It is not easy to provide a test for all the platforms, because we expose different interfaces in each case. Of course, we could add some extra files of platform specific interfaces like: asan_interface_linux.inc asan_interface_windows.inc , etc. But I really don't like that.
In https://reviews.llvm.org/D29229 I fix the tests for Windows. It checks that the list of functions mentioned in sanitizer_interface.inc files matches the list of functions exported by asan library. So, after this changes, this continue working as before, if you forget to add a new function to the interface list, only the tests on Windows fail.

I have a new idea that would simplify all of this. We could generate these files automatically, with some cmake code, just after creating the sanitizers libraries. So this is what I propose:

  • WEAK SYMBOLS: Instead of the sanitizer_interface.inc files, we could have a different file: sanitizer_weak_interface.inc that only lists weak symbols. So it can be used by apple, and we also need it to generate the sanitizers main libraries for windows (asan dll, and asan static library) and the dynamic_runtime_thunks. Also, we could generate it automatically searching in a sanitizer directory for all the definitions like "WEAK_DEF(....)". We could include that to cmake, and we avoid a lot of problems.
  • INTERFACE LIST: We need a list of the functions exported by the sanitizers to generate the dll_thunk static library. So, we include some cmake code to list all the symbols exported by the asan library and then use that file to compile dll_thunk library.

I will work on this.
Thanks,

@kubamracek @kcc

For Windows, we consider 2 different cases depending on MD / MT.

+ When considering MD:
We include all the sanitizers in a dll: "asan_dynamic.dll"
Also, we create an auxiliary static library "asan_dynamic_runtime_thunk.lib" that clang driver includes and links with both, dlls and executables.
Both, the executable and the dll redirect the calls to the external dll.

+ When considering MT:
We include all the sanitizers in a static library: "asan.lib" which is linked to the main executable.
Also, we create an auxiliary static library "asan_dll_thunk.lib" that clang driver includes and links with dlls, so they can access to the implementation in the main executable.


  1. When asan is implemented as a dll (MD), we need the list of weak functions for:
  1. When asan is implemented as a static library linked to the main executable (MT), we need the list of all the sanitizer interface for:
    • asan_win_dll_thunk.lib : because we use interception to refer to the implementation in the main executable from other dlls.

So, before this diff, all the functions exported were listed in asan_win_dll_thunk.cc. I am moving them into a general file.

I would like to generate these header files automatically. I have been working on this, but it is a bit complicated because we have some limitations. This is the general idea:

  • For the list of weak functions: search for the macro SANITIZER_INTERFACE_WEAK_DEF(...) in all the .cc files (for example using grep).
  • For the list of sanitizer interface: execute "dumpbin /exports asan_dynamic.dll" and save the output in the header file.

In both cases, we could generate the headers at build time, with "add_custom_command(...)" and add apropriate dependencies with OBJECT_DEPENDS.
I have implemented this, just using grep and sed, and works fine. When building, the headers files: sanitizer_interface.h are generated in the build directory, and are included by the source files.

The problem is that on Windows (http://llvm.org/docs/GettingStartedVS.html) , we don't require gnu tools for building. We only require them for running the tests. So, I don't think I can use grep or sed when building asan.

Also, I can't do this at configure time, because I need to execute dumpbin after building asan_dynamic.dll.

I could try to do the same using windows tools like findstr (for grep), but I find it particularly difficult to find an equivalent to sed.

Before continue working on this, I would like to know your opinion.

Thanks,

mpividori updated this revision to Diff 86236.Jan 29 2017, 4:35 PM

@kubamracek @kcc

I updated the diff to include tests for: Linux, Darwin and Windows. Now, if you export a new function not present in the interface list, the test "interface_symbols_[darwin|windows|linux].c" fails.

Also, I remove the comments: "/* OPTIONAL */" which are not required any more, because we use the macro: "INTERFACE_WEAK_FUNCTION()" for weak functions.

I tested on Linux, and Windows. Could you check if the test works on Darwin? Probably need to change some details.

Also, we could remove the "weak_symbols.txt" files and read the "interface.inc" files for Darwin.

Thanks.

If we can have a lit test that works on linux and if this is just moving stuff from win-specific to generic -- might be ok.
Right now the code is often broken by changes that work on linux and don't work on windows.

With the last diff I meet the mentioned requirements:
+ I am moving stuff from asan_win_dll_thunks.cc.
+ I added tests for Windows, Linux and Darwin.

rnk accepted this revision.Jan 30 2017, 9:53 AM

Looks good to me. As I understand it, this change doesn't use the .inc files for any functional purpose other than to check the list of symbols exported by the Darwin and Linux ASan runtimes. D29158 and other patches add the uses.

This revision is now accepted and ready to land.Jan 30 2017, 9:53 AM
alekseyshl accepted this revision.Jan 30 2017, 10:49 AM
alekseyshl added inline comments.
lib/asan/asan_interface.inc
167 ↗(On Diff #86236)

Can you sort .inc files lexicographically (within function groups, where grouping makes sense)?

mpividori added inline comments.Jan 30 2017, 10:51 AM
lib/asan/asan_interface.inc
167 ↗(On Diff #86236)

@alekseyshl Ok, I will do that.

mpividori updated this revision to Diff 86365.Jan 30 2017, 4:38 PM

Done.
@kubamracek could you test it on Darwin? should I submit the commit and see if buildbot fails?
Thanks!

This revision was automatically updated to reflect the committed changes.

FYI, on Darwin, this currently fails the test with:

=== NOTE === If you see a mismatch below, please update sanitizer_interface.inc files.
52a53
> __asan_option_detect_stack_use_after_return
106a108
> __asan_shadow_memory_dynamic_address
140a143
> __asan_test_only_reported_buggy_pointer
188a192,199
> __sanitizer_mz_calloc
> __sanitizer_mz_destroy
> __sanitizer_mz_free
> __sanitizer_mz_malloc
> __sanitizer_mz_memalign
> __sanitizer_mz_realloc
> __sanitizer_mz_size
> __sanitizer_mz_valloc

I don't see a reason for __sanitizer_mz_* functions to be exported on Darwin. They originally were not and @glider marked them with SANITIZER_INTERFACE_ATTRIBUTE in r227967.

@kubamracek I fixed the test in https://reviews.llvm.org/D29345 . Let me know if you agree with that commit.
Also, if you want to remove SANITIZER_INTERFACE_ATTRIBUTE to __sanitizer_mz_* , we should use nm -D, otherwise we will have the same problem.
Also, if we don't want to export them, I think we should also remove them from the export list. Otherwise they will be exported even if they are hidden I guess.