Skip to content

Commit

Permalink
This reverts 0e5d4ea to fix the build on MacOSX
Browse files Browse the repository at this point in the history 
On Mac thread_local and __thread are not ABI compatible, in addition, thread_local
comes with additional overhead, __thread seems to be the most suitable linkage to use
regardless of c++/c
  • Loading branch information
@krakjoe
krakjoe committed Jun 11, 2019
1 parent 9ecc0a4 commit 69190ce
Showing 1 changed file with 3 additions and 7 deletions.
10 changes: 3 additions & 7 deletions TSRM/TSRM.h
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -159,14 +159,10 @@ TSRM_API void *tsrm_get_ls_cache(void);
TSRM_API uint8_t tsrm_is_main_thread(void); TSRM_API uint8_t tsrm_is_main_thread(void);
TSRM_API const char *tsrm_api_name(void); TSRM_API const char *tsrm_api_name(void);


#if defined(__cplusplus) && __cplusplus > 199711L #ifdef TSRM_WIN32
# define TSRM_TLS thread_local # define TSRM_TLS __declspec(thread)
#else #else
# ifdef TSRM_WIN32 # define TSRM_TLS __thread
# define TSRM_TLS __declspec(thread)
# else
# define TSRM_TLS __thread
# endif
#endif #endif


#define TSRM_SHUFFLE_RSRC_ID(rsrc_id) ((rsrc_id)+1) #define TSRM_SHUFFLE_RSRC_ID(rsrc_id) ((rsrc_id)+1)
Expand Down

4 comments on commit 69190ce

@weltling
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krakjoe what was the original issue, could you link please? This seems to be a step back in proper modern C++ support.

Thanks.

@nikic
Copy link
Member

@nikic nikic commented on 69190ce Jun 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weltling Linker error on macos:

clang: warning: argument unused during compilation: '-pthread' [-Wunused-command-line-argument]
Undefined symbols for architecture x86_64:
  "thread-local wrapper routine for _tsrm_ls_cache", referenced from:
      _zim_IntlDateFormatter___construct in dateformat_create.o
      _timezone_convert_to_datetimezone in timezone_class.o
      _zif_intlcal_from_date_time in calendar_methods.o
      _zif_intlcal_to_date_time in calendar_methods.o
      _php_intlgregcal_constructor_body(_zend_execute_data*, _zval_struct*, unsigned char) in gregoriancalendar_methods.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [sapi/cli/php] Error 1

If some files are compiled as C and some as C++, some will use __thread and some thread_local, which is an ABI violation.

@weltling
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic thanks for the snippet. Clang on Mac is always a different beast, but in this case the issue might be to be C linkage in a C++ file. Removing U_CFUNC or rewriting _php_intlgregcal_constructor_body to use C linkage as well could fixed it. Unfortunately no chance to test on Mac, but used at other places like https://github.com/weltling/parle/blob/master/parle.cpp#L278 even C++14 didn't show issues. The _tsrm_ls_cache pointer is delivered by a function call, it's per DSO, so no issues would be expected if there's a clean separation.

Clearly we're anyway far from having a good C++ API and it's too bad there're not many to develop on Mac, Solaris and so on. But it makes that even more far by preventing writing a clean C++ code eventually :( At quite some places Clang is platform dependent, not sure it's possible to enforce it to be as strict as possible. The point is just, IMO, it would make sense to strive better C++ compat as it moves forward quite rapidly.

Thanks.

@sergeyklay
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.