This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mgorny on Jun 6 2018, 3:34 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny created this revision.Jun 6 2018, 3:34 AM
Herald added subscribers: Restricted Project, llvm-commits, kubamracek. · View Herald TranscriptJun 6 2018, 3:34 AM

Gentle ping.

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 added a comment.Jul 9 2018, 5:57 AM

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.

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 added a comment.Jul 9 2018, 8:22 AM

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 updated this revision to Diff 171646.Oct 30 2018, 2:08 AM

Rebased.

mgorny updated this revision to Diff 179453.Dec 23 2018, 2:58 PM
mgorny retitled this revision from [sanitizer_common] Fix using libtirpc on Linux to [compiler-rt] [sanitizer_common] Fix using libtirpc on Linux.
mgorny edited the summary of this revision. (Show Details)
mgorny added a reviewer: Lekensteyn.

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

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 this revision to Diff 179460.Dec 24 2018, 1:26 AM

Updated as requested by @Lekensteyn

krytarowski added a subscriber: krytarowski.EditedDec 24 2018, 1:37 AM

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 this revision to Diff 179556.Dec 27 2018, 8:38 AM
mgorny retitled this revision from [compiler-rt] [sanitizer_common] Fix using libtirpc on Linux to [compiler-rt] [sanitizer_common] Remove support for tirpc/rpc/xdr.h.
mgorny edited the summary of this revision. (Show Details)
mgorny added reviewers: krytarowski, vitalybuka.

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.

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

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 added a comment.Jan 8 2019, 9:50 AM

Ping again.

vitalybuka accepted this revision.Jan 9 2019, 1:19 PM

Looks like no activity by @ygribov after September
LGTM

This revision is now accepted and ready to land.Jan 9 2019, 1:19 PM
This revision was automatically updated to reflect the committed changes.