[PATCH] D40898: [Sanitizers] Basic sanitizer Solaris support (PR 33274)

[Rainer Orth via Phabricator via llvm-commits]

ro added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_internal_defs.h:94
 # define SANITIZER_CAN_USE_PREINIT_ARRAY 1
+// Before Solaris 11.4, .preinit_array is fully supported only with GNU ld.
+// FIXME: Check for those conditions.
----------------
krytarowski wrote:
> Is this supported in SmartOS?
I cannot tell: .preinit_array support was already in OpenSolaris
and is thus in the Illumos derivatives.  However, when I tried
to switch gcc to .init_array and friends, there were issues that
had to be resolved first (don't remember the details).

If this would affect the current .preinit_array use at all I don't know.

This being a volunteer effort, I have neither the time nor the
hardware to do such testing myself (and on OmniOS and 
OpenIndiana), so this will require help from those communities.


================
Comment at: lib/sanitizer_common/sanitizer_linux.cc:482
 // Like getenv, but reads env directly from /proc (on Linux) or parses the
-// 'environ' array (on FreeBSD) and does not use libc. This function should be
-// called first inside __asan_init.
+// 'environ' array (on FreeBSD and Solaris) and does not use libc. This
+// function should be called first inside __asan_init.
----------------
krytarowski wrote:
> I would make this list of OSes generic (we could include there NetBSD as well).
Fine with me: it would be way more readable (and maintainable)
to use generic (say) USE_ENVIRON macros than maintaining
long SANITIZER_THIS || SANITIZER_THAT lists all over the 
place ;-)


================
Comment at: lib/sanitizer_common/sanitizer_platform_limits_posix.cc:17
 
-#if SANITIZER_LINUX || SANITIZER_FREEBSD || SANITIZER_MAC
+#if SANITIZER_LINUX || SANITIZER_FREEBSD || SANITIZER_MAC || SANITIZER_SOLARIS
 // Tests in this file assume that off_t-dependent data structures match the
----------------
krytarowski wrote:
> When I was adding NetBSD support I was prompted to move all the NetBSD definitions to a dedicated file: sanitizer_platform_limits_netbsd.cc. I think the same should be repeated for Solaris.
I see.  When I started this port, that file (and convention)
didn't exist, I belive.

At first glance this seems still quite repetitive, but it's certainly
way clearer than the current #if/#ifdef maze.  I'll have a look,
thanks.


================
Comment at: lib/sanitizer_common/sanitizer_posix_libcdep.cc:65
   if (beg_aligned < end_aligned)
+#if SANITIZER_SOLARIS
+    // In the default compilation environment, madvise() is declared to take a
----------------
krytarowski wrote:
> Can we cast all systems to (char*)?
Only testing will tell for certain, but I suspect so.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D40898