Page MenuHomePhabricator

[libcxx] __thread_id should be available when an external threading API is used.
AbandonedPublic

Authored by DavidSpickett on Aug 15 2019, 8:56 AM.

Details

Summary

Commit r368867 "Fix a layering violation in mutex"
moved "thread_id" from <thread> to <threading_support>.

Along the way the conditions for its presence changed.
Before it only required that threading was supported.
Now it needs that and you not to be using an external
API.

This change restores the old behaviour by putting it
in a separate block with the correct condition.

Diff Detail

Event Timeline

DavidSpickett created this revision.Aug 15 2019, 8:56 AM
zoecarver added inline comments.
libcxx/include/__threading_support
398

Any reason this cannot go at the top of the file before _LIBCPP_BEGIN_NAMESPACE_STD?

Yes. Because then it would be in the global namespace.

Yes. Because then it would be in the global namespace.

(assuming you were responding to me) Isn't that the intent? It looks like it's just conditionally adding an #include.

Isn't that the intent? It looks like it's just conditionally adding an #include.

No, that's not the intent. There may be some misunderstanding going on here so I'll have a go at explaining the intent in my own words (I worked with David on this patch).

Near the top of the file there's:

#if defined(_LIBCPP_HAS_THREAD_API_EXTERNAL)
# include <__external_threading>
#elif !defined(_LIBCPP_HAS_NO_THREADS)

this was followed by the rest of the code in the header. Since __thread_id et al. was moved into this header from another header (about a week ago), it's only compiled if neither _LIBCPP_HAS_THREAD_API_EXTERNAL nor _LIBCPP_HAS_NO_THREADS are defined whereas previously it had been compiled irrespective of _LIBCPP_HAS_THREAD_API_EXTERNAL.
To restore the previous behaviour, we're ending the #if block (#endif // !_LIBCPP_HAS_NO_THREADS and !_LIBCPP_HAS_THREAD_API_EXTERNAL) and starting a new one (#if !defined(_LIBCPP_HAS_NO_THREADS)), which removes the _LIBCPP_HAS_THREAD_API_EXTERNAL condition.

The _LIBCPP_END_NAMESPACE_STD stuff we're adding is only there to make sure there isn't a mismatch in the number of times we open or close the namespace, or push or pop macros.

Hope that makes things clearer.

Thank you for explaining. That makes a lot of sense.

I put an alternate solution up as D66480

DavidSpickett abandoned this revision.Aug 20 2019, 9:09 AM

Abandoned in favour of Marshall's fix.