[AIX][z/OS][Support] Provide no-op mapped_file_region::dontNeedImpl implementation

[daltenty]

Summary

mapped_file_region::dontNeedImpl added in D116366 calls madvise, which causes problems for z/OS and AIX.

For z/OS, we don't have either madvise, so treat this as a no-op, same as Windows does.

For AIX, it doesn't have any effect, doesn't have a standardized signature, and it needs certain feature test macros (i.e. _ALL_SOURCE) we don't set by default on for LLVM on AIX, so just make it a no-op too.

[aganea]

llvm/lib/Support/Unix/Path.inc

875

Shouldn't we protect the contents of this function with #ifdef HAVE_SYS_MMAN_H?

[daltenty]

Add HAVE_SYS_MMAN_H guard

[zibi]

llvm/lib/Support/Unix/Path.inc

875

I don't think we can use HAVE_SYS_MMAN_H since this is defined on z/OS since we do have sys/mman.h header file. We just don't have madvise().

[daltenty]

This comment was removed by daltenty.

[zibi]

LGTM, but I would prefer if we could use ::posix_madvise() not just on AIX. This will remove the unnecessary directive path.
However, we need to buy-in from the originator which introduced this call.

[daltenty]

llvm/lib/Support/Unix/Path.inc

875

The version here now should provide the fallback for both the case where we the header isn't available or if we are on z/OS, so I think we're ok.

[daltenty]

LGTM, but I would prefer if we could use ::posix_madvise() not just on AIX. This will remove the unnecessary directive path.

However, we need to buy-in from the originator which introduced this call.

Unfortunately it sounds like there is a different semantic for glibc on Linux though (namely the kernel is allowed to immediately discard dirty pages, which is why we assert the readonly property I assume), so the POSIX version is a no-op there, so we can't really unify the paths or we'll break the original behaviour. Worse, looking at the trace AIX I think we also don't make the system call, so I think we may need to abandon posix_madvise.

[daltenty]

Use madvise on AIX as well, since the semantics of posix_madvise don't match,

[daltenty]

Update comment

[daltenty]

Fix indent

[MaskRay]

POSIX specifies sys/mman.h and it is available on all major *NIX operation systems, so I don't think we need HAVE_SYS_MMAN_H.

We cannot use posix_madvise on Linux glibc/musl because MADV_DONTNEED (desired semantics on a read-only file mapping) is a no-op in their posix_madvise implementations.

If madvise doesn't have a benefit on AIX, consider just not implementing it to avoid some CMake complexity.

llvm/lib/Support/CMakeLists.txt

3 ↗

(On Diff #397365)

llvm/lib/Support/Unix/Path.inc

875–883

if (Mapping) { or use early return

[daltenty]

If madvise doesn't have a benefit on AIX, consider just not implementing it to avoid some CMake complexity.

I think you are right, I did some more digging and it seems like the OS is unlikely to use the information we are giving it any way:

The madvise subroutine is provided for compatibility only. The system takes no action on the advice specified.

https://www.ibm.com/docs/en/aix/7.1?topic=memory-understanding-mapping
https://www.ibm.com/docs/en/aix/7.1?topic=m-madvise-subroutine

I'll simplify this patch as suggested.

[daltenty]

Simply the patch since we've confirmed madvise doesn't have a useful effect on AIX anyway

[abhina.sreeskantharajan]

LGTM with minor formatting comment

llvm/lib/Support/Unix/Path.inc

874

Can this be #endif instead of #else? Not sure if this counts as not using an else after return.

[zibi]

LGTM

[daltenty]

llvm/lib/Support/Unix/Path.inc

874

IIUC what you're suggesting, we'd end up including the problem callsite bellow then, so I think we need to keep this.

llvm/lib/Support/Unix/Path.inc

 void mapped_file_region::dontNeedImpl() {
   assert(Mode == mapped_file_region::readonly);
   if (Mapping)
+#if defined(__MVS__)
+    // z/OS doesn't have either madvise, so treat this as a nop.
+    return;
+#elif defined(_AIX)
+    // Prefer the POSIX implementation on AIX, since the signature for madvise is non-standardised
+    // and only exists with certain feature test macros.
+    ::posix_madvise(Mapping, Size, POSIX_MADV_DONTNEED);
+#else
     ::madvise(Mapping, Size, MADV_DONTNEED);
+#endif
 }
 
 int mapped_file_region::alignment() {