[PATCH] D86166: sanitizer_common: Use void* for madvise first argument on Solaris.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In D86166#2224751 <https://reviews.llvm.org/D86166#2224751>, @ro wrote:

> In D86166#2224690 <https://reviews.llvm.org/D86166#2224690>, @pcc wrote:
>
>> @ro On D85870 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86166/new/

https://reviews.llvm.org/D86166