This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY
ClosedPublic

Authored by ldionne on Jul 3 2018, 12:33 PM.

Details

Summary

We never actually mean to always inline a function -- all the uses of
the macro I could find are actually attempts to control the visibility
of symbols. This is better described by _LIBCPP_INLINE_VISIBILITY, which
is actually always defined the same.

This change is orthogonal to the decision of what we're actually going
to do with _LIBCPP_INLINE_VISIBILITY -- it just simplifies things by
having one canonical way of doing things.

Diff Detail

Repository
rL LLVM

Event Timeline

ldionne created this revision.Jul 3 2018, 12:33 PM

Note that even in case we end up renaming _LIBCPP_INLINE_VISIBILITY to something else, this patch is useful because it ensures that nothing was _actually_ meant to be always inline. So any potential rename can then be just that -- a mechanical rename with no potential for semantic change.

FWIW, this change LGTM... but I wouldn't mind hearing from Eric or Marshall before commit.

libcxx/include/any
107–115 ↗(On Diff #153958)

It's possible that this (and other __throw_* functions) were trying to force inlining (i.e., not just controlling visibility). However, if so, I disagree with it; I'd rather trust the optimizer to do the right thing.

ldionne added inline comments.Jul 3 2018, 12:52 PM
libcxx/include/any
107–115 ↗(On Diff #153958)

What would be the reason for wanting to always inline that?

dexonsmith added inline comments.Jul 3 2018, 12:56 PM
libcxx/include/any
107–115 ↗(On Diff #153958)

Hypothetically, I've seen always_inline used to force things to be inlined where people don't trust the optimizer. I'm not sure if it was used that way here... but given that we have a macro called _LIBCCP_ALWAYS_INLINE, I suspect that was the intention at some point for some of the uses.

EricWF accepted this revision.Jul 3 2018, 7:10 PM

LGTM.

I meant to do this earlier, but I figured Howard had a specific difference in mind between _LIBCPP_ALWAYS_INLINE and _LIBCPP_INLINE_VISIBILITY, and I wanted to discover that difference.
But the more I think about this, the less sense _LIBCPP_ALWAYS_INLINE current makes.

This revision is now accepted and ready to land.Jul 3 2018, 7:10 PM

Note that this revision also removes _LIBCPP_ALWAYS_INLINE because it is not used anymore and I want to avoid any potential confusion. If we do come around a need to mark something as always inline in the future, we should reintroduce the macro for that use case. I'm not sure such a use case will come any time soon, though.

Thank you for doing this!

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
davide reopened this revision.Jul 5 2018, 11:29 AM
davide added a subscriber: davide.

The lldb bot started failing very recently and the blamelist hints at this change.

http://green.lab.llvm.org/green/job/lldb-cmake/7777/

Can you please take a look?

For your convenience, this is failing building LibCxx testcases with a linker error:

Build Command Output:
Undefined symbols for architecture x86_64:
  "std::__1::__vector_base_common<true>::__vector_base_common()", referenced from:
      std::__1::__vector_base<int, std::__1::allocator<int> >::__vector_base() in main.o
      std::__1::__vector_base<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > >::__vector_base() in main.o
ld: symbol(s) not found for architecture x86_64
clang-7: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [a.out] Error 1
This revision is now accepted and ready to land.Jul 5 2018, 11:29 AM
davide requested changes to this revision.Jul 5 2018, 11:29 AM
This revision now requires changes to proceed.Jul 5 2018, 11:29 AM

The lldb bot started failing very recently and the blamelist hints at this change.

http://green.lab.llvm.org/green/job/lldb-cmake/7777/

Can you please take a look?

For your convenience, this is failing building LibCxx testcases with a linker error:

Build Command Output:
Undefined symbols for architecture x86_64:
  "std::__1::__vector_base_common<true>::__vector_base_common()", referenced from:
      std::__1::__vector_base<int, std::__1::allocator<int> >::__vector_base() in main.o
      std::__1::__vector_base<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > >::__vector_base() in main.o
ld: symbol(s) not found for architecture x86_64
clang-7: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [a.out] Error 1

Interesting. The failing jobs all have things like this:

#include <string>
#ifdef _LIBCPP_INLINE_VISIBILITY
#undef _LIBCPP_INLINE_VISIBILITY
#endif
#define _LIBCPP_INLINE_VISIBILITY
#include <map>
#include <vector>

We should revert to green, but... why is LLDB doing that?

I reverted this commit. Sorry for the blunder. I'll take a look at why LLDB's tests are doing this.

ldionne updated this revision to Diff 154415.Jul 6 2018, 8:33 AM

This revision to the patch fixes a problem where __pbump had been applied
_LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY instead of _LIBCPP_INLINE_VISIBILITY,
which caused the symbols exported in the ABI to change and broke the CI.

Independently, LLDB tests that were failing because they relied on
_LIBCPP_INLINE_VISIBILITY were fixed so that this change should not
break them again.

I still haven't been able to run the ABI tests locally (they fail on my
machine), and I'd like to do that before I submit this patch.

ldionne added inline comments.Jul 6 2018, 8:36 AM
libcxx/include/streambuf
261 ↗(On Diff #154415)

This one was marked as _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY instead of _LIBCPP_INLINE_VISIBILITY in the previous revision. This is what caused the check-cxx-abilist test to fail.

I've now managed to run the check-cxx-abilist test on my machine and it passes. I'd like to commit this again, @EricWF am I good to go?

Pushing since I got an offline O.K. from Eric.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 11 2018, 4:19 PM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.