Revert libsanitizer r318802 as we don't use Scudo allocator (PR sanitizer/87892).

Message ID adcf682b-bbb4-b07c-50a5-0231f6868414@suse.cz
State New
Headers show
Series
  • Revert libsanitizer r318802 as we don't use Scudo allocator (PR sanitizer/87892).
Related show

Commit Message

Martin Liška Nov. 8, 2018, 8:27 a.m.
Hi.

The GetNumberOfCPUs functionality is only used from Scudo allocator:
https://llvm.org/docs/ScudoHardenedAllocator.html

The hardening allocator is not used from GCC, thus I recommend to remove
the function.

Ready for trunk?
Martin

libsanitizer/ChangeLog:

2018-11-08  Martin Liska  <mliska@suse.cz>

	PR sanitizer/87892
	* (all files): Revert upstream r318802.
---
 .../sanitizer_common/sanitizer_common.cc      |  1 -
 .../sanitizer_common/sanitizer_common.h       |  9 ---
 .../sanitizer_common/sanitizer_fuchsia.cc     |  4 --
 .../sanitizer_linux_libcdep.cc                | 69 -------------------
 .../sanitizer_common/sanitizer_mac.cc         |  4 --
 .../sanitizer_common/sanitizer_win.cc         |  6 --
 6 files changed, 93 deletions(-)

Comments

Jeff Law Nov. 8, 2018, 8:43 p.m. | #1
On 11/8/18 1:27 AM, Martin Liška wrote:
> Hi.
> 
> The GetNumberOfCPUs functionality is only used from Scudo allocator:
> https://llvm.org/docs/ScudoHardenedAllocator.html
> 
> The hardening allocator is not used from GCC, thus I recommend to remove
> the function.
> 
> Ready for trunk?
> Martin
> 
> libsanitizer/ChangeLog:
> 
> 2018-11-08  Martin Liska  <mliska@suse.cz>
> 
> 	PR sanitizer/87892
> 	* (all files): Revert upstream r318802.
Is it causing a build failure or somesuch?  ie, why specifically are you
wanting to remove it?

jeff
Jakub Jelinek Nov. 8, 2018, 8:48 p.m. | #2
On Thu, Nov 08, 2018 at 01:43:29PM -0700, Jeff Law wrote:
> On 11/8/18 1:27 AM, Martin Liška wrote:
> > libsanitizer/ChangeLog:
> > 
> > 2018-11-08  Martin Liska  <mliska@suse.cz>
> > 
> > 	PR sanitizer/87892
> > 	* (all files): Revert upstream r318802.
> Is it causing a build failure or somesuch?  ie, why specifically are you
> wanting to remove it?

Yes.  But perhaps it would be enough to just guard the CPU_COUNT use with
#ifdef CPU_COUNT and have some fallback, including say returning just 1.
If we care about Scudo or whatever is (what would be needed for that on the
compiler side?), then we'd need a proper implementation, one that doesn't
fail if a machine has more CPUs than fit into cpu_set_t, or if old glibc is
used and CPU_COUNT isn't defined, or even if the kernel doesn't have
affinity stuff at all.

	Jakub
Jeff Law Nov. 8, 2018, 8:50 p.m. | #3
On 11/8/18 1:48 PM, Jakub Jelinek wrote:
> On Thu, Nov 08, 2018 at 01:43:29PM -0700, Jeff Law wrote:
>> On 11/8/18 1:27 AM, Martin Liška wrote:
>>> libsanitizer/ChangeLog:
>>>
>>> 2018-11-08  Martin Liska  <mliska@suse.cz>
>>>
>>> 	PR sanitizer/87892
>>> 	* (all files): Revert upstream r318802.
>> Is it causing a build failure or somesuch?  ie, why specifically are you
>> wanting to remove it?
> 
> Yes.  But perhaps it would be enough to just guard the CPU_COUNT use with
> #ifdef CPU_COUNT and have some fallback, including say returning just 1.
> If we care about Scudo or whatever is (what would be needed for that on the
> compiler side?), then we'd need a proper implementation, one that doesn't
> fail if a machine has more CPUs than fit into cpu_set_t, or if old glibc is
> used and CPU_COUNT isn't defined, or even if the kernel doesn't have
> affinity stuff at all.
The obvious idea being to disable it with a more minimal patch making
future merges easier -- if there's a less intrusive way to disable the
bits, then that's fine with me.

jeff
Martin Liška Nov. 9, 2018, 9:12 a.m. | #4
On 11/8/18 9:50 PM, Jeff Law wrote:
> On 11/8/18 1:48 PM, Jakub Jelinek wrote:
>> On Thu, Nov 08, 2018 at 01:43:29PM -0700, Jeff Law wrote:
>>> On 11/8/18 1:27 AM, Martin Liška wrote:
>>>> libsanitizer/ChangeLog:
>>>>
>>>> 2018-11-08  Martin Liska  <mliska@suse.cz>
>>>>
>>>> 	PR sanitizer/87892
>>>> 	* (all files): Revert upstream r318802.
>>> Is it causing a build failure or somesuch?  ie, why specifically are you
>>> wanting to remove it?
>>
>> Yes.  But perhaps it would be enough to just guard the CPU_COUNT use with
>> #ifdef CPU_COUNT and have some fallback, including say returning just 1.
>> If we care about Scudo or whatever is (what would be needed for that on the
>> compiler side?), then we'd need a proper implementation, one that doesn't
>> fail if a machine has more CPUs than fit into cpu_set_t, or if old glibc is
>> used and CPU_COUNT isn't defined, or even if the kernel doesn't have
>> affinity stuff at all.
> The obvious idea being to disable it with a more minimal patch making
> future merges easier -- if there's a less intrusive way to disable the
> bits, then that's fine with me.
> 
> jeff
> 

Ok, then I'm going to install following patch.

Martin
From 40766a658e15019c8fd03f678a988ce53c2495b1 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Fri, 9 Nov 2018 09:44:15 +0100
Subject: [PATCH] Fallback in libsanitizer for scudo sanitizer (PR
 sanitizer/87892).

libsanitizer/ChangeLog:

2018-11-09  Martin Liska  <mliska@suse.cz>

	PR sanitizer/87892
	* sanitizer_common/sanitizer_linux_libcdep.cc (defined): Return
	1 when CPU_COUNT macro is not defined.
---
 libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc b/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
index 32f335eaf23..28360f5656a 100644
--- a/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
+++ b/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
@@ -694,9 +694,13 @@ u32 GetNumberOfCPUs() {
 #elif SANITIZER_SOLARIS
   return sysconf(_SC_NPROCESSORS_ONLN);
 #else
+#if defined(CPU_COUNT)
   cpu_set_t CPUs;
   CHECK_EQ(sched_getaffinity(0, sizeof(cpu_set_t), &CPUs), 0);
   return CPU_COUNT(&CPUs);
+#else
+  return 1;
+#endif
 #endif
 }
Jakub Jelinek Nov. 9, 2018, 9:14 a.m. | #5
On Fri, Nov 09, 2018 at 10:12:32AM +0100, Martin Liška wrote:
> Ok, then I'm going to install following patch.

Thanks.

> 2018-11-09  Martin Liska  <mliska@suse.cz>
> 
> 	PR sanitizer/87892
> 	* sanitizer_common/sanitizer_linux_libcdep.cc (defined): Return
> 	1 when CPU_COUNT macro is not defined.
> ---
>  libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc b/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
> index 32f335eaf23..28360f5656a 100644
> --- a/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
> +++ b/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
> @@ -694,9 +694,13 @@ u32 GetNumberOfCPUs() {
>  #elif SANITIZER_SOLARIS
>    return sysconf(_SC_NPROCESSORS_ONLN);
>  #else
> +#if defined(CPU_COUNT)
>    cpu_set_t CPUs;
>    CHECK_EQ(sched_getaffinity(0, sizeof(cpu_set_t), &CPUs), 0);
>    return CPU_COUNT(&CPUs);
> +#else
> +  return 1;
> +#endif
>  #endif
>  }
>  
> -- 
> 2.19.1
> 

	Jakub

Patch

diff --git a/libsanitizer/sanitizer_common/sanitizer_common.cc b/libsanitizer/sanitizer_common/sanitizer_common.cc
index 7f0f47c005d..29f35bb599a 100644
--- a/libsanitizer/sanitizer_common/sanitizer_common.cc
+++ b/libsanitizer/sanitizer_common/sanitizer_common.cc
@@ -23,7 +23,6 @@  const char *SanitizerToolName = "SanitizerTool";
 
 atomic_uint32_t current_verbosity;
 uptr PageSizeCached;
-u32 NumberOfCPUsCached;
 
 // PID of the tracer task in StopTheWorld. It shares the address space with the
 // main process, but has a different PID and thus requires special handling.
diff --git a/libsanitizer/sanitizer_common/sanitizer_common.h b/libsanitizer/sanitizer_common/sanitizer_common.h
index 603d922b969..d57434ae1dd 100644
--- a/libsanitizer/sanitizer_common/sanitizer_common.h
+++ b/libsanitizer/sanitizer_common/sanitizer_common.h
@@ -939,15 +939,6 @@  void CheckNoDeepBind(const char *filename, int flag);
 // be used to seed a PRNG. Defaults to blocking like the underlying syscall.
 bool GetRandom(void *buffer, uptr length, bool blocking = true);
 
-// Returns the number of logical processors on the system.
-u32 GetNumberOfCPUs();
-extern u32 NumberOfCPUsCached;
-INLINE u32 GetNumberOfCPUsCached() {
-  if (!NumberOfCPUsCached)
-    NumberOfCPUsCached = GetNumberOfCPUs();
-  return NumberOfCPUsCached;
-}
-
 }  // namespace __sanitizer
 
 inline void *operator new(__sanitizer::operator_new_size_type size,
diff --git a/libsanitizer/sanitizer_common/sanitizer_fuchsia.cc b/libsanitizer/sanitizer_common/sanitizer_fuchsia.cc
index 6602f97b40b..d38151d2711 100644
--- a/libsanitizer/sanitizer_common/sanitizer_fuchsia.cc
+++ b/libsanitizer/sanitizer_common/sanitizer_fuchsia.cc
@@ -482,10 +482,6 @@  bool GetRandom(void *buffer, uptr length, bool blocking) {
   return true;
 }
 
-u32 GetNumberOfCPUs() {
-  return zx_system_get_num_cpus();
-}
-
 uptr GetRSS() { UNIMPLEMENTED(); }
 
 }  // namespace __sanitizer
diff --git a/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc b/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
index 32f335eaf23..0cdc97111f4 100644
--- a/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
+++ b/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
@@ -35,17 +35,14 @@ 
 #if SANITIZER_FREEBSD
 #include <pthread_np.h>
 #include <osreldate.h>
-#include <sys/sysctl.h>
 #define pthread_getattr_np pthread_attr_get_np
 #endif
 
 #if SANITIZER_OPENBSD
 #include <pthread_np.h>
-#include <sys/sysctl.h>
 #endif
 
 #if SANITIZER_NETBSD
-#include <sys/sysctl.h>
 #include <sys/tls.h>
 #endif
 
@@ -55,16 +52,6 @@ 
 
 #if SANITIZER_ANDROID
 #include <android/api-level.h>
-#if !defined(CPU_COUNT) && !defined(__aarch64__)
-#include <dirent.h>
-#include <fcntl.h>
-struct __sanitizer::linux_dirent {
-  long           d_ino;
-  off_t          d_off;
-  unsigned short d_reclen;
-  char           d_name[];
-};
-#endif
 #endif
 
 #if !SANITIZER_ANDROID
@@ -644,62 +631,6 @@  uptr GetRSS() {
   return rss * GetPageSizeCached();
 }
 
-// sysconf(_SC_NPROCESSORS_{CONF,ONLN}) cannot be used on most platforms as
-// they allocate memory.
-u32 GetNumberOfCPUs() {
-#if SANITIZER_FREEBSD || SANITIZER_NETBSD || SANITIZER_OPENBSD
-  u32 ncpu;
-  int req[2];
-  uptr len = sizeof(ncpu);
-  req[0] = CTL_HW;
-  req[1] = HW_NCPU;
-  CHECK_EQ(internal_sysctl(req, 2, &ncpu, &len, NULL, 0), 0);
-  return ncpu;
-#elif SANITIZER_ANDROID && !defined(CPU_COUNT) && !defined(__aarch64__)
-  // Fall back to /sys/devices/system/cpu on Android when cpu_set_t doesn't
-  // exist in sched.h. That is the case for toolchains generated with older
-  // NDKs.
-  // This code doesn't work on AArch64 because internal_getdents makes use of
-  // the 64bit getdents syscall, but cpu_set_t seems to always exist on AArch64.
-  uptr fd = internal_open("/sys/devices/system/cpu", O_RDONLY | O_DIRECTORY);
-  if (internal_iserror(fd))
-    return 0;
-  InternalMmapVector<u8> buffer(4096);
-  uptr bytes_read = buffer.size();
-  uptr n_cpus = 0;
-  u8 *d_type;
-  struct linux_dirent *entry = (struct linux_dirent *)&buffer[bytes_read];
-  while (true) {
-    if ((u8 *)entry >= &buffer[bytes_read]) {
-      bytes_read = internal_getdents(fd, (struct linux_dirent *)buffer.data(),
-                                     buffer.size());
-      if (internal_iserror(bytes_read) || !bytes_read)
-        break;
-      entry = (struct linux_dirent *)buffer.data();
-    }
-    d_type = (u8 *)entry + entry->d_reclen - 1;
-    if (d_type >= &buffer[bytes_read] ||
-        (u8 *)&entry->d_name[3] >= &buffer[bytes_read])
-      break;
-    if (entry->d_ino != 0 && *d_type == DT_DIR) {
-      if (entry->d_name[0] == 'c' && entry->d_name[1] == 'p' &&
-          entry->d_name[2] == 'u' &&
-          entry->d_name[3] >= '0' && entry->d_name[3] <= '9')
-        n_cpus++;
-    }
-    entry = (struct linux_dirent *)(((u8 *)entry) + entry->d_reclen);
-  }
-  internal_close(fd);
-  return n_cpus;
-#elif SANITIZER_SOLARIS
-  return sysconf(_SC_NPROCESSORS_ONLN);
-#else
-  cpu_set_t CPUs;
-  CHECK_EQ(sched_getaffinity(0, sizeof(cpu_set_t), &CPUs), 0);
-  return CPU_COUNT(&CPUs);
-#endif
-}
-
 #if SANITIZER_LINUX
 
 # if SANITIZER_ANDROID
diff --git a/libsanitizer/sanitizer_common/sanitizer_mac.cc b/libsanitizer/sanitizer_common/sanitizer_mac.cc
index 28b2906e226..bd4949c1b75 100644
--- a/libsanitizer/sanitizer_common/sanitizer_mac.cc
+++ b/libsanitizer/sanitizer_common/sanitizer_mac.cc
@@ -1082,10 +1082,6 @@  bool GetRandom(void *buffer, uptr length, bool blocking) {
   return true;
 }
 
-u32 GetNumberOfCPUs() {
-  return (u32)sysconf(_SC_NPROCESSORS_ONLN);
-}
-
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_MAC
diff --git a/libsanitizer/sanitizer_common/sanitizer_win.cc b/libsanitizer/sanitizer_common/sanitizer_win.cc
index ebc6c503036..74c04050a7a 100644
--- a/libsanitizer/sanitizer_common/sanitizer_win.cc
+++ b/libsanitizer/sanitizer_common/sanitizer_win.cc
@@ -1047,12 +1047,6 @@  bool GetRandom(void *buffer, uptr length, bool blocking) {
   UNIMPLEMENTED();
 }
 
-u32 GetNumberOfCPUs() {
-  SYSTEM_INFO sysinfo = {};
-  GetNativeSystemInfo(&sysinfo);
-  return sysinfo.dwNumberOfProcessors;
-}
-
 }  // namespace __sanitizer
 
 #endif  // _WIN32