Page MenuHomePhabricator

[libcxxabi] Fix build of cxa_guard.cpp on configurations with _LIBCXXABI_HAS_NO_THREADS
AbandonedPublic

Authored by JamesNagurne on Apr 24 2019, 1:07 PM.

Details

Reviewers
EricWF
Summary

Fix build of cxa_guard.cpp on configurations with _LIBCXXABI_HAS_NO_THREADS

cxa_guard_impl.h currently contains a forward declaration of two classes,
LibcppMutex and LibcppCondVar, whose definitions are guarded by
!defined(_LIBCXXABI_HAS_NO_THREADS). When this configuration is set, the
definitions are not seen, and the compiler emits an error when the
incomplete types are used.

I've refined the locations of the guards around these definitions to only the
spots where not having thread support matters, and have stubbed those
definitions with the expected return code.

Diff Detail

Event Timeline

JamesNagurne created this revision.Apr 24 2019, 1:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2019, 1:07 PM

This is an example of a solution, but I'm not married to the mechanisms by which the results are achieved:

  • If you note the definition of CurrentImplementation on line 555, there's no path by which GlobalLock is used when _LIBCXXABI_HAS_NO_THREADS is set. Thus, we could alternatively guard the entirety of the implementation of InitByteGlobalMutex and insert a stub for the class.
  • The code could use helpers that either call __libcpp_mutex_*/_libcpp_cond_* or return 0, so that the implementation of LibcppCondVar and LibcppMutex are clear of preprocessor directives.

Hi James,

It's embarrassing we don't have test coverage for this. I've filed llvm.org/PR41596 to add buildbots for this configuration.
Sorry for the breakage and thanks for the patch.

libcxxabi/src/cxa_guard_impl.h
241

We just need to make the types complete. They don't need a mock interface. (In fact they shouldn't have one).

How about this:

  1. Remove the forward declarations.
  2. #ifdef out the entire body of LibcppMutex and LibcppCondVar when threads are disabled.

Ex:

struct LibcppMutex {
#ifndef _LIBCXXABI_HAS_NO_THREADS
  ...
#endif
};
JamesNagurne added inline comments.Apr 25 2019, 8:57 AM
libcxxabi/src/cxa_guard_impl.h
241

It looks like someone already made a commit. to fix this. Do you still want this modification to my patch?
I apologize for the delayed response. The responses came during my after-hours.

sbc100 added a subscriber: sbc100.Apr 25 2019, 9:43 AM

Looks like someone already landed a fix: rL359175

I saw that too. Not really too thrilled with the fix itself, since it further fragments the source with directives, but it works.
I do have another question for @EricWF regarding the inclusion of unistd.h, but I should probably take that to a separate forum (though I've commented on the original review that added the inclusion).

Shall I cancel this review? This is my first so I'm just starting to learn the ropes :)

JamesNagurne abandoned this revision.Apr 26 2019, 10:32 AM

Closing this, as a fix has already been committed and validated on our end.