Enable weak hooks on darwin

[fjricci]

Summary

By default, darwin requires a definition for weak interface functions at
link time. Adding the '-U' link flag with each weak function allows these
weak interface functions to be used without definitions, which mirrors
behavior on linux and windows.

[kubamracek]

-undefined dynamic_lookup

This means the linker won't warn/error out on any undefined symbol. Sounds very dangerous to me and it's likely to cause a lot of development headaches.

Can we solve this in a different way? E.g. a macro that would expand to a empty function body on Darwin?

[compnerd]

Its actually not that dangerous, given that ELF by default does this for any dynamic linking. However, it is possible to be more granular and specify the symbols which are okay to be undefined using the -U <name> option to the linker. It is slightly nicer than the blanket approach, but at a higher maintainence cost (any time the sanitizers change the hooks, it would need to be updated. But, that doesnt seem too bad.

[fjricci]

Upload correct commit

[fjricci]

Individually lists each undefined symbol as undefined in the linker invocation.

[fjricci]

Enable on darwin

[kubamracek]

Looks better, but I'd rather see the list of weak symbols in each subdirectory (asan/, ubsan/, etc.), because each sanitizer has its own list. It looks wrong to list sanitizer-specific symbols in a global CMake file. Can this also be stored in a separate file and not directly in CMake (with a dependency on that file)?

Can we also make the Linux linker use this list? I only looked briefly at man ld, but it seems that --dynamic-list may achieve the same?

Overall, I support this change, but I want to avoid situations where a weak symbol is forgotten to be added to the list, and the Linux build will work fine, but Darwin will be broken.

[compnerd]

Ish. Most linkers can read the arguments from a file. The GNU linker uses @file syntax as does link (Windows). ld64 can read a set of files to link via -file_list but not all the arguments AFAIK.

COFF does not allow any undefined symbols anyways, and this patch addresses MachO. On ELFish targets, -z defs will prevent undefined symbols even in DSOs, but I don't think that applies to STB_WEAK symbols.

FWIW, I think that getting the same behavior across the two is a bit beyond the scope of this change.

[fjricci]

As far as I can tell, @compnerd is correct with respect to STB_WEAK symbols on Linux. I'll upload a refactored change targeting Darwin with the weak symbols pushed down into the sanitizer-specific directories.

[fjricci]

Refactor symbols into text files only included in required link steps

[fjricci]

Accidentally committed with another commit. Reverting.

[kubamracek]

LGTM with a nit.

Note that D28359 was trying to solve a similar problem with weak linking on Windows, so maybe there are more changes coming, but this patch doesn't need to be blocked on that.

cmake/Modules/SanitizerUtils.cmake

53 ↗

(On Diff #83423)

nit: add a comment saying that this is specific to Darwin/Mach-O

lib/sanitizer_common/sanitizer_internal_defs.h

35 ↗

(On Diff #83423)

What about FreeBSD? Just curious.

[compnerd]

lib/sanitizer_common/sanitizer_internal_defs.h

35 ↗

(On Diff #83423)

I believe that FreeBSD uses ELF, so it should be able to support weak linkage. However, if its untested ...

[fjricci]

cmake/Modules/SanitizerUtils.cmake

53 ↗

(On Diff #83423)

Will do before commit.

lib/sanitizer_common/sanitizer_internal_defs.h

35 ↗

(On Diff #83423)

I don't have any way to run this on FreeBSD, so I'll leave this for future work.

[fjricci]

Adds undefined symbols to test cases as well

[fjricci]

Also add function comment

[fjricci]

I realized during testing that the symbols must also be added to some test cases directly. Re-uploaded with those changes, to give the opportunity for review before submitting.

[fjricci]

Fix typo

[kubamracek]

There are still a few typos where LINK_FLAGS (with the underscore) is used.

Besides that, the patch looks good.

cmake/config-ix.cmake

if(APPLE)
   # We're setting the flag manually for each target OS
   set(CMAKE_OSX_DEPLOYMENT_TARGET "")
   
   set(DARWIN_COMMON_CFLAGS -stdlib=libc++)
   set(DARWIN_COMMON_LINKFLAGS
     -stdlib=libc++
     -lc++
     -lc++abi)
-  
+  set(DARWIN_WEAK_SYMBOLS
+      ___asan_default_options
+      ___asan_default_suppressions
+      ___asan_on_error
+      ___lsan_default_suppressions
+      ___lsan_is_turned_off
+      ___sanitizer_free_hook
+      ___sanitizer_malloc_hook
+      ___sanitizer_symbolize_code
+      ___sanitizer_symbolize_data
+      ___sanitizer_symbolize_demangle
+      ___sanitizer_symbolize_flush
+      ___ubsan_default_options)
+
+  foreach(SYMBOL ${DARWIN_WEAK_SYMBOLS})
+    set(DARWIN_COMMON_LINKFLAGS ${DARWIN_COMMON_LINKFLAGS} -Wl,-U,${SYMBOL})
+  endforeach()
+  
   check_linker_flag("-fapplication-extension" COMPILER_RT_HAS_APP_EXTENSION)
   if(COMPILER_RT_HAS_APP_EXTENSION)
     list(APPEND DARWIN_COMMON_LINKFLAGS "-fapplication-extension")
   endif()
 
   set(DARWIN_osx_CFLAGS
     ${DARWIN_COMMON_CFLAGS}
     -mmacosx-version-min=${SANITIZER_MIN_OSX_VERSION})