[PATCH] D47819: [compiler-rt] [test] Disable sunrpc tests when rpc/xdr.h is missing

Mon Jan 14 10:07:00 PST 2019

mgorny marked 2 inline comments as done.
mgorny added a comment.

In D47819#1356347 <https://reviews.llvm.org/D47819#1356347>, @Lekensteyn wrote:

> The direction of this patch looks reasonable to me. Is it worth mentioning the issue (https://github.com/google/sanitizers/issues/974) in the commit message?

Yes, makes sense. I'll add it when committing.

> What environment have you tested this in, could you verify that the tests are indeed skipped on a system without these headers, and passed otherwise?

I have only tests without those headers. I'll try to rebuild glibc with them enabled if it's still supported but it'd be preferable if somebody else tested it on 'natural' environment.

================
Comment at: lib/sanitizer_common/CMakeLists.txt:195
+set(SANITIZER_COMMON_DEFINITIONS
+  HAVE_RPC_XDR_H=${HAVE_RPC_XDR_H})

----------------
Lekensteyn wrote:
> The old code defined HAVE_xxx to 0 or 1, but in general wouldn't it be preferable to define it to 1 if available, and not define it at all if unavailable? This would match most other HAVE_xxx checks.
> 
> In order to make the lit config work in this case, you have to use something like `pythonize_bool(HAVE_RPC_XDR_H)`.
Well, I suppose I could do that but given that the old code did it like this, the 0/1 definition seems to be a common practice in compiler-rt, and there is no apparent benefit from doing it the other way, I don't think it a good thing to do.

================
Comment at: test/lit.common.configured.in:39
 set_default("android", @ANDROID_PYBOOL@)
+set_default("have_rpc_xdr_h", @HAVE_RPC_XDR_H@)
 config.available_features.add('target-is-%s' % config.target_arch)
----------------
Lekensteyn wrote:
> This becomes 0 or 1 instead of True or False, not sure if that will cause issues later.
It shouldn't. 0/1 evaluates the same in boolean context, and other LLVM tests are relying on 0/1 being passed to lit already.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D47819/new/

https://reviews.llvm.org/D47819