Re: Patch ping Re: [PATCH] -fsanitize=address,undefined support on s390x

[Jakub Jelinek <jakub at redhat dot com>]

On Tue, Jan 31, 2017 at 01:15:20PM +0100, Andreas Krebbel wrote:
> >> 2017-01-23  Jakub Jelinek  <jakub@redhat.com>
> >>
> >> gcc/
> >> 	* config/s390/s390.c (s390_asan_shadow_offset): New function.
> >> 	(TARGET_ASAN_SHADOW_OFFSET): Redefine.
> >> libsanitizer/
> >> 	* configure.tgt: Enable asan and ubsan on 64-bit s390*-*-linux*.
> 
> Ok. Thanks!
> 
> >> +/* Implement the TARGET_ASAN_SHADOW_OFFSET hook.  */
> >> +
> >> +static unsigned HOST_WIDE_INT
> >> +s390_asan_shadow_offset (void)
> >> +{
> >> +  return TARGET_64BIT ? HOST_WIDE_INT_1U << 52 : HOST_WIDE_INT_UC (0x20000000);
> >> +}
> 
> These values probably come from the LLVM implementation?!
> The 64 bit offset appears to reside beyond the default task size which is 4TB (1<<42). So it will
> trigger the task to be upgraded to an additional level of page tables. Sounds reasonable in order to
> avoid collisions for most of the cases. The task size in that case will end up as 1<<53 so the
> shadow map should end up near to the top of it.
> The 32 bit value should not be used since 32 bit is disabled - right? This appears to be the default
> value for 32 bit targets. I'm not sure if we will have adjust it since we only have 31 bit address
> space.

Both values reflect on what is right now in libsanitizer/asan/asan_mapping.h
(the library has to agree with the compiler on the shift).
That header has for s390{,x}:

// Default Linux/S390 mapping:
// || `[0x30000000, 0x7fffffff]` || HighMem    ||
// || `[0x26000000, 0x2fffffff]` || HighShadow ||
// || `[0x24000000, 0x25ffffff]` || ShadowGap  ||
// || `[0x20000000, 0x23ffffff]` || LowShadow  ||
// || `[0x00000000, 0x1fffffff]` || LowMem     ||
//
// Default Linux/SystemZ mapping:
// || `[0x14000000000000, 0x1fffffffffffff]` || HighMem    ||
// || `[0x12800000000000, 0x13ffffffffffff]` || HighShadow ||
// || `[0x12000000000000, 0x127fffffffffff]` || ShadowGap  ||
// || `[0x10000000000000, 0x11ffffffffffff]` || LowShadow  ||
// || `[0x00000000000000, 0x0fffffffffffff]` || LowMem     ||
...
static const u64 kDefaultShadowOffset32 = 1ULL << 29;  // 0x20000000
...
static const u64 kSystemZ_ShadowOffset64 = 1ULL << 52;
...
#if SANITIZER_WORDSIZE == 32
...
#  else
#    define SHADOW_OFFSET kDefaultShadowOffset32
#  endif
#else
...
#  elif defined(__s390x__)
#    define SHADOW_OFFSET kSystemZ_ShadowOffset64
...

If you think some of those aren't appropriate for s390 64-bit or 32-bit,
then it would need to be changed in upstream too.
The if:

> >> --- libsanitizer/configure.tgt.jj	2017-01-23 15:25:21.000000000 +0100
> >> +++ libsanitizer/configure.tgt	2017-01-23 15:36:40.787456320 +0100
> >> @@ -39,6 +39,11 @@ case "${target}" in
> >>  	;;
> >>    sparc*-*-linux*)
> >>  	;;
> >> +  s390*-*-linux*)
> >> +	if test x$ac_cv_sizeof_void_p = x4; then
> >> +		UNSUPPORTED=1
> >> +	fi
> >> +	;;

part is purely because in Fedora s390 31-bit is not available anymore (only
64-bit s390x), so I can't test that.  If you could test it somewhere and
verify even 31-bit works fine, then the
  if test x$ac_cv_sizeof_void_p = x4; then
	UNSUPPORTED=1
  fi
lines could be removed.

	Jakub