Rename TARGET_DARWIN to TARGET_OSX #33959
Conversation
dotnet#33716 set TARGET_DARWIN for iOS too but most of the logic is really OSX-specific. Unset TARGET_DARWIN for iOS and rename the rest of the occurrences to TARGET_OSX.
| if(NOT CLR_CMAKE_TARGET_ARCH_WASM AND NOT CLR_CMAKE_TARGET_IOS) | ||
| if(CLR_CMAKE_TARGET_OSX) | ||
| add_definitions(-DTARGET_OSX) | ||
| add_link_options(-Wl,-bind_at_load) |
There was a problem hiding this comment.
@janvorli I see that you have added -bind_at_load in https://github.com/dotnet/runtime/pull/685/files#diff-1e019a64c92fd71addd77bc8288ca11fR174 . Was it intentional? Why is it needed?
It looks like a unnecessary de-optimization to me.
There was a problem hiding this comment.
The -bind_at_load is an equivalent of -z,now that we use on Linux.
From https://gcc.gnu.org/onlinedocs/gcc-4.7.4/gcc/Darwin-Options.html:
-bind_at_load
Causes the output file to be marked such that the dynamic linker will bind all undefined references when the file is loaded or launched.
From https://linux.die.net/man/1/ld
now
When generating an executable or shared library, mark it to tell the dynamic linker to resolve all symbols when the program is started, or when the shared library is linked to using dlopen, instead of deferring function call resolution to the point when the function is first called.
There was a problem hiding this comment.
I understand what it does. I do not understand why we need it. What is not going to work when we remove -z,now and -bind_at_load ?
There was a problem hiding this comment.
It is a hardening feature. -z,relro enables just a partial relro. To enable full relro, both -z,relro and -z,now is needed. See https://www.redhat.com/en/blog/hardening-elf-binaries-using-relocation-read-only-relro. GOT ovewrite attacks are still possible with partial relro, but not with full relro.
| @@ -213,8 +213,6 @@ extern "C" { | |||
| // includes are not included, so we need to define them. | |||
| #ifndef PAL_IMPLEMENTATION | |||
|
|
|||
| // OS X already defines these types in 64 bit | |||
| #if !defined(TARGET_OSX) | |||
There was a problem hiding this comment.
The PR uncovered an interesting issue: before this PR TARGET_OSX was already set in src/coreclr/src/pal/src/CMakeLists.txt and used here, but not set when accessed from other object files.
Now that TARGET_OSX is defined globally this code here got disabled which resulted in a compilation error:
In file included from /Users/alexander/dev/runtime/src/coreclr/src/nativeresources/resourcestring.cpp:7:
In file included from /Users/alexander/dev/runtime/src/coreclr/src/inc/utilcode.h:25:
/Users/alexander/dev/runtime/src/coreclr/src/inc/daccess.h:2350:14: error: unknown type name 'uint32_t'
typedef DPTR(uint32_t) PTR_uint32_t;
^
Removing the #if seems to work fine and should match the earlier behavior but I'd appreciate a second set of eyes. @jkotas
There was a problem hiding this comment.
It is fine since it is under #ifndef PAL_IMPLEMENTATION and so it can never collide with platform header files. We used to build part of PAL, the safecrt, without this define in the past, but it was fixed about a week ago in #32800, so the define doesn't make sense here anyways.
iOS requires 3.14.5 version and we have a special case for it. Since this is a minor version difference, and no distro is particularly providing cmake 3.14.2 package, it is safe to update to 3.14.5 to cover our supported platform matrix. Discussion: dotnet/runtime#33959 (comment) Fixes: dotnet/runtime#33976 cc @akoeplinger, @jkotas
iOS requires 3.14.5 version and we have a special case for it. Since this is a minor version difference, and no distro is particularly providing cmake 3.14.2 package, it is safe to update to 3.14.5 to cover our supported platform matrix. Discussion: #33959 (comment) Fixes: #33976 Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
ghost
locked as resolved and limited conversation to collaborators
#33716 set TARGET_DARWIN for iOS too but most of the logic is really OSX-specific.
Unset TARGET_DARWIN for iOS and rename the rest of the occurrences to TARGET_OSX.