Page MenuHomePhabricator

[libcxx] Fix compile for BUILD_EXTERNAL_THREAD_LIBRARY
ClosedPublic

Authored by Nicu on Aug 26 2020, 1:47 AM.

Details

Summary

Fix compilation with -DLIBCXX_BUILD_EXTERNAL_THREAD_LIBRARY when using clang. Now linking target 'cxx_external_threads' with 'cxx-headers'. Fix mismatching visibility for libcpp_timed_backoff_policy function in file <__threading_support>.

Diff Detail

Event Timeline

Nicu created this revision.Aug 26 2020, 1:47 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptAug 26 2020, 1:47 AM
Nicu requested review of this revision.Aug 26 2020, 1:47 AM
miyuki added inline comments.Aug 26 2020, 5:54 AM
libcxx/include/__threading_support
277

Please remove stray changes (2 added/removed blank lines)

Nicu updated this revision to Diff 287954.Aug 26 2020, 6:23 AM

Removed previously added blank line.
Added previously removed blank line.

ldionne requested changes to this revision.Aug 26 2020, 1:58 PM
ldionne added inline comments.
libcxx/src/CMakeLists.txt
344

I believe it would be better to use target_link_libraries(cxx_external_threads PRIVATE cxx-headers). This will setup the right flags for using the current libc++ headers.

This revision now requires changes to proceed.Aug 26 2020, 1:58 PM
Nicu updated this revision to Diff 288239.Aug 27 2020, 2:12 AM
Nicu edited the summary of this revision. (Show Details)

Updated CMakeList.txt to link against 'cxx-headers' rather than just including 'libcxx/include'.
Updated commit's summary to reflect this change.

ldionne accepted this revision.Aug 27 2020, 7:47 AM
This revision is now accepted and ready to land.Aug 27 2020, 7:47 AM
This revision was automatically updated to reflect the committed changes.

Did you run the tests before committing this? It looks like it broke several builders, e.g. http://lab.llvm.org:8011/builders/libcxx-libcxxabi-x86_64-linux-debian-noexceptions/builds/2370

Sorry about that. I've reverted the change and will investigate.

Sorry about that. I've reverted the change and will investigate.

Thanks. I think the fix is just to remove the inline before _LIBCPP_THREAD_ABI_VISIBILITY, since _LIBCPP_THREAD_ABI_VISIBILITY already contains inline when not in the external threading library mode.

Nicu added a comment.Aug 27 2020, 9:15 AM

Sorry about that. I've reverted the change and will investigate.

Thanks. I think the fix is just to remove the inline before _LIBCPP_THREAD_ABI_VISIBILITY, since _LIBCPP_THREAD_ABI_VISIBILITY already contains inline when not in the external threading library mode.

My bad, I did run tests but using the BUILD_EXTERNAL_THREAD_LIBRARY mode only, and forgot about the normal config.
As for the fix, if I remove that inline it breaks the external threading mode, but in order to fix that do you think is safe to change line 43 in __threading_support to #define _LIBCPP_THREAD_ABI_VISIBILITY inline _LIBCPP_FUNC_VIS. Adding the inline there?

As for the fix, if I remove that inline it breaks the external threading mode, [...]

Actually, looking at it again, the fix is to always use _LIBCPP_INLINE_VISIBILITY. Please rebase your change on top of 21a1a263a6d9c0c44ef8eb0744786e2aa5d59e53

This seems to have been reverted in

commit a19fd1aab519ccec18654f76a01b0345880c5200
Author: Mikhail Maltsev <mikhail.maltsev@arm.com>
Date:   Thu Aug 27 16:47:18 2020 +0100

    Revert "[libcxx] Fix compile for BUILD_EXTERNAL_THREAD_LIBRARY"

    This reverts commit 3b71f91558ff8b569199547efe800cb501c3cf94.

    The commit is breaking some build bots.

Has it been committed again with a fix? I don't see it -- what's the status?

Nicu added a comment.Sep 1 2020, 1:04 AM

This seems to have been reverted in

commit a19fd1aab519ccec18654f76a01b0345880c5200
Author: Mikhail Maltsev <mikhail.maltsev@arm.com>
Date:   Thu Aug 27 16:47:18 2020 +0100

    Revert "[libcxx] Fix compile for BUILD_EXTERNAL_THREAD_LIBRARY"

    This reverts commit 3b71f91558ff8b569199547efe800cb501c3cf94.

    The commit is breaking some build bots.

Has it been committed again with a fix? I don't see it -- what's the status?

Hi Louis, this is the revision that fixes the compilation with EXTERNAL_LIBRARY (https://reviews.llvm.org/D86773), without changes to the visibility in the operator. As you fixed that already.

miyuki added a comment.Sep 1 2020, 4:48 AM

I've just committed D86773

Ah, I was confused, thanks for clarifying!