This is an archive of the discontinued LLVM Phabricator instance.

Refactor sanitizer_tls_get_addr.cpp into its own library
AbandonedPublic

Authored by thurston on Apr 11 2023, 3:31 PM.

Details

Reviewers
vitalybuka
Summary

sanitizer_tls_get_addr.cpp is currently part of SANITIZER_SOURCES_NOTERMINATION, which means it gets compiled into nearly everything, even if it isn’t actually needed. This is problematic when we try to change sanitizer_tls_get_addr() to call internal allocator functions (e.g., __sanitizer_get_allocated_begin() in https://reviews.llvm.org/D147459), because some targets (dyndd, ubsan, MemProfUnitTests) do not have an allocator.

To avoid the need for weak annotations for the internal allocator functions, this patch moves sanitizer_tls_get_addr() into its own library, and updates the CMakeLists to add the library only if needed.

Diff Detail

Event Timeline

thurston created this revision.Apr 11 2023, 3:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2023, 3:31 PM
Herald added a subscriber: Enna1. · View Herald Transcript
thurston requested review of this revision.Apr 11 2023, 3:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2023, 3:31 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

you can try to update llvm-project/llvm/utils/gn/secondary/compiler-rt/lib/ubsan/BUILD.gn and llvm-project/llvm/utils/gn/secondary/compiler-rt/lib/sanitizer_common/BUILD.gn

maybe just this?

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_allocator_interface.h b/compiler-rt/lib/sanitizer_common/sanitizer_allocator_interface.h
index d372f7750702..41e924932323 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_allocator_interface.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_allocator_interface.h
@@ -21,9 +21,9 @@ extern "C" {
 SANITIZER_INTERFACE_ATTRIBUTE
 uptr __sanitizer_get_estimated_allocated_size(uptr size);
 SANITIZER_INTERFACE_ATTRIBUTE int __sanitizer_get_ownership(const void *p);
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE void *
+SANITIZER_INTERFACE_ATTRIBUTE void *
 __sanitizer_get_allocated_begin(const void *p);
-SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE uptr
+SANITIZER_INTERFACE_ATTRIBUTE uptr
 __sanitizer_get_allocated_size(const void *p);
 SANITIZER_INTERFACE_ATTRIBUTE uptr __sanitizer_get_current_allocated_bytes();
 SANITIZER_INTERFACE_ATTRIBUTE uptr __sanitizer_get_heap_size();
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_tls_get_addr.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_tls_get_addr.cpp
index a8c8acdfb7e6..8d31675a2b66 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_tls_get_addr.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_tls_get_addr.cpp
@@ -102,6 +102,15 @@ static const uptr kDtvOffset = 0x800;
 static const uptr kDtvOffset = 0;
 #endif
 
+extern "C" {
+SANITIZER_WEAK_ATTRIBUTE
+uptr
+__sanitizer_get_allocated_size(const void *p);
+
+SANITIZER_WEAK_ATTRIBUTE
+void *__sanitizer_get_allocated_begin(const void *p);
+}
+
 DTLS::DTV *DTLS_on_tls_get_addr(void *arg_void, void *res,
                                 uptr static_tls_begin, uptr static_tls_end) {
   if (!common_flags()->intercept_tls_get_addr) return 0;
vitalybuka accepted this revision.Apr 11 2023, 5:14 PM

I guess either wait is fine, but SANITIZER_WEAK_ATTRIBUTE (in sanitizer_tls_get_addr, not the header) is simpler to maintain

This revision is now accepted and ready to land.Apr 11 2023, 5:14 PM
thurston abandoned this revision.Apr 11 2023, 9:18 PM

Thanks, your alternative approach is much more elegant! I'll abandon this revision and incorporate your suggested code into D147459.