Page MenuHomePhabricator

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

Authored by pcc on Aug 18 2020, 1:10 PM.

Diff Detail

Event Timeline

pcc created this revision.Aug 18 2020, 1:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2020, 1:10 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
pcc requested review of this revision.Aug 18 2020, 1:10 PM
pcc added a comment.Aug 18 2020, 1:15 PM

@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 added a comment.Aug 18 2020, 1:33 PM
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 updated this revision to Diff 286393.Aug 18 2020, 2:05 PM

Add explanation to comment

pcc added a comment.Aug 18 2020, 2:05 PM
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 accepted this revision.Aug 19 2020, 6:57 AM

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.

This revision is now accepted and ready to land.Aug 19 2020, 6:57 AM