Fix checking for rpc/xdr.h

[ngie]

Summary

rpc/xdr.h (on FreeBSD) at least requires rpc/types.h. Check for both
headers (and include both headers) whenever including rpc/xdr.h to
ensure that the code will compile on supported platforms.

While this won't affect FreeBSD, this may affect non-standard Linux-based
or Solaris-based platforms, based on FreeBSD's variant of libc (ex:
android's bionic library).

Signed-off-by: Enji Cooper <[e-mail]>

[MaskRay]

I think the fix is correct, but is there any patch fixing these rpc/*.h headers on FreeBSD's side (for bool_t)? https://svnweb.freebsd.org/base/head/include/rpc/xdr.h

glibc and Solaris-based platforms shouldn't be affected by this change, but what does bionic do? Does it provide rpc/*.h?

[ngie]

In D58427#1405336, @MaskRay wrote:

I think the fix is correct, but is there any patch fixing these rpc/*.h headers on FreeBSD's side (for bool_t)? https://svnweb.freebsd.org/base/head/include/rpc/xdr.h

The API on FreeBSD clearly calls for rpc/types.h: https://www.freebsd.org/cgi/man.cgi?query=xdr&sektion=3&apropos=0&manpath=freebsd .

By convention, FreeBSD splits up the types from the API declarations to avoid header pollution. This is why sys/socket.h (for instance) needing sys/types.h is more necessary on FreeBSD than Linux.

glibc and Solaris-based platforms shouldn't be affected by this change, but what does bionic do? Does it provide rpc/*.h?

[Upstream] Bionic libc doesn't currently include these headers. However, if it did, it would be affected by this on FreeBSD. I was citing a potential trigger for this issue, which doesn't exist yet.

[MaskRay]

The API on FreeBSD clearly calls for rpc/types.h: https://www.freebsd.org/cgi/man.cgi?query=xdr&sektion=3&apropos=0&manpath=freebsd .

By convention, FreeBSD splits up the types from the API declarations to avoid header pollution. This is why sys/socket.h (for instance) needing sys/types.h is more necessary on FreeBSD than Linux.

Thanks for the explanation (to prevent header pollution), but for this particular case, I'm still not sure why https://github.com/freebsd/freebsd/blob/master/sys/rpc/xdr.h cannot include rpc/types.h. The file directly references TRUE bool_t. If it didn't, it would be justified to not include rpc/includes.h...

[ngie]

In D58427#1408294, @MaskRay wrote:

The API on FreeBSD clearly calls for rpc/types.h: https://www.freebsd.org/cgi/man.cgi?query=xdr&sektion=3&apropos=0&manpath=freebsd .

By convention, FreeBSD splits up the types from the API declarations to avoid header pollution. This is why sys/socket.h (for instance) needing sys/types.h is more necessary on FreeBSD than Linux.

Thanks for the explanation (to prevent header pollution), but for this particular case, I'm still not sure why https://github.com/freebsd/freebsd/blob/master/sys/rpc/xdr.h cannot include rpc/types.h. The file directly references TRUE bool_t. If it didn't, it would be justified to not include rpc/includes.h...

@MaskRay: again, this is being done for historical reasons (~20 years worth of history) in terms of design choices, because of the philosophy that one should not need to require sys/types.h (in reality sys/_types.h) in multiple include headers, when documenting their inclusion once is sufficient, and generally done.

It avoids having to pollute many headers, like rpc/xdr.h, with includes to rpc/types.h (the total count of rpc headers now being 28)...

$ ls -1 /usr/include/rpc/ | grep -vc types.h
28

... this reduces complexity and removes the need for having a split between multiple header directories, like Linux does, e.g., bits/, asm-*/, etc.

It's not just FreeBSD that does this; many of the other BSDs follow this philosophy.

[MaskRay]

In D58427#1447978, @ngie wrote:

In D58427#1408294, @MaskRay wrote:

The API on FreeBSD clearly calls for rpc/types.h: https://www.freebsd.org/cgi/man.cgi?query=xdr&sektion=3&apropos=0&manpath=freebsd .

By convention, FreeBSD splits up the types from the API declarations to avoid header pollution. This is why sys/socket.h (for instance) needing sys/types.h is more necessary on FreeBSD than Linux.

Thanks for the explanation (to prevent header pollution), but for this particular case, I'm still not sure why https://github.com/freebsd/freebsd/blob/master/sys/rpc/xdr.h cannot include rpc/types.h. The file directly references TRUE bool_t. If it didn't, it would be justified to not include rpc/includes.h...

@MaskRay: again, this is being done for historical reasons (~20 years worth of history) in terms of design choices, because of the philosophy that one should not need to require sys/types.h (in reality sys/_types.h) in multiple include headers, when documenting their inclusion once is sufficient, and generally done.

It avoids having to pollute many headers, like rpc/xdr.h, with includes to rpc/types.h (the total count of rpc headers now being 28)...

$ ls -1 /usr/include/rpc/ | grep -vc types.h
28

... this reduces complexity and removes the need for having a split between multiple header directories, like Linux does, e.g., bits/, asm-*/, etc.

It's not just FreeBSD that does this; many of the other BSDs follow this philosophy.

again, this is being done for historical reasons (~20 years worth of history) in terms of design choices

I don't get the explanation why on FreeBSD, rpc/xdr.h not including rpc/types.h is justified. sys/types.h, Linux /usr/include/{bits/,asm-*/} are irrelevant.

Modern systems inheriting Sun RPC no longer require rpc/xdr.h to include rpc/types.h. glibc is one example, illumos (derived from OpenSolaris) https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/rpc/xdr.h is another.

Working around this issue for older FreeBSD or FreeBSD <= 12 is fine, but I'd like to see at least an attempt to address this issue in the FreeBSD upstream.

[MaskRay]

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_solaris.cc

31 ↗

(On Diff #187515)

Are you sure Solaris needs this? This is contrary to my research.

compiler-rt/test/msan/Linux/sunrpc.cc

17 ↗

(On Diff #187515)

Does Linux need this?

compiler-rt/test/msan/Linux/sunrpc_bytes.cc

10 ↗

(On Diff #187515)

Does Linux need this?

compiler-rt/test/msan/Linux/sunrpc_string.cc

10 ↗

(On Diff #187515)

Does Linux need this?

[MaskRay]

compiler-rt/test/msan/Linux/sunrpc.cc

17 ↗

(On Diff #187515)

See https://github.com/APokorny/libtirpc/blob/master/tirpc/rpc/xdr.h#L46 and /usr/include/rpc/xdr.h (glibc; newer glibc removed sunrpc headers) on a Linux distribution.

[ngie]

Remove all Linux and Solaris-specific changes, as they OSes don't seem to need rpc/types.h, unlike FreeBSD.

[ngie]

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_solaris.cc

31 ↗

(On Diff #187515)

I couldn't find any references, so I removed it.

compiler-rt/test/msan/Linux/sunrpc.cc

17 ↗

(On Diff #187515)

No. I'll remove it since it's OS platform specific.

compiler-rt/test/msan/Linux/sunrpc_bytes.cc

10 ↗

(On Diff #187515)

No. I'll remove it since it's OS platform specific.

compiler-rt/test/msan/Linux/sunrpc_string.cc

10 ↗

(On Diff #187515)

No. I'll remove it since it's OS platform specific.

[MaskRay]

As I said before, I think this a bug in FreeBSD's implementation. Its https://github.com/freebsd/freebsd/blob/master/sys/rpc/xdr.h should include rpc/types.h. The rationale is that that file directly references TRUE bool_t and some other rpc/includes.h types. It can't be used without including rpc/includes.h first. The pollution concern is irrelevant.

compiler-rt/cmake/base-config-ix.cmake

 # The CompilerRT build system requires CMake version 2.8.8 or higher in order
 # to use its support for building convenience "libraries" as a collection of
 # .o files. This is particularly useful in producing larger, more complex
 # runtime libraries.
 
 include(CheckIncludeFile)
+include(CheckIncludeFiles)
 include(CheckCXXSourceCompiles)
 
 check_include_file(unwind.h HAVE_UNWIND_H)
 
 # Used by sanitizer_common and tests.
-check_include_file(rpc/xdr.h HAVE_RPC_XDR_H)
+check_include_files("rpc/types.h;rpc/xdr.h" HAVE_RPC_XDR_H)
 if (NOT HAVE_RPC_XDR_H)
   set(HAVE_RPC_XDR_H 0)
 endif()
 
 # Top level target used to build all compiler-rt libraries.
 add_custom_target(compiler-rt ALL)
 add_custom_target(install-compiler-rt)
 add_custom_target(install-compiler-rt-stripped)

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cc

 #include <glob.h>
 #include <obstack.h>
 #include <mqueue.h>
 #include <net/if_ppp.h>
 #include <netax25/ax25.h>
 #include <netipx/ipx.h>
 #include <netrom/netrom.h>
 #if HAVE_RPC_XDR_H
+# include <rpc/types.h>
 # include <rpc/xdr.h>
 #endif
 #include <scsi/scsi.h>
 #include <sys/mtio.h>
 #include <sys/kd.h>
 #include <sys/shm.h>
 #include <sys/statvfs.h>
 #include <sys/timex.h>