[sanitizer] Use consistent checks for XDR

[iana]

Summary

sanitizer_platform_limits_posix.h defines __sanitizer_XDR if SANITIZER_LINUX && !SANITIZER_ANDROID, but sanitizer_platform_limits_posix.cpp tries to check it if HAVE_RPC_XDR_H. This coincidentally works because macOS has a broken <rpc/xdr.h> which causes HAVE_RPC_XDR_H to be 0, but if <rpc/xdr.h> is fixed then clang fails to compile on macOS. Restore the platform checks so that <rpc/xdr.h> can be fixed on macOS.

[iana]

The checks were (I think erroneously) removed in D8698

[vitalybuka]

If OSX fixes HAVE_RPC_XDR_H=1 why not to fix mac handling of XDR here instead?

[kstoimenov]

Please get approval from vitalybuka.

[iana]

In D130060#3666808, @vitalybuka wrote:

If OSX fixes HAVE_RPC_XDR_H=1 why not to fix mac handling of XDR here instead?

I didn't want to change the behavior on macOS, this part has never been on on that platform.

[iana]

That would be changing the struct __sanitizer_XDR declaration in sanitizer_platform_limits_posix.h to be HAVE_RPC_XDR_H instead of SANITIZER_LINUX && !SANITIZER_ANDROID. I think right now it doesn't check HAVE_RPC_XDR_H because it's not actually using anything from xdr.h.

Mostly I wasn't totally sure what the intended behavior was, and I'm just trying to fix the build, not add new functionality to non-Linux platforms.

[vitalybuka]

In D130060#3666880, @iana wrote:

In D130060#3666808, @vitalybuka wrote:

If OSX fixes HAVE_RPC_XDR_H=1 why not to fix mac handling of XDR here instead?

I didn't want to change the behavior on macOS, this part has never been on on that platform.

#if HAVE_RPC_XDR_H && !SANITIZER_APPLE
with TODO to fix for Apple?

@yln @wrotki added few recent Apple committers for opinion

[iana]

In D130060#3667327, @vitalybuka wrote:

In D130060#3666880, @iana wrote:

In D130060#3666808, @vitalybuka wrote:

If OSX fixes HAVE_RPC_XDR_H=1 why not to fix mac handling of XDR here instead?

I didn't want to change the behavior on macOS, this part has never been on on that platform.

#if HAVE_RPC_XDR_H && !SANITIZER_APPLE
with TODO to fix for Apple?

@yln @wrotki added few recent Apple committers for opinion

Wouldn't it be best to match the sanitizer_platform_limits_posix.h guard? That's the crux of the bug anyway, the declaration and checks are guarded with totally different macros.

[vitalybuka]

In D130060#3667367, @iana wrote:

In D130060#3667327, @vitalybuka wrote:

In D130060#3666880, @iana wrote:

In D130060#3666808, @vitalybuka wrote:

If OSX fixes HAVE_RPC_XDR_H=1 why not to fix mac handling of XDR here instead?

I didn't want to change the behavior on macOS, this part has never been on on that platform.

#if HAVE_RPC_XDR_H && !SANITIZER_APPLE
with TODO to fix for Apple?

@yln @wrotki added few recent Apple committers for opinion

Wouldn't it be best to match the sanitizer_platform_limits_posix.h guard? That's the crux of the bug anyway, the declaration and checks are guarded with totally different macros.

No. This file applies to FREEBSD as well
so it should be HAVE_RPC_XDR_H && ((SANITIZER_LINUX && !SANITIZER_ANDROID) || SANITIZER_FREEBSD)
but on all these platform HAVE_RPC_XDR_H will be set anyway
so (HAVE_RPC_XDR_H && !SANITIZER_APPLE) is just shorter

TODO is not needed as Apple does not intercept XDR

[iana]

Update the platform checks.

[iana]

In D130060#3667440, @vitalybuka wrote:

In D130060#3667367, @iana wrote:

In D130060#3667327, @vitalybuka wrote:

In D130060#3666880, @iana wrote:

In D130060#3666808, @vitalybuka wrote:

If OSX fixes HAVE_RPC_XDR_H=1 why not to fix mac handling of XDR here instead?

I didn't want to change the behavior on macOS, this part has never been on on that platform.

#if HAVE_RPC_XDR_H && !SANITIZER_APPLE
with TODO to fix for Apple?

@yln @wrotki added few recent Apple committers for opinion

Wouldn't it be best to match the sanitizer_platform_limits_posix.h guard? That's the crux of the bug anyway, the declaration and checks are guarded with totally different macros.

No. This file applies to FREEBSD as well
so it should be HAVE_RPC_XDR_H && ((SANITIZER_LINUX && !SANITIZER_ANDROID) || SANITIZER_FREEBSD)
but on all these platform HAVE_RPC_XDR_H will be set anyway
so (HAVE_RPC_XDR_H && !SANITIZER_APPLE) is just shorter

TODO is not needed as Apple does not intercept XDR

Ok, updated, thanks for the help!

[vitalybuka]

so (HAVE_RPC_XDR_H && !SANITIZER_APPLE) is just shorter

So why not a shorter version? Do I miss some benefits of this long expression?

[iana]

In D130060#3667462, @vitalybuka wrote:

so (HAVE_RPC_XDR_H && !SANITIZER_APPLE) is just shorter

So why not a shorter version? Do I miss some benefits of this long expression?

Because I misread your comment the first time >.>

[iana]

Update the platform checks to be the nicer shorter version.

[iana]

In D130060#3667463, @iana wrote:

In D130060#3667462, @vitalybuka wrote:

so (HAVE_RPC_XDR_H && !SANITIZER_APPLE) is just shorter

So why not a shorter version? Do I miss some benefits of this long expression?

Because I misread your comment the first time >.>

Sorry, updated!

[vitalybuka]

() is not requirement
do you have committer access?

[iana]

In D130060#3667467, @vitalybuka wrote:

() is not requirement
do you have committer access?

Good point, let me get rid of the parens. I think I have committer access.

[iana]

Get rid of the extra parens

[vitalybuka]

Feel free to land it then. I don't see a point to wait for other feedback. At first I thought it's a different kind of issue.

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp

 
 
 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
+#if HAVE_RPC_XDR_H && !SANITIZER_APPLE
 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);