This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] [sancov] Move __sancov_default_options declaration outside the namespace __sancov
ClosedPublic

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

Details

Summary

I need to move __sancov_default_options declaration outside the namespace __sancov, so I can include sancov_flags.h and refer to __sancov_default_options without requiring the namespace prefix. Which makes sense since __sancov_default_options includes extern "C", so it is exported without the namespace mangling.

Diff Detail

Repository
rL LLVM

Event Timeline

mpividori created this revision.Jan 25 2017, 8:45 PM
kcc edited edge metadata.Jan 25 2017, 9:11 PM

code LGTM, but please add a test

@kcc A simple tests would be:

\\ test.cc
#include "sancov_flags.h"
int main() {
  __sancov_default_options();
  return 0;
}
kcc added a comment.Jan 25 2017, 9:56 PM

no, users should not need to include sancov_flags.h, and so test shouldn't include it.
and no, this is not a test I want to see.
Check test/asan/TestCases/default_options.cc for an example.

@kcc Again. I think you are not understanding what I mean. This is only a simple change in the header, that only impact to the internal code that use that header. We don't need a test for this.

kcc added a comment.Jan 25 2017, 10:06 PM

sancov_default_options is a part of the interface, just like asan_default_options.
We did not have a test, and so the current implementation is plain wrong.
Your fix makes it right. And so need a test.

@kcc
As far as I understand, headers provided to the users of sanitizer coverage are included in compiler-rt\include\sanitizer\coverage_interface.h . That file is outdated and doesn't include any of the new functions. For example: __sancov_default_options is not there.
The problem that I mention is with the internal header sancov_flags.h. The library generated is exactly the same. It provides __sancov_default_options with 'C' linkage regardless of this diff. This diff only updates the declaration, so I can use it internally.

@kcc
If we define __sancov_default_options inside a namespace, it will be ignored, and it will be included with 'C' linkage.

// somefile.cc
namespace  __sancov {
   extern "C" const char* __sancov_default_options() { return ""; }
}

But if we declare it inside a namespace like:

// somefile.h
namespace  __sancov {
   extern "C" void __sancov_default_options();
}

I need to add the namespace when using it. (even though the compiler will add an external undefined symbol with 'C' linkage, without the namespace)

// main.cc
#include "somefile.h"
int main() {
  somenamespace::somefun();
  return 0;
}

So, because of that I simply move the external declaration outside the namespace. So, I can refer to it as somefun() without the namespace.

kcc accepted this revision.Jan 30 2017, 5:21 PM

LGTM, thanks for explaining

This revision is now accepted and ready to land.Jan 30 2017, 5:21 PM
This revision was automatically updated to reflect the committed changes.