sanitizer_common: Use void* for madvise first argument on Solaris.

[pcc]

@ro On D85870 you mentioned that the declaration has been changed in Solaris 11.4 to use void * which causes a build break on 11.4 due to the incompatible declaration. I checked illumos and it looks like their declaration still has caddr_t, although the comment above this declaration implies that that shouldn't matter because the system's declaration isn't visible. But what about older versions of Solaris? Presumably they still have a declaration with caddr_t so I imagine the build would break with an incompatible declaration error there. Or do we not care about older versions?

[ro]

In D86166#2224690, @pcc wrote:

@ro On D85870 you mentioned that the declaration has been changed in Solaris 11.4 to use void * which causes a build break on 11.4 due to the incompatible declaration. I checked illumos and it looks like their declaration still has caddr_t, although the comment above this declaration implies that that shouldn't matter because the system's declaration isn't visible. But what about older versions of Solaris? Presumably they still have a declaration with caddr_t so I imagine the build would break with an incompatible declaration error there. Or do we not care about older versions?

As was discussed in D84046, Solaris doesn't need an explicit declaration at all: with __EXTENSIONS__ defined, a declaration of madvise will be visible without doing anything special.

However, on Illumos with _XOPEN_SOURCE defined (as g++ does on Solaris), there is no way to make an madvise declaration visible. So on Illumos, it doesn't matter if it uses void * or caddr_t since there can be no conflict with a system declaration. However the declaration currently needs to agree with the Solaris one to avoid the build breakage.

Part of the trouble is that we currently cannot distinguish between Solaris and Illumos at all at compile time; I have a patch to define __illumos__ based on uname -o for cases where it's difficult otherwise. All a royal mess, but done for the benefit of Illumos, not for not caring about it.

With respect to other older versions of Solaris, the only one even remotely interesting (and the oldest one I still test in GCC) is 11.3. I have a bunch of patches to at least allow LLVM to compile, but there are so many issues (ld not wrapping sections in __start_<sec>/__stop_<sec> symbols, lack of __cxa_atexit, lack of fmemopen, lack of constructor priority support, and that's just the tip of the iceberg) that I strongly doubt there's any value in trying to pursue this further. In fact the 11.3 madvise declaration still using caddr_t prompted me to do the __illumos__ patch so an Illumos-only declaration wouldn't interfere with Solaris.

[pcc]

Add explanation to comment

[pcc]

In D86166#2224751, @ro wrote:

In D86166#2224690, @pcc wrote:

@ro On D85870 you mentioned that the declaration has been changed in Solaris 11.4 to use void * which causes a build break on 11.4 due to the incompatible declaration. I checked illumos and it looks like their declaration still has caddr_t, although the comment above this declaration implies that that shouldn't matter because the system's declaration isn't visible. But what about older versions of Solaris? Presumably they still have a declaration with caddr_t so I imagine the build would break with an incompatible declaration error there. Or do we not care about older versions?

As was discussed in D84046, Solaris doesn't need an explicit declaration at all: with __EXTENSIONS__ defined, a declaration of madvise will be visible without doing anything special.

However, on Illumos with _XOPEN_SOURCE defined (as g++ does on Solaris), there is no way to make an madvise declaration visible. So on Illumos, it doesn't matter if it uses void * or caddr_t since there can be no conflict with a system declaration. However the declaration currently needs to agree with the Solaris one to avoid the build breakage.

Part of the trouble is that we currently cannot distinguish between Solaris and Illumos at all at compile time; I have a patch to define __illumos__ based on uname -o for cases where it's difficult otherwise. All a royal mess, but done for the benefit of Illumos, not for not caring about it.

With respect to other older versions of Solaris, the only one even remotely interesting (and the oldest one I still test in GCC) is 11.3. I have a bunch of patches to at least allow LLVM to compile, but there are so many issues (ld not wrapping sections in __start_<sec>/__stop_<sec> symbols, lack of __cxa_atexit, lack of fmemopen, lack of constructor priority support, and that's just the tip of the iceberg) that I strongly doubt there's any value in trying to pursue this further. In fact the 11.3 madvise declaration still using caddr_t prompted me to do the __illumos__ patch so an Illumos-only declaration wouldn't interfere with Solaris.

Okay, thanks for that explanation, which I used to expand on the comment next to the declaration. Please let me know if it looks correct.

[ro]

Tested on sparcv9-sun-solaris2.11 and amd64-pc-solaris2.11 (both Solaris 11.4 and OpenIndiana).

It's quite a bit of verbiage for a snippet of code I'd rather restrict to Illumos only. However, if this avoids breaking this a third time, it's a comment well spent.

LGTM, Thanks.

compiler-rt/lib/sanitizer_common/sanitizer_solaris.cpp

 }
 
 DECLARE__REAL_AND_INTERNAL(int, mprotect, void *addr, uptr length, int prot) {
   return _REAL(mprotect)(addr, length, prot);
 }
 
 // Illumos' declaration of madvise cannot be made visible if _XOPEN_SOURCE
 // is defined as g++ does on Solaris.
-extern "C" int madvise(caddr_t, size_t, int);
+//
+// This declaration is consistent with Solaris 11.4. Both Illumos and Solaris
+// versions older than 11.4 declared madvise with a caddr_t as the first
+// argument, but we don't currently support Solaris versions older than 11.4,
+// and as mentioned above the declaration is not visible on Illumos so we can
+// use any declaration we like on Illumos.
+extern "C" int madvise(void *, size_t, int);
 
 int internal_madvise(uptr addr, uptr length, int advice) {
-  return madvise((caddr_t)addr, length, advice);
+  return madvise((void *)addr, length, advice);
 }
 
 DECLARE__REAL_AND_INTERNAL(uptr, close, fd_t fd) {
   return _REAL(close)(fd);
 }
 
 extern "C" int _REAL64(open)(const char *, int, ...);