Merge lp:~jpakkane/unity-scopes-api/visibility into lp:unity-scopes-api/devel

[Jussi Pakkanen]

Visibility flag fix.

Don't use -fvisibility=default because it is, well, the default. Instead use -fvisibility-inlines-hidden because it is safe and gives the compiler room to do some optimizations it could not do without it.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:322
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/483/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-amd64-ci/35/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/35/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-i386-ci/35/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/483/rebuild

review: Needs Fixing (continuous-integration)

[lp:~jpakkane/unity-scopes-api/visibility]

323. By Jussi Pakkanen on 2014-05-08

Remove inline destructors that are no longer exported from the symbol file.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:323
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/485/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-amd64-ci/37
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/37
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/37/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-i386-ci/37

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/485/rebuild

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

Looks like this actually breaks ABI, even though the internal and testing stuff wouldn't really matter, there is the CategorisedResult and OptionSelectorFilter which do.

Is it really safe to remove these?

review: Needs Information

[Michi Henning (michihenning)]

I'm feeling uneasy about this because it's more complicated than what we had previously. None of these destructors are even vaguely performance critical, and saving a dozen entries in the symbol table hardly seems worth it. On balance, I think I'd prefer to leave things as they are. The whole symbol munging stuff is complicated enough already…

[Jussi Pakkanen (jpakkane)]

The performance improvement does not come from these few methods. They come from all the small helper functions and the like that come from libraries, especially templates. Because of the symbol map these are not exported but this is not known at compile time and thus the compiler needs to assume the worst. Gcc's wiki and Ulrich Drepper's shared library tutorial paper have a more through discussion on this subject. As an example, on a debug build the test .so (which has all symbols) shrinked from 40MB to 38MB. The symbol stripped one stayed roughly the same size.

As far as the ABI is concerned this may or may not be an ABI break. The reasons for this are a bit complicated. Since the destructors are compiler generated, they are inline and also show up in the .so. However those methods are never called in the .so, but rather the inlined version in the client is called. (To be precise, either one could be called, but gcc probably always uses the inline ones).

This has the downside that if we ever ever create an actual method for one of these, the visible ABI does not change but we have silently broken the ABI. Any code linked against the old version will call its own inlined destructor instead of the one in the .so which causes things so fail, eiher violently or silently.

If we are really serious about ABI then we can't merge this. But on the other hand we can never create an implementation for these affected methods either.

[Michi Henning (michihenning)]

> The performance improvement does not come from these few methods. They come
> from all the small helper functions and the like that come from libraries,
> especially templates. Because of the symbol map these are not exported but
> this is not known at compile time and thus the compiler needs to assume the
> worst. Gcc's wiki and Ulrich Drepper's shared library tutorial paper have a
> more through discussion on this subject. As an example, on a debug build the
> test .so (which has all symbols) shrinked from 40MB to 38MB. The symbol
> stripped one stayed roughly the same size.

I read the paper. That was an interesting read, thanks!

Maybe I'm missing something. But, pretty much by definition, this flag removes exactly 12 entries from the shared library symbol table (out of hundreds, I'm guessing--I didn't count them).

I cannot see where any performance benefit would come from here.

> As far as the ABI is concerned this may or may not be an ABI break. The
> reasons for this are a bit complicated. Since the destructors are compiler
> generated, they are inline and also show up in the .so. However those methods
> are never called in the .so, but rather the inlined version in the client is
> called. (To be precise, either one could be called, but gcc probably always
> uses the inline ones).

Sure, so there is a method in the .so that won't ever be called. So what? There are twelve destructors that won't be called. I don't see the benefit of eliminating those from the .so.

I'm not all that concerned about the ABI, seeing that we are still at the pre-release stage. With or without this change, I don't think ABI stability would be affected.

From the man page:

    "This switch declares that the user does not attempt to compare
     pointers to inline functions or methods where the addresses of
     the two functions are taken in different shared objects."

Seeing that all of the functions involved are destructors, I don't think this is likely to ever affect ABI compatibility.

My concern is more about the fact that -fvisibility-inlines-hidden is an obscure flag that most people will have to look up to find out what it does (I had to look it up), and that there is no tangible benefit (as far as I can see) to the size or performance of our code.

[Jussi Pakkanen (jpakkane)]

The benefit does not come from those 12 entries. In fact I originally did not even realize they would show up. To see what the issue is let's look at an example.

Suppose we use something from the standard library, say a template std::foo<>. That uses in its implementation some other template/class/whatever, say std::foo<>::private::something. When the compiler generates code for something::method, it can do some optimizations if it knows that the method is never accessed outside the current .so. I don't know exactly what those are, but this is what GCC's documentation says. Thus -fvisibility-inlines-hidden affects classes and methods that we normally never see, as is the case here.

The performance benefit is probably minimal in this particular case, so there's no point in putting too much effort into this. However it seems the better choice to tell the compiler not to produce garbage in the first place rather than filtering it out with a linker script afterwards.

And no, you can't survive with only one of these. The documents mentioned above imply very strongly that you need both visibility definitions and a linker script to get optimal performance. This sucks, but unfortunately shared libraries are a bit of a black magic thing.

[Jussi Pakkanen (jpakkane)]

Also: it is considered dubious practice to have inline methods in your headers even in plain C. An alternative to removing those symbols from the symbols file is to make sure we don't have any compiler generated methods in our public classes. Basically it would mean going from this:

class Foo {
  Foo(bar) { ... }
};

Into this:

class Foo {
  Foo(bar);
};

and then in the cpp file have:

Foo::Foo(bar) {
}

This would mean that we can change the implementation of this constuctor/destructor/etc without breaking ABI.

I started doing this already but then decided that removing the symbols from the file would be simpler. Thinking about it this explicit declaration would probably be the better solution overall.

[Michi Henning (michihenning)]

> Suppose we use something from the standard library, say a template std::foo<>.
> That uses in its implementation some other template/class/whatever, say
> std::foo<>::private::something. When the compiler generates code for
> something::method, it can do some optimizations if it knows that the method is
> never accessed outside the current .so. I don't know exactly what those are,
> but this is what GCC's documentation says. Thus -fvisibility-inlines-hidden
> affects classes and methods that we normally never see, as is the case here.

From the man page:

"The effect of this is that GCC may, effectively, mark inline methods with
"__attribute__ ((visibility("hidden")))" so that they do not appear in the
export table of a DSO and do not require a PLT indirection when used within
the DSO. Enabling this option can have a dramatic effect on load and link
times of a DSO as it massively reduces the size of the dynamic export table
when the library makes heavy use of templates."

OK, so all it really means is "inline methods don't go into the symbol table".

I can see how this can make a big difference due to a smaller symbol table,
as well as avoiding the indirection through the procedure linkage table
for inline methods, if there are tons of inline methods.

But this doesn't apply to our code, other than for those twelve destructors.

> The performance benefit is probably minimal in this particular case, so
> there's no point in putting too much effort into this. However it seems the
> better choice to tell the compiler not to produce garbage in the first place
> rather than filtering it out with a linker script afterwards.

One other concern with this… Inlining is always only a recommendation to the
compiler, and the compiler can choose to not inline something whenever it feels
like it. Now, suppose we ship the library compiled with gcc, and someone uses
clang to build their code. As far as I can see, removing the inlined functions
from the symbol table will work only if clang inlines functions in exactly the
same way as gcc. If it doesn't, it's possible that clang does not inline
something in a header file that gcc *does* inline, and then, at link time,
there will be an undefined symbol because the method inlined by gcc doesn't appear
in the symbol table.

> Also: it is considered dubious practice to have inline methods in your headers even in plain C.

That's the first time I have come across that. At least in cases where ABI stability is not important, I don't see the harm. And, for templates, doing so is pretty much unavoidable.

Anyway, looking at the affected destructors, they are basically all cases where we have a class that defines a constructor, but doesn't bother to define a destructor because the default destructor works just fine. I don't see anything wrong with this. Explicitly defining the destructor won't improve anything here, I believe.

I'm still concerned about the potential difficulties when someone uses clang to compile their code and links against our gcc-generated library. I checked, and clang essentially does not document -fvisibility. All it says is "This flag sets the default visibility level." So, I don't think we can...

[Marcus Tomlinson (marcustomlinson)]

Cleaning up old MPs

review: Disapprove

Unmerged revisions

323. By Jussi Pakkanen on 2014-05-08

Remove inline destructors that are no longer exported from the symbol file.

322. By Jussi Pakkanen on 2014-05-08

Fix visibility flags.

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2014-05-06 10:10:37 +0000
+++ CMakeLists.txt	2014-05-08 14:40:18 +0000
@@ -123,8 +123,9 @@
 
 # By default, all symbols are visible in the library. We strip out things we don't
 # want at link time, by running a version script (see unity-scopes.map and the
-# setting of LINK_FLAGS below).
-set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fvisibility=default")
+# setting of LINK_FLAGS below). We just hide the inlines because that is safe
+# and gives the compiler more possibilities to optimize.
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fvisibility-inlines-hidden")
 
 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -pedantic -Wall -Wextra")
 set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -pedantic -Wall -Wextra") # Can't use -std=c++11 with C compiler
 
=== modified file 'debian/libunity-scopes1.symbols'
--- debian/libunity-scopes1.symbols	2014-05-08 06:04:29 +0000
+++ debian/libunity-scopes1.symbols	2014-05-08 14:40:18 +0000
@@ -186,7 +186,6 @@
  (c++)"unity::scopes::CategorisedResult::CategorisedResult(unity::scopes::internal::ResultImpl*)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::CategorisedResult::CategorisedResult(unity::scopes::CategorisedResult const&)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::CategorisedResult::CategorisedResult(std::shared_ptr<unity::scopes::Category const>)@Base" 0.4.0+14.04.20140312.1
- (c++)"unity::scopes::CategorisedResult::~CategorisedResult()@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::CategorisedResult::operator=(unity::scopes::CategorisedResult&&)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::CategorisedResult::operator=(unity::scopes::CategorisedResult const&)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::NotFoundException::NotFoundException(unity::scopes::NotFoundException const&)@Base" 0.4.0+14.04.20140312.1
@@ -234,7 +233,6 @@
  (c++)"unity::scopes::OptionSelectorFilter::OptionSelectorFilter(std::map<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, unity::scopes::Variant, std::less<std::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::basic_string<char, std::char_traits<char>, std::allocator<char> > const, unity::scopes::Variant> > > const&)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::OptionSelectorFilter::OptionSelectorFilter(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::OptionSelectorFilter::OptionSelectorFilter(std::map<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, unity::scopes::Variant, std::less<std::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::basic_string<char, std::char_traits<char>, std::allocator<char> > const, unity::scopes::Variant> > > const&)@Base" 0.4.0+14.04.20140312.1
- (c++)"unity::scopes::OptionSelectorFilter::~OptionSelectorFilter()@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::ActivationListenerBase::finished(unity::scopes::ListenerBase::Reason, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::ActivationListenerBase::activated(unity::scopes::ActivationResponse const&)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::ActivationListenerBase::ActivationListenerBase()@Base" 0.4.0+14.04.20140312.1
@@ -324,12 +322,10 @@
  (c++)"unity::scopes::Variant::operator=(int)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::testing::StudentsTTest::one_sample(unity::scopes::testing::Sample const&, double, double)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::testing::StudentsTTest::two_independent_samples(unity::scopes::testing::Sample const&, unity::scopes::testing::Sample const&)@Base" 0.4.0+14.04.20140312.1
- (c++)"unity::scopes::testing::StudentsTTest::Result::~Result()@Base" 0.4.2+14.04.20140404.2
  (c++)"unity::scopes::testing::InProcessBenchmark::for_action(std::shared_ptr<unity::scopes::ScopeBase> const&, unity::scopes::testing::Benchmark::ActionConfiguration)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::testing::InProcessBenchmark::for_preview(std::shared_ptr<unity::scopes::ScopeBase> const&, unity::scopes::testing::Benchmark::PreviewConfiguration)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::testing::InProcessBenchmark::for_activation(std::shared_ptr<unity::scopes::ScopeBase> const&, unity::scopes::testing::Benchmark::ActivationConfiguration)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::testing::InProcessBenchmark::for_query(std::shared_ptr<unity::scopes::ScopeBase> const&, unity::scopes::testing::Benchmark::QueryConfiguration)@Base" 0.4.0+14.04.20140312.1
- (c++)"unity::scopes::testing::InProcessBenchmark::~InProcessBenchmark()@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::testing::AndersonDarlingTest::for_normality(unity::scopes::testing::Sample const&)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::testing::ScopeMetadataBuilder::description(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::testing::ScopeMetadataBuilder::search_hint(unity::scopes::testing::ScopeMetadataBuilder::Optional<std::basic_string<char, std::char_traits<char>, std::allocator<char> > > const&)@Base" 0.4.0+14.04.20140312.1
@@ -346,16 +342,11 @@
  (c++)"unity::scopes::testing::OutOfProcessBenchmark::for_preview(std::shared_ptr<unity::scopes::ScopeBase> const&, unity::scopes::testing::Benchmark::PreviewConfiguration)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::testing::OutOfProcessBenchmark::for_activation(std::shared_ptr<unity::scopes::ScopeBase> const&, unity::scopes::testing::Benchmark::ActivationConfiguration)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::testing::OutOfProcessBenchmark::for_query(std::shared_ptr<unity::scopes::ScopeBase> const&, unity::scopes::testing::Benchmark::QueryConfiguration)@Base" 0.4.0+14.04.20140312.1
- (c++)"unity::scopes::testing::OutOfProcessBenchmark::~OutOfProcessBenchmark()@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::testing::Result::Result()@Base" 0.4.0+14.04.20140312.1
- (c++)"unity::scopes::testing::Result::~Result()@Base" 0.4.0+14.04.20140312.1
- (c++)"unity::scopes::testing::Sample::~Sample()@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::testing::Benchmark::Result::save_to_xml(std::basic_ostream<char, std::char_traits<char> >&)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::testing::Benchmark::Result::load_from_xml(std::basic_istream<char, std::char_traits<char> >&)@Base" 0.4.0+14.04.20140312.1
- (c++)"unity::scopes::testing::Benchmark::Result::Timing::~Timing()@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::testing::Benchmark::Result::save_to(std::basic_ostream<char, std::char_traits<char> >&)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::testing::Benchmark::Result::load_from(std::basic_istream<char, std::char_traits<char> >&)@Base" 0.4.0+14.04.20140312.1
- (c++)"unity::scopes::testing::Benchmark::~Benchmark()@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::testing::operator==(unity::scopes::testing::Benchmark::Result const&, unity::scopes::testing::Benchmark::Result const&)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::testing::operator<<(std::basic_ostream<char, std::char_traits<char> >&, unity::scopes::testing::Benchmark::Result const&)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::Category::Category(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unity::scopes::CategoryRenderer const&)@Base" 0.4.0+14.04.20140312.1
@@ -419,7 +410,6 @@
  (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::ScopeProcess(unity::scopes::internal::RegistryObject::ScopeExecData)@Base" 0.4.2+14.04.20140404.2
  (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::ScopeProcess(unity::scopes::internal::RegistryObject::ScopeProcess const&)@Base" 0.4.2+14.04.20140404.2
  (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::~ScopeProcess()@Base" 0.4.2+14.04.20140404.2
- (c++)"unity::scopes::internal::RegistryObject::ScopeExecData::~ScopeExecData()@Base" 0.4.2+14.04.20140404.2
  (c++)"unity::scopes::internal::RegistryObject::state_receiver()@Base" 0.4.2+14.04.20140404.2
  (c++)"unity::scopes::internal::RegistryObject::add_local_scope(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, unity::scopes::ScopeMetadata const&, unity::scopes::internal::RegistryObject::ScopeExecData const&)@Base" 0.4.2+14.04.20140404.2
  (c++)"unity::scopes::internal::RegistryObject::is_scope_running(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)@Base" 0.4.2+14.04.20140404.2
@@ -430,7 +420,6 @@
  (c++)"unity::scopes::internal::RegistryObject::locate(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::internal::RegistryObject::RegistryObject(core::posix::ChildProcess::DeathObserver&, std::shared_ptr<unity::scopes::internal::Executor> const&)@Base" 0.4.3+14.10.20140428
  (c++)"unity::scopes::internal::RegistryObject::~RegistryObject()@Base" 0.4.0+14.04.20140312.1
- (c++)"unity::scopes::internal::MiddlewareFactory::MiddlewareData::~MiddlewareData()@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::internal::MiddlewareFactory::to_kind(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::internal::MiddlewareFactory::MiddlewareFactory(unity::scopes::internal::RuntimeImpl*)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::internal::MiddlewareFactory::~MiddlewareFactory()@Base" 0.4.0+14.04.20140312.1
@@ -460,7 +449,6 @@
  (c++)"unity::scopes::internal::StateReceiverObject::~StateReceiverObject()@Base" 0.4.2+14.04.20140404.2
  (c++)"unity::scopes::internal::Executor::exec(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, std::map<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > const&, core::posix::StandardStream const&)@Base" 0.4.3+14.10.20140428
  (c++)"unity::scopes::internal::Executor::Executor()@Base" 0.4.3+14.10.20140428
- (c++)"unity::scopes::internal::Executor::~Executor()@Base" 0.4.3+14.10.20140428
  (c++)"unity::scopes::internal::ScopeImpl::perform_action(unity::scopes::Result const&, unity::scopes::ActionMetadata const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::shared_ptr<unity::scopes::ActivationListenerBase> const&)@Base" 0.4.0+14.04.20140312.1
  (c++)"unity::scopes::internal::ScopeImpl::fwd()@Base" 0.4.3+14.10.20140428
  (c++)"unity::scopes::internal::ScopeImpl::create(std::shared_ptr<unity::scopes::internal::MWScope> const&, unity::scopes::internal::RuntimeImpl*, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)@Base" 0.4.0+14.04.20140312.1