[Support] Fix build on illumos

[nikic]

Summary

D116366 added a call to madvise() in Path.inc. Unfortunately, Illumos does not declare this function if _XOPEN_SOURCE is defined (which it is by default) and we need to provide the declaration manually. This is the same workaround used in sanitizers: https://github.com/llvm/llvm-project/blob/ee423d93ead39e94c2970b3cc7ef6e6faa75d10b/compiler-rt/lib/sanitizer_common/sanitizer_solaris.cpp#L77-L85

[ro]

llvm/lib/Support/Unix/Path.inc

121

The canonical way to check for Solaris/Illumos is defined(__sun__ && defined(__svr4__).

As issues like this don't come up for the first time, it would be extremely helpful if Illumos would provide a predefine to support that, something like __illumos__ or whatever the community sees fit.

[nikic]

Check svr4 as well.

[ro]

LGTM

llvm/lib/Support/Unix/Path.inc

123

Illumos might also consider following what Solaris 11.4 did in <sys/mman.h>:

#if !defined(__XOPEN_OR_POSIX) || defined(__EXTENSIONS__)
#if !defined(__USE_LEGACY_PROTOTYPES__)
[...]
extern int madvise(void *, size_t, int);
#else
[...]
extern int madvise(caddr_t, size_t, int);
#endif

or something to that effect.

[MaskRay]

I don't fully understand the caddr_t thing. If there is an action item for illumnos, we should make sure they know, so that llvm-project can drop the hacks.

[ro]

In D119695#3320426, @MaskRay wrote:

I don't fully understand the caddr_t thing. If there is an action item for illumnos, we should make sure they know, so that llvm-project can drop the hacks.

It's discussed in D84046. The suggested way forward for them is what I outlined Solaris did in their <sys/mman.h>: it had the same issue before and Illumos inherited it from there. It would indeed be helpful if @nikic could file an Illumos bug for this, although the workaround would have to stay for some time.

[nikic]

I filed a bug here: https://www.illumos.org/issues/14504

[joerg]

Wouldn't the correct solution for Illumos be to use posix_madvise instead? Possibly with __EXTENSIONS__. Declaring prototypes like this is just begging for problems long term...

[ro]

In D119695#3322927, @joerg wrote:

Wouldn't the correct solution for Illumos be to use posix_madvise instead? Possibly with __EXTENSIONS__. Declaring prototypes like this is just begging for problems long term...

This should work indeed: all of Solaris 11.3, 11.4, and Illumos have it. One could simply use

#ifdef POSIX_MADV_DONTNEED
    ::posix_madvise(Mapping, Size, POSIX_MADV_DONTNEED);
#else
    ::madvise(Mapping, Size, MADV_DONTNEED);
#endif

Something similar could be done in compiler-rt/lib/sanitizer_common/sanitizer_solaris.cpp (internal_madvise): according to <sys/mman.h>, the MADV_* and POSIX_MADV_* constants are kept in sync.

[jclulow]

llvm/lib/Support/Unix/Path.inc

121

We are part way through making that happen with 13726 distinguish ourselves with a macro. Indeed, the definition is already exposed by GCC 10.X, the default compiler shipped with OmniOS r151038 LTS systems (a popular distribution):

$ head -1 /etc/release
  OmniOS v11 r151038ag
$ gcc --version | head -1
gcc (OmniOS 151038/10.3.0-il-1) 10.3.0
$ gcc -E - <<< 'is this __illumos__?' | tail -1
is this 1?

We do still need to circle back around and get a fallback definition added to a core header file -- but we're definitely committed to __illumos__ and you should be able to start using it.

[ro]

llvm/lib/Support/Unix/Path.inc

121

That's excellent news, thanks. It will certainly simplify things in the future, especially in codebases like LLVM that often avoid configure/cmake tests in favor of hardcoding info based on target macros.

I think there are two more areas that need to be dealt with:

• clang needs to distinguish at runtime if it's compiling for Solaris or Illumos. Right now, we have isOSSolaris() which just looks at the os element of the triple (which is still identical between the two).
• In many cmake files we have checks like ${CMAKE_SYSTEM_NAME} MATCHES "SunOS". In some cases, the two will need to be distinguished there, too.

I guess the general model to follow will be what was done for Go: the build tag solaris is true for both, while a separate one (illumos) can be used to disambiguate if need be.

I have an idea how to go about with the introduction of isOSIllumos(), but that needs to be flashed out with the LLVM maintainers once there's general agreement on the direction.

[ro]

llvm/lib/Support/Unix/Path.inc

121

I've now filed Issue #53919 so the information is in one place rather than spread over several reviews.

llvm/lib/Support/Unix/Path.inc

 
 #if defined(__NetBSD__) || defined(__DragonFly__) || defined(__GNU__) || \
     defined(__MVS__)
 #define STATVFS_F_FLAG(vfs) (vfs).f_flag
 #else
 #define STATVFS_F_FLAG(vfs) (vfs).f_flags
 #endif
 
+#if defined(__sun__) && defined(__svr4__)
+// The madvise() declaration on Illumos cannot be made visible if _XOPEN_SOURCE
+// is defined. This declaration is also compatible with Solaris 11.4.
+extern "C" int madvise(void *, size_t, int);
+#endif
+
 using namespace llvm;
 
 namespace llvm {
 namespace sys  {
 namespace fs {
 
 const file_t kInvalidFile = -1;