This is an archive of the discontinued LLVM Phabricator instance.

[libc++] add `inline` for __open in ifstream and ofstream
ClosedPublic

Authored by jasonliu on Mar 26 2021, 9:38 AM.

Details

Summary

When building with gcc on AIX, it seems that gcc does not like the always_inline without the inline keyword.
Such error is observed:

/home/jasonli/llvm/build2/include/c++/v1/fstream: In function 'bool std::__1::__fs::filesystem::detail::{anonymous}::copy_file_impl(std::__1::__fs::filesystem::detail::{anonymous}::FileDescriptor&, std::__1::__fs::filesystem::detail::{anonymous}::FileDescriptor&, std::__1::error_code&)':
/home/jasonli/llvm/build2/include/c++/v1/fstream:1329:6: error: inlining failed in call to always_inline 'void std::__1::basic_ifstream<_CharT, _Traits>::__open(int, std::__1::ios_base::openmode) [with _CharT = char; _Traits = std::__1::char_traits<char>]': function body not available
/home/jasonli/GitHub/llvm-project/libcxx/src/filesystem/operations.cpp:856:14: note: called from here
     in.__open(read_fd.fd, ios::binary);
     ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~

So adding the inline keywords in for __open in ifstream and ofstream. That will also make it consistent with __open in basic_filebuf (it seems we added inline there for gcc build as well).

Diff Detail

Event Timeline

jasonliu requested review of this revision.Mar 26 2021, 9:38 AM
jasonliu created this revision.
jasonliu edited the summary of this revision. (Show Details)
Quuxplusone added inline comments.
libcxx/include/fstream
1543

I think you should just copy what close does on lines 1194 and 1555. Namely, put _LIBCPP_INLINE_VISIBILITY (only) on the declaration and inline (only) on the definition. Does that make GCC happy enough?

jasonliu added inline comments.Mar 26 2021, 9:59 AM
libcxx/include/fstream
1543

Thanks for the speedy review. Yes, it does. Do we want to make the matching change for basic_filebuf's __open as well? Currently it's using the same style as what we have here.

libcxx/include/fstream
1543

My impression from the code is that we are really trying to have (only?) two different kinds of member functions:

  • ones like basic_filebuf::open(const char*, ios_base::openmode) and basic_filebuf::close and basic_filebuf::swap, which are marked neither inline nor _LIBCPP_INLINE_VISIBILITY;
  • ones like basic_filebuf::open(const string&, ios_base::openmode) and basic_filebuf::is_open, which are marked inline on the definition and _LIBCPP_INLINE_VISIBILITY on the declaration.

basic_filebuf::__open is currently not in either of these categories. I think we should change it to one or the other. But beware that I don't understand the criteria by which we choose one or the other; in particular I don't know if there might be ABI implications somehow?

jasonliu updated this revision to Diff 333595.Mar 26 2021, 11:05 AM

Making basic_filebuf's __open consistent with other __open.

jasonliu added inline comments.Mar 26 2021, 11:11 AM
libcxx/include/fstream
1543

I think it makes sense to to choose the first categories you listed there, as we already have _LIBCPP_INLINE_VISIBILITY on those declarations.

@ldionne Hi Louis, what do you think about this patch? Do you have any ABI implication concern?

ldionne accepted this revision.Apr 12 2021, 9:31 AM
This revision is now accepted and ready to land.Apr 12 2021, 9:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2021, 12:26 PM