[compiler-rt] [sanitizer_common] Remove support for tirpc/rpc/xdr.h

[mgorny]

Summary

Remove the partial support for rpc/xdr.h from libtirpc. Since it is
an entirely external library, we ought to build it sanitized separately
and not attempt to intercept like the libc implementation. Besides,
the existing code for tirpc support was neither complete nor working.

Noted by @krytarowski.

[mgorny]

Gentle ping.

[Lekensteyn]

Hi, thank you for the patch. First a disclaimer, I am not familiar with this RPC API at all.

This would be the first user of pkg-config here. I am not sure if this would be the best fix. Usually you cannot (easily) recompile libc and override it, but for external libs such as libtirpc this should be more doable (I think).

I'm not comfortable with adding the tirpc include path to the default include path and stripping -nodefaultlibs either, would this approach work for cross-compilation?

[mgorny]

In D47817#1155637, @Lekensteyn wrote:

Hi, thank you for the patch. First a disclaimer, I am not familiar with this RPC API at all.

Me neither. I'm only familiar with this particular issue because it's what we're hitting a lot recently.

This would be the first user of pkg-config here. I am not sure if this would be the best fix. Usually you cannot (easily) recompile libc and override it, but for external libs such as libtirpc this should be more doable (I think).

I don't think libtirpc's include path is expected to be predictable by design. I think it's something distro maintainers have to choose to avoid collision with headers that (used to be) installed by glibc. In any case, I can't think of a better solution than pkg-config here (libtirpc doesn't come with CMake modules).

I'm not comfortable with adding the tirpc include path to the default include path and stripping -nodefaultlibs either, would this approach work for cross-compilation?

I can't see why not — I suppose most of cross-compilation environments already redefine pkg-config paths appropriately. Even if it wouldn't, this shouldn't cause a regression because the 'default' case (RPC headers sraight in default paths) is not affected, and libtirpc case never could have worked.

As for no-defaultlibs, that is only stripped for the purpose of check_include_file and is afterwards restored. I can't think of a reason it would fail with stripping, and it certainly fails for me if it's not stripped.

[Lekensteyn]

In D47817#1155717, @mgorny wrote:

This would be the first user of pkg-config here. I am not sure if this would be the best fix. Usually you cannot (easily) recompile libc and override it, but for external libs such as libtirpc this should be more doable (I think).

I don't think libtirpc's include path is expected to be predictable by design. I think it's something distro maintainers have to choose to avoid collision with headers that (used to be) installed by glibc. In any case, I can't think of a better solution than pkg-config here (libtirpc doesn't come with CMake modules).

On Arch (libtirpc-1.0.3-2), Debian (libtirpc-dev 0.2.5-1.2 in sid), Gentoo (libtirpc-1.0.3), the include files happen to be installed in /usr/include/libtirpc, so it seems pretty consistent. So you could consider writing a cmake/Modules/FindLibtirpc.cmake module that looks like:

# sets Libtirpc_FOUND Libtirpc_INCLUDE_DIRS

find_path(Libtirpc_INCLUDE_DIR
  NAMES rpc/xdr.h
  PATH_SUFFIXES tirpc
)
include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(Libtirpc REQUIRED_VARS Libtirpc_INCLUDE_DIR)
if(Libtirpc_FOUND)
  set(Libtirpc_INCLUDE_DIRS ${Libtirpc_INCLUDE_DIR})
endif()

then you can use find_package(Libtirpc) without depending on pkg-config.

libtirpc case never could have worked.

The reason for that is probably because of lib/sanitizer_common/sanitizer_platform.h saying:

// Assume obsolete RPC headers are available by default
#if !defined(HAVE_RPC_XDR_H) && !defined(HAVE_TIRPC_RPC_XDR_H)
# define HAVE_RPC_XDR_H (SANITIZER_LINUX && !SANITIZER_ANDROID)
# define HAVE_TIRPC_RPC_XDR_H 0
#endif

That should probably be addressed too.

[mgorny]

In D47817#1155788, @Lekensteyn wrote:

In D47817#1155717, @mgorny wrote:

This would be the first user of pkg-config here. I am not sure if this would be the best fix. Usually you cannot (easily) recompile libc and override it, but for external libs such as libtirpc this should be more doable (I think).

I don't think libtirpc's include path is expected to be predictable by design. I think it's something distro maintainers have to choose to avoid collision with headers that (used to be) installed by glibc. In any case, I can't think of a better solution than pkg-config here (libtirpc doesn't come with CMake modules).

On Arch (libtirpc-1.0.3-2), Debian (libtirpc-dev 0.2.5-1.2 in sid), Gentoo (libtirpc-1.0.3), the include files happen to be installed in /usr/include/libtirpc, so it seems pretty consistent. So you could consider writing a cmake/Modules/FindLibtirpc.cmake module that looks like:

# sets Libtirpc_FOUND Libtirpc_INCLUDE_DIRS

find_path(Libtirpc_INCLUDE_DIR
  NAMES rpc/xdr.h
  PATH_SUFFIXES tirpc
)
include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(Libtirpc REQUIRED_VARS Libtirpc_INCLUDE_DIR)
if(Libtirpc_FOUND)
  set(Libtirpc_INCLUDE_DIRS ${Libtirpc_INCLUDE_DIR})
endif()

then you can use find_package(Libtirpc) without depending on pkg-config.

I'm sorry but I'm not interested in putting a lot of effort to write a cheap hack-a-round to avoid following upstream recommendations. Not using pkg-config is just asking for forcing downstream maintainers to specially account for LLVM, and it only creates yet another excuse not to use pkg-config in the future ('because we managed to hack-it-around so far').

libtirpc case never could have worked.

The reason for that is probably because of lib/sanitizer_common/sanitizer_platform.h saying:

// Assume obsolete RPC headers are available by default
#if !defined(HAVE_RPC_XDR_H) && !defined(HAVE_TIRPC_RPC_XDR_H)
# define HAVE_RPC_XDR_H (SANITIZER_LINUX && !SANITIZER_ANDROID)
# define HAVE_TIRPC_RPC_XDR_H 0
#endif

That should probably be addressed too.

Are you suggesting that I remove that?

[mgorny]

Rebased.

[mgorny]

Ok, I give up. Here's a version using find_package(). Can we finally get this fixed?

[Lekensteyn]

There appears to be other definitions for HAVE_TIRPC_RPC_XDR_H in
lib/sanitizer_common/sanitizer_platform.h
lib/sanitizer_common/sanitizer_platform_limits_freebsd.cc

While at it, perhaps the #if HAVE_FOO could be replaced by #ifdef HAVE_FOO?

[mgorny]

Updated as requested by @Lekensteyn

[krytarowski]

If libtirpc is from an external library we shall not install interceptors in sanitizers for it. If it is a part of libc (on glibc? on solaris?) it shall be intercepted only there.

[mgorny]

Updated to remove tirpc support completely instead, as suggested by @krytarowski. Now only built-in glibc implementation is intercepted, and the external tirpc library is left for being built with -fsanitize.

[krytarowski]

Looks good to me, but I will let a Linux person to accept it.

[vitalybuka]

In D47817#1341490, @krytarowski wrote:

Looks good to me, but I will let a Linux person to accept it.

LGTM as well, but would be nice to have input from @ygribov who added this code.

[mgorny]

@ygribov, ping.

[mgorny]

Ping again.

[vitalybuka]

Looks like no activity by @ygribov after September
LGTM

cmake/Modules/FindLibtirpc.cmake

+# CMake find_package() Module for libtirpc
+#
+# Example usage:
+#
+# find_package(Libtirpc)
+#
+# If successful the following variables will be defined
+# Libtirpc_FOUND
+# Libtirpc_INCLUDE_DIRS
+
+find_path(Libtirpc_INCLUDE_DIR
+  NAMES rpc/xdr.h
+  PATH_SUFFIXES tirpc
+)
+
+include(FindPackageHandleStandardArgs)
+find_package_handle_standard_args(Libtirpc
+  REQUIRED_VARS Libtirpc_INCLUDE_DIR)
+
+if(Libtirpc_FOUND)
+  set(Libtirpc_INCLUDE_DIRS ${Libtirpc_INCLUDE_DIR})
+endif()

lib/sanitizer_common/CMakeLists.txt

set(SANITIZER_IMPL_HEADERS
   sanitizer_win_defs.h
   sanitizer_win_dll_thunk.h
   sanitizer_win_weak_interception.h)
 
 include_directories(..)
 
 set(SANITIZER_COMMON_DEFINITIONS)
 
-include(CheckIncludeFile)
-append_have_file_definition(rpc/xdr.h HAVE_RPC_XDR_H SANITIZER_COMMON_DEFINITIONS)
-append_have_file_definition(tirpc/rpc/xdr.h HAVE_TIRPC_RPC_XDR_H SANITIZER_COMMON_DEFINITIONS)
+find_package(Libtirpc)
+
+if (Libtirpc_FOUND)
+  include_directories(${Libtirpc_INCLUDE_DIRS})
+  list(APPEND SANITIZER_COMMON_DEFINITIONS HAVE_RPC_XDR_H=1)
+endif()
 
 set(SANITIZER_CFLAGS ${SANITIZER_COMMON_CFLAGS})
 append_rtti_flag(OFF SANITIZER_CFLAGS)
 
 append_list_if(SANITIZER_LIMIT_FRAME_SIZE -Wframe-larger-than=570
                SANITIZER_CFLAGS)
 append_list_if(COMPILER_RT_HAS_WGLOBAL_CONSTRUCTORS_FLAG -Wglobal-constructors
                SANITIZER_CFLAGS)

lib/sanitizer_common/sanitizer_platform_limits_posix.cc

 #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/xdr.h>
-#elif HAVE_TIRPC_RPC_XDR_H
-# include <tirpc/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>
 #if defined(__mips64)

 
 
 CHECK_TYPE_SIZE(group);
 CHECK_SIZE_AND_OFFSET(group, gr_name);
 CHECK_SIZE_AND_OFFSET(group, gr_passwd);
 CHECK_SIZE_AND_OFFSET(group, gr_gid);
 CHECK_SIZE_AND_OFFSET(group, gr_mem);
 
-#if HAVE_RPC_XDR_H || HAVE_TIRPC_RPC_XDR_H
+#if HAVE_RPC_XDR_H
 CHECK_TYPE_SIZE(XDR);
 CHECK_SIZE_AND_OFFSET(XDR, x_op);
 CHECK_SIZE_AND_OFFSET(XDR, x_ops);
 CHECK_SIZE_AND_OFFSET(XDR, x_public);
 CHECK_SIZE_AND_OFFSET(XDR, x_private);
 CHECK_SIZE_AND_OFFSET(XDR, x_base);
 CHECK_SIZE_AND_OFFSET(XDR, x_handy);
 COMPILER_CHECK(__sanitizer_XDR_ENCODE == XDR_ENCODE);