sparc bootstrap still broken

sparc bootstrap still broken

[David Miller]

I don't think it's reasonable that the sparc bootstrap is still broken
in the tree, even though a fix has existed for nearly a week.

It is not acceptable to say "everyone has to apply a special patch
until some external dependency that will take an unknown, variable,
length of time to resolve is out of the way"

This is exactly why changes should be installed directly into the
GCC sources as soon as they are ready, and the current way things
are being handled with ASAN is beyond unacceptable.

I'm very tempted to just commit the build fix in myself at this point.

Re: sparc bootstrap still broken

[Konstantin Serebryany]

On Tue, Nov 20, 2012 at 9:16 AM, David Miller <davem@davemloft.net> wrote:
>
> I don't think it's reasonable that the sparc bootstrap is still broken
> in the tree, even though a fix has existed for nearly a week.
>
> It is not acceptable to say "everyone has to apply a special patch
> until some external dependency that will take an unknown, variable,
> length of time to resolve is out of the way"
>
> This is exactly why changes should be installed directly into the
> GCC sources as soon as they are ready, and the current way things
> are being handled with ASAN is beyond unacceptable.
>
> I'm very tempted to just commit the build fix in myself at this point.

Please do (the same that was applied upstream).
Please also note:
  - I am on vacation with random access to PC, that's why I did not
want to rush with my first commits to gcc trunk.
  - asan for SPARC is not known to work at all (is it?) and thus a
simpler fix might be to disable it for now.

I hope we can agree on the procedure soon.

--kcc

Re: sparc bootstrap still broken

[David Miller]

From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
Date: Tue, 20 Nov 2012 09:20:29 +0400

> Please do (the same that was applied upstream).

Which one was that?

> Please also note:
>   - I am on vacation with random access to PC, that's why I did not
> want to rush with my first commits to gcc trunk.

This is actually another argument for not having external dependencies
on fixing ASAN problems directly in the GCC sources.

>   - asan for SPARC is not known to work at all (is it?) and thus a
> simpler fix might be to disable it for now.

I asked for the code to be enabled, because this code should be
portable to all Linux systems and I also plan to work on Sparc support
as the backend bits doesn't look terribly difficult (and this is
largely proven by how fast the powerpc port was written).

But this fix also cures problems on supported targets.

Re: sparc bootstrap still broken

[Konstantin Serebryany]

On Tue, Nov 20, 2012 at 9:26 AM, David Miller <davem@davemloft.net> wrote:
> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
> Date: Tue, 20 Nov 2012 09:20:29 +0400
>
>> Please do (the same that was applied upstream).
>
> Which one was that?
http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.cc?r1=168301&r2=168300&pathrev=168301

>
>> Please also note:
>>   - I am on vacation with random access to PC, that's why I did not
>> want to rush with my first commits to gcc trunk.
>
> This is actually another argument for not having external dependencies
> on fixing ASAN problems directly in the GCC sources.

Agree (bad timing, another maintainer from our side is on a conference).


>
>>   - asan for SPARC is not known to work at all (is it?) and thus a
>> simpler fix might be to disable it for now.
>
> I asked for the code to be enabled, because this code should be
> portable to all Linux systems and I also plan to work on Sparc support
> as the backend bits doesn't look terribly difficult (and this is
> largely proven by how fast the powerpc port was written).

Cool! (the port should be easy indeed given that we have ARM, PowerPC and x86)

>
> But this fix also cures problems on supported targets.

Re: sparc bootstrap still broken

[David Miller]

From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
Date: Tue, 20 Nov 2012 09:34:14 +0400

> On Tue, Nov 20, 2012 at 9:26 AM, David Miller <davem@davemloft.net> wrote:
>> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
>> Date: Tue, 20 Nov 2012 09:20:29 +0400
>>
>>> Please do (the same that was applied upstream).
>>
>> Which one was that?
> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.cc?r1=168301&r2=168300&pathrev=168301

That change is broken and will not work.

There is no such CPP define as __sparc64__

You have to check "__sparc__ && __arch64__"

HJ's patch was more concise and actually works for all known
targets supported by both GCC and LLVM combined.

It is also likely to just work out of the box for new targets as well.

Whereas your version of the fix is higher maintainence in the long run
since it creates yet another spot where each new 64-bit target has to
place a target specific check.

Re: sparc bootstrap still broken

[Konstantin Serebryany]

On Tue, Nov 20, 2012 at 10:20 AM, David Miller <davem@davemloft.net> wrote:
> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
> Date: Tue, 20 Nov 2012 09:34:14 +0400
>
>> On Tue, Nov 20, 2012 at 9:26 AM, David Miller <davem@davemloft.net> wrote:
>>> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
>>> Date: Tue, 20 Nov 2012 09:20:29 +0400
>>>
>>>> Please do (the same that was applied upstream).
>>>
>>> Which one was that?
>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.cc?r1=168301&r2=168300&pathrev=168301
>
> That change is broken and will not work.
>
> There is no such CPP define as __sparc64__
>
> You have to check "__sparc__ && __arch64__"
>
> HJ's patch was more concise and actually works for all known
> targets supported by both GCC and LLVM combined.
>
> It is also likely to just work out of the box for new targets as well.
>
> Whereas your version of the fix is higher maintainence in the long run
> since it creates yet another spot where each new 64-bit target has to
> place a target specific check.


Ok. Will this work?


// Are we using 32-bit or 64-bit syscalls?
// x32 (which defines __x86_64__) has __WORDSIZE == 32
// but it still needs to use 64-bit syscalls.
#if defined(__x86_64__) || __WORDSIZE == 64
# define SANITIZER_LINUX_USES_64BIT_SYSCALLS 1
#else
# define SANITIZER_LINUX_USES_64BIT_SYSCALLS 0
#endif


BTW, I've created a SPARC section at
http://code.google.com/p/address-sanitizer/wiki/SupportedPlatforms,
you are welcome to contribute to the documentation there. (I'll need
your google-account-email to grant you write access)

--kcc

Re: sparc bootstrap still broken

[David Miller]

From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
Date: Tue, 20 Nov 2012 11:41:03 +0400

> Ok. Will this work?
> 
> // Are we using 32-bit or 64-bit syscalls?
> // x32 (which defines __x86_64__) has __WORDSIZE == 32
> // but it still needs to use 64-bit syscalls.
> #if defined(__x86_64__) || __WORDSIZE == 64
> # define SANITIZER_LINUX_USES_64BIT_SYSCALLS 1
> #else
> # define SANITIZER_LINUX_USES_64BIT_SYSCALLS 0
> #endif

Of course, as it matches the test H.J. Liu's patch used.

Please, at a bare minimum, give him some credit for this fix.

Re: sparc bootstrap still broken

[Konstantin Serebryany]

Done: http://llvm.org/viewvc/llvm-project?rev=168358&view=rev

On Tue, Nov 20, 2012 at 12:19 PM, David Miller <davem@davemloft.net> wrote:
> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
> Date: Tue, 20 Nov 2012 11:41:03 +0400
>
>> Ok. Will this work?
>>
>> // Are we using 32-bit or 64-bit syscalls?
>> // x32 (which defines __x86_64__) has __WORDSIZE == 32
>> // but it still needs to use 64-bit syscalls.
>> #if defined(__x86_64__) || __WORDSIZE == 64
>> # define SANITIZER_LINUX_USES_64BIT_SYSCALLS 1
>> #else
>> # define SANITIZER_LINUX_USES_64BIT_SYSCALLS 0
>> #endif
>
> Of course, as it matches the test H.J. Liu's patch used.
>
> Please, at a bare minimum, give him some credit for this fix.

Re: sparc bootstrap still broken

[Peter Bergner]

On Tue, 2012-11-20 at 13:00 +0400, Konstantin Serebryany wrote:
> On Tue, Nov 20, 2012 at 12:19 PM, David Miller <davem@davemloft.net> wrote:
> > From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
> > Date: Tue, 20 Nov 2012 11:41:03 +0400
> >
> >> Ok. Will this work?
> >>
> >> // Are we using 32-bit or 64-bit syscalls?
> >> // x32 (which defines __x86_64__) has __WORDSIZE == 32
> >> // but it still needs to use 64-bit syscalls.
> >> #if defined(__x86_64__) || __WORDSIZE == 64
> >> # define SANITIZER_LINUX_USES_64BIT_SYSCALLS 1
> >> #else
> >> # define SANITIZER_LINUX_USES_64BIT_SYSCALLS 0
> >> #endif
> >
> > Of course, as it matches the test H.J. Liu's patch used.
> >
> > Please, at a bare minimum, give him some credit for this fix.
> 
> Done: http://llvm.org/viewvc/llvm-project?rev=168358&view=rev

I assume we are just waiting for someone to commit this to the GCC src,
correct?  David (Miller), were you going to do that?  I'd like that
change committed before I commit our ppc asan changes.

Peter

Re: sparc bootstrap still broken

[David Miller]

From: Peter Bergner <bergner@vnet.ibm.com>
Date: Tue, 20 Nov 2012 12:08:00 -0600

> On Tue, 2012-11-20 at 13:00 +0400, Konstantin Serebryany wrote:
>> On Tue, Nov 20, 2012 at 12:19 PM, David Miller <davem@davemloft.net> wrote:
>> > From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
>> > Date: Tue, 20 Nov 2012 11:41:03 +0400
>> >
>> >> Ok. Will this work?
>> >>
>> >> // Are we using 32-bit or 64-bit syscalls?
>> >> // x32 (which defines __x86_64__) has __WORDSIZE == 32
>> >> // but it still needs to use 64-bit syscalls.
>> >> #if defined(__x86_64__) || __WORDSIZE == 64
>> >> # define SANITIZER_LINUX_USES_64BIT_SYSCALLS 1
>> >> #else
>> >> # define SANITIZER_LINUX_USES_64BIT_SYSCALLS 0
>> >> #endif
>> >
>> > Of course, as it matches the test H.J. Liu's patch used.
>> >
>> > Please, at a bare minimum, give him some credit for this fix.
>> 
>> Done: http://llvm.org/viewvc/llvm-project?rev=168358&view=rev
> 
> I assume we are just waiting for someone to commit this to the GCC src,
> correct?  David (Miller), were you going to do that?  I'd like that
> change committed before I commit our ppc asan changes.

The problem is there are other dependencies in those two commits, the
change doesn't apply cleanly.

This situation is a very serious mess, and a merge burdon like this
really shouldn't fall upon the gcc community at large.

Re: sparc bootstrap still broken

[Konstantin Serebryany]

>> I assume we are just waiting for someone to commit this to the GCC src,

One possible value of 'someone' is kcc (me), but I *may* not be able
to do it until ~ Monday.
Other possible values of 'someone' are wmi and dvyukov

>> correct?  David (Miller), were you going to do that?  I'd like that
>> change committed before I commit our ppc asan changes.
>
> The problem is there are other dependencies in those two commits, the
> change doesn't apply cleanly.
>
> This situation is a very serious mess,

Agree. I am sorry.

> and a merge burdon like this
> really shouldn't fall upon the gcc community at large.

I really need your help to resolve this mess.
Your comments on how to automate pullbacks from upstream are welcome
here: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55376

--kcc

Re: sparc bootstrap still broken

[David Miller]

From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
Date: Tue, 20 Nov 2012 23:02:36 +0400

> I really need your help to resolve this mess.

I thought it was abundantly clear that the burdon falls upon the ASAN
folks, since they are the ones who care about the external dependency.

Nobody else inside of the GCC community cares about that.

Other examples of identical situations were given, including libffi
and GO.  Where it is %100 up to the maintainer of those modules to
deal with the merge hassles created by the external dependency, and to
shield the GCC development process completely from any part of that.

Finally, a bugzilla entry with a very limited audience is absolutely
not the appropriate place to discuss this issue.  The GCC mailing
lists are.

Re: sparc bootstrap still broken

[Konstantin Serebryany]

On Tue, Nov 20, 2012 at 11:09 PM, David Miller <davem@davemloft.net> wrote:
> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
> Date: Tue, 20 Nov 2012 23:02:36 +0400
>
>> I really need your help to resolve this mess.
>
> I thought it was abundantly clear that the burdon falls upon the ASAN
> folks, since they are the ones who care about the external dependency.
>
> Nobody else inside of the GCC community cares about that.

If nobody else in the GCC community cares about ASAN, we should
disable the SPARC build and
let us handle the issue without rush.
We are interested in SPARC, but much less than in our own sanity.

--kcc

>
> Other examples of identical situations were given, including libffi
> and GO.  Where it is %100 up to the maintainer of those modules to
> deal with the merge hassles created by the external dependency, and to
> shield the GCC development process completely from any part of that.
>
> Finally, a bugzilla entry with a very limited audience is absolutely
> not the appropriate place to discuss this issue.  The GCC mailing
> lists are.

Re: sparc bootstrap still broken

[David Miller]

From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
Date: Tue, 20 Nov 2012 23:19:51 +0400

> On Tue, Nov 20, 2012 at 11:09 PM, David Miller <davem@davemloft.net> wrote:
>> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
>> Date: Tue, 20 Nov 2012 23:02:36 +0400
>>
>>> I really need your help to resolve this mess.
>>
>> I thought it was abundantly clear that the burdon falls upon the ASAN
>> folks, since they are the ones who care about the external dependency.
>>
>> Nobody else inside of the GCC community cares about that.
> 
> If nobody else in the GCC community cares about ASAN, we should
> disable the SPARC build and
> let us handle the issue without rush.
> We are interested in SPARC, but much less than in our own sanity.

I am saying that nobody has a direct interest in the LLVM
dependencies, therefore they are entirely your responsibility.  Please
don't twist my words.

Everyone in the GCC community cares that core features are supported
on as many targets as possible.

Re: sparc bootstrap still broken

[Konstantin Serebryany]

On Tue, Nov 20, 2012 at 11:37 PM, David Miller <davem@davemloft.net> wrote:
> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
> Date: Tue, 20 Nov 2012 23:19:51 +0400
>
>> On Tue, Nov 20, 2012 at 11:09 PM, David Miller <davem@davemloft.net> wrote:
>>> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
>>> Date: Tue, 20 Nov 2012 23:02:36 +0400
>>>
>>>> I really need your help to resolve this mess.
>>>
>>> I thought it was abundantly clear that the burdon falls upon the ASAN
>>> folks, since they are the ones who care about the external dependency.
>>>
>>> Nobody else inside of the GCC community cares about that.
>>
>> If nobody else in the GCC community cares about ASAN, we should
>> disable the SPARC build and
>> let us handle the issue without rush.
>> We are interested in SPARC, but much less than in our own sanity.
>
> I am saying that nobody has a direct interest in the LLVM
> dependencies,

We do. As long as we maintain libasan in GCC, so do you (by transitive
relation).

Let's stop this. Please apply whatever minimal patch required to
unbreak the SPARC build.
We will not be accepting any non-trivial patches until we set up
semi-automated way to pull the upstream sources.

--kcc

> therefore they are entirely your responsibility.  Please
> don't twist my words.
>
> Everyone in the GCC community cares that core features are supported
> on as many targets as possible.

Re: sparc bootstrap still broken

[David Miller]

From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
Date: Tue, 20 Nov 2012 23:52:48 +0400

> Please apply whatever minimal patch required to unbreak the SPARC
> build.  We will not be accepting any non-trivial patches until we
> set up semi-automated way to pull the upstream sources.

Will do.

Sparc ASAN (was Re: sparc bootstrap still broken)

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Tue, 20 Nov 2012 14:59:10 -0500 (EST)

> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
> Date: Tue, 20 Nov 2012 23:52:48 +0400
> 
>> Please apply whatever minimal patch required to unbreak the SPARC
>> build.  We will not be accepting any non-trivial patches until we
>> set up semi-automated way to pull the upstream sources.
> 
> Will do.

I tossed together a quick sparc implementation and there seems to
be only two issues to fix:

1) As noticed by the powerpc people, you have to probe the page
   size of the system properly.  It's variable even within a
   target/OS.

   Probably a new hook, implemented in asan_linux.cc via:

	#include <unistd.h>

	uptr GetPageSize()
	{
	  return getpagesize();
	}

   I would just get rid of kPageSizeBits, rather than compute it
   dynamically as well, as it's really not used as far as I can tell.

   The mmap of the shadow area won't work on sparc without this being
   resolved.

2) The current code emitted to set the shadow values results in
   unaligned stores.  For example, for the memcmp-1.c test on 32-bit
   we get:

   0x10488 <main+8>:    add  %fp, -160, %i5
 ...
   0x10490 <main+16>:   sethi  %hi(0xf1f1f000), %g2
 ...
   0x104a0 <main+32>:   srl  %i5, 3, %i5
 ...
   0x104a8 <main+40>:   or  %g2, 0x1f1, %g2
   0x104ac <main+44>:   sethi  %hi(0x20000000), %g1
 ...
=> 0x104b4 <main+52>:   st  %g2, [ %i5 + %g1 ]

   Here %fp is 0xffffd538, and this %i5 + %g1 is 0x3ffffa93, which
   is not aligned properly for a 32-bit store.

   Therefore, this won't work for any STRICT_ALIGNMENT target.

Those seem to be the only problems that need to be resolved for this
feature to be fully working.

Re: Sparc ASAN

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Tue, 20 Nov 2012 21:20:40 -0500 (EST)

> Those seem to be the only problems that need to be resolved for this
> feature to be fully working.

FWIW, here are the changes I am using which, besides the sparc backend
bits, has some temporary workarounds for the issues I brought up.

The address violation detection seems to work properly and the only
thing that seems to be left are some backtrace/unwind issues.  These
are perhaps similar to the unwind bits that the powerpc folks ran
into.

diff --git a/gcc/asan.c b/gcc/asan.c
index bd90e0a..d9b3f1b 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -321,7 +321,10 @@ asan_emit_stack_protection (rtx base, HOST_WIDE_INT *offsets, tree *decls,
 			      NULL_RTX, 1, OPTAB_DIRECT);
   gcc_assert (asan_shadow_set != -1
 	      && (ASAN_RED_ZONE_SIZE >> ASAN_SHADOW_SHIFT) == 4);
-  shadow_mem = gen_rtx_MEM (SImode, shadow_base);
+  if (STRICT_ALIGNMENT)
+    shadow_mem = gen_rtx_MEM (QImode, shadow_base);
+  else
+    shadow_mem = gen_rtx_MEM (SImode, shadow_base);
   set_mem_alias_set (shadow_mem, asan_shadow_set);
   prev_offset = base_offset;
   for (l = length; l; l -= 2)
@@ -349,7 +352,20 @@ asan_emit_stack_protection (rtx base, HOST_WIDE_INT *offsets, tree *decls,
 	      }
 	    else
 	      shadow_bytes[i] = ASAN_STACK_MAGIC_PARTIAL;
-	  emit_move_insn (shadow_mem, asan_shadow_cst (shadow_bytes));
+	  if (STRICT_ALIGNMENT)
+	    {
+	      for (i = 0; i < 4; i++)
+		{
+		  rtx mem = adjust_address (shadow_mem, VOIDmode, i);
+		  rtx val;
+
+		  val = GEN_INT (trunc_int_for_mode (shadow_bytes[i],
+						     QImode));
+		  emit_move_insn (mem, val);
+		}
+	    }
+	  else
+	    emit_move_insn (shadow_mem, asan_shadow_cst (shadow_bytes));
 	  offset = aoff;
 	}
       while (offset <= offsets[l - 2] - ASAN_RED_ZONE_SIZE)
@@ -359,7 +375,22 @@ asan_emit_stack_protection (rtx base, HOST_WIDE_INT *offsets, tree *decls,
 				       >> ASAN_SHADOW_SHIFT);
 	  prev_offset = offset;
 	  memset (shadow_bytes, cur_shadow_byte, 4);
-	  emit_move_insn (shadow_mem, asan_shadow_cst (shadow_bytes));
+	  if (STRICT_ALIGNMENT)
+	    {
+	      int i;
+
+	      for (i = 0; i < 4; i++)
+		{
+		  rtx mem = adjust_address (shadow_mem, VOIDmode, i);
+		  rtx val;
+
+		  val = GEN_INT (trunc_int_for_mode (shadow_bytes[i],
+						     QImode));
+		  emit_move_insn (mem, val);
+		}
+	    }
+	  else
+	    emit_move_insn (shadow_mem, asan_shadow_cst (shadow_bytes));
 	  offset += ASAN_RED_ZONE_SIZE;
 	}
       cur_shadow_byte = ASAN_STACK_MAGIC_MIDDLE;
diff --git a/gcc/config/sparc/sparc.c b/gcc/config/sparc/sparc.c
index 4e9de98..7bcc815 100644
--- a/gcc/config/sparc/sparc.c
+++ b/gcc/config/sparc/sparc.c
@@ -600,6 +600,7 @@ static void sparc_print_operand_address (FILE *, rtx);
 static reg_class_t sparc_secondary_reload (bool, rtx, reg_class_t,
 					   enum machine_mode,
 					   secondary_reload_info *);
+static unsigned HOST_WIDE_INT sparc_asan_shadow_offset (void);
 \f
 #ifdef SUBTARGET_ATTRIBUTE_TABLE
 /* Table of valid machine attributes.  */
@@ -754,6 +755,9 @@ char sparc_hard_reg_printed[8];
 #undef TARGET_OPTION_OVERRIDE
 #define TARGET_OPTION_OVERRIDE sparc_option_override
 
+#undef TARGET_ASAN_SHADOW_OFFSET
+#define TARGET_ASAN_SHADOW_OFFSET sparc_asan_shadow_offset
+
 #if TARGET_GNU_TLS && defined(HAVE_AS_SPARC_UA_PCREL)
 #undef TARGET_ASM_OUTPUT_DWARF_DTPREL
 #define TARGET_ASM_OUTPUT_DWARF_DTPREL sparc_output_dwarf_dtprel
@@ -11034,6 +11038,14 @@ get_some_local_dynamic_name_1 (rtx *px, void *data ATTRIBUTE_UNUSED)
   return 0;
 }
 
+/* Implement the TARGET_ASAN_SHADOW_OFFSET hook.  */
+
+static unsigned HOST_WIDE_INT
+sparc_asan_shadow_offset (void)
+{
+  return (unsigned HOST_WIDE_INT) 1 << (TARGET_ARCH64 ? 44 : 29);
+}
+
 /* This is called from dwarf2out.c via TARGET_ASM_OUTPUT_DWARF_DTPREL.
    We need to emit DTP-relative relocations.  */
 
diff --git a/libsanitizer/sanitizer_common/sanitizer_common.h b/libsanitizer/sanitizer_common/sanitizer_common.h
index cddefd7..00628fc 100644
--- a/libsanitizer/sanitizer_common/sanitizer_common.h
+++ b/libsanitizer/sanitizer_common/sanitizer_common.h
@@ -21,7 +21,7 @@ namespace __sanitizer {
 // Constants.
 const uptr kWordSize = __WORDSIZE / 8;
 const uptr kWordSizeInBits = 8 * kWordSize;
-const uptr kPageSizeBits = 12;
+const uptr kPageSizeBits = 13;
 const uptr kPageSize = 1UL << kPageSizeBits;
 const uptr kCacheLineSize = 64;
 #ifndef _WIN32

Re: Sparc ASAN

[Jakub Jelinek]

On Tue, Nov 20, 2012 at 11:19:33PM -0500, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Tue, 20 Nov 2012 21:20:40 -0500 (EST)
> 
> > Those seem to be the only problems that need to be resolved for this
> > feature to be fully working.
> 
> FWIW, here are the changes I am using which, besides the sparc backend
> bits, has some temporary workarounds for the issues I brought up.

To explain a little bit these unaligned stores.
From what I understand LLVM will always put all the ASAN protected
automatic vars into a single block and align the whole block to 32 bytes
(on i?86/x86_64 in GCC that is the backend dynamic stack realignment,
those andq $-32, %rsp etc. in the prologue, on other targets rth added
a release or two ago handling of these via alloca that where the alloca
allocates up to the alignment - 1 bytes more and we add alignment - 1
to the result of alloca and mask it).
But it seemed nothing on the libasan relies on it actually being 32-byte
aligned, so what I've implemented was keep the vars as aligned as they
were before (well, the base), and just put the padding in between as if
the base was 32-byte aligned and I wanted to align them all to 32-bytes
(ignoring here in the description more aligned vars etc.).
That of course means the shadow memory stores may be unaligned, but as
i?86/x86_64 were the only supported targets initially, and the unaligned
stores are IMHO pretty fast there, it isn't a big deal.

Now, for strict alignment targets, the question is what is the fastest
way to implement this.  There is the cost of dynamically realigning the
stack var block (which isn't just about the alloca at the beginning, but
also pretty much having one extra register reserved for the result of
the alloca, used as the base to access all the local vars, sure, you can
spill it, but if it is used frequently, it will likely be assigned to a hard
reg most of its lifetime), and on the other side there is the cost of
accessing the shadow memory at smaller granularity.

If some target has the virtual-stack-vars reg not even 8 byte aligned,
some realignment is required, as the shadow shift is 3.
If it is just 8 byte aligned and we don't realign the block, then yes,
shadow memory needs to be accessed by bytes, if it is 16 byte aligned,
perhaps we could use 16-bit stores instead of just 8-bit ones.
And perhaps the decision (for strict alignment arches) whether to realign or
not the whole block could be best dynamic, for small number of asan
protected vars (i.e. small number of expected shadow memory stores)
we could just not realign and use 8-bit or 16-bit shadow memory stores,
for larger amounts of vars perhaps it might be cheaper to realign.

So, concerning your patch, which implements basically the never realign,
just sometimes use smaller size of shadow memory accesses, the decision
shouldn't be done solely based on STRICT_ALIGNMENT, but needs to take
into account the alignment of virtual_stack_vars_rtx in the current
function, if it's byte alignment is >= (4 << ASAN_SHADOW_SHIFT), then
you can use the same code as for !STRICT_ALIGNMENT, if it's byte alignment
is >= (2 << ASAN_SHADOW_SHIFT), then you can use HImode stores, otherwise
QImode.

> --- a/gcc/asan.c
> +++ b/gcc/asan.c
> @@ -321,7 +321,10 @@ asan_emit_stack_protection (rtx base, HOST_WIDE_INT *offsets, tree *decls,
>  			      NULL_RTX, 1, OPTAB_DIRECT);
>    gcc_assert (asan_shadow_set != -1
>  	      && (ASAN_RED_ZONE_SIZE >> ASAN_SHADOW_SHIFT) == 4);
> -  shadow_mem = gen_rtx_MEM (SImode, shadow_base);
> +  if (STRICT_ALIGNMENT)
> +    shadow_mem = gen_rtx_MEM (QImode, shadow_base);
> +  else
> +    shadow_mem = gen_rtx_MEM (SImode, shadow_base);
>    set_mem_alias_set (shadow_mem, asan_shadow_set);
>    prev_offset = base_offset;
>    for (l = length; l; l -= 2)
> @@ -349,7 +352,20 @@ asan_emit_stack_protection (rtx base, HOST_WIDE_INT *offsets, tree *decls,
>  	      }
>  	    else
>  	      shadow_bytes[i] = ASAN_STACK_MAGIC_PARTIAL;
> -	  emit_move_insn (shadow_mem, asan_shadow_cst (shadow_bytes));
> +	  if (STRICT_ALIGNMENT)
> +	    {
> +	      for (i = 0; i < 4; i++)
> +		{
> +		  rtx mem = adjust_address (shadow_mem, VOIDmode, i);
> +		  rtx val;
> +
> +		  val = GEN_INT (trunc_int_for_mode (shadow_bytes[i],
> +						     QImode));
> +		  emit_move_insn (mem, val);
> +		}
> +	    }
> +	  else
> +	    emit_move_insn (shadow_mem, asan_shadow_cst (shadow_bytes));
>  	  offset = aoff;
>  	}
>        while (offset <= offsets[l - 2] - ASAN_RED_ZONE_SIZE)
> @@ -359,7 +375,22 @@ asan_emit_stack_protection (rtx base, HOST_WIDE_INT *offsets, tree *decls,
>  				       >> ASAN_SHADOW_SHIFT);
>  	  prev_offset = offset;
>  	  memset (shadow_bytes, cur_shadow_byte, 4);
> -	  emit_move_insn (shadow_mem, asan_shadow_cst (shadow_bytes));
> +	  if (STRICT_ALIGNMENT)
> +	    {
> +	      int i;
> +
> +	      for (i = 0; i < 4; i++)
> +		{
> +		  rtx mem = adjust_address (shadow_mem, VOIDmode, i);
> +		  rtx val;
> +
> +		  val = GEN_INT (trunc_int_for_mode (shadow_bytes[i],
> +						     QImode));
> +		  emit_move_insn (mem, val);
> +		}
> +	    }
> +	  else
> +	    emit_move_insn (shadow_mem, asan_shadow_cst (shadow_bytes));
>  	  offset += ASAN_RED_ZONE_SIZE;
>  	}
>        cur_shadow_byte = ASAN_STACK_MAGIC_MIDDLE;

	Jakub

Re: Sparc ASAN

[Konstantin Serebryany]

On Wed, Nov 21, 2012 at 1:05 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Nov 20, 2012 at 11:19:33PM -0500, David Miller wrote:
>> From: David Miller <davem@davemloft.net>
>> Date: Tue, 20 Nov 2012 21:20:40 -0500 (EST)
>>
>> > Those seem to be the only problems that need to be resolved for this
>> > feature to be fully working.
>>
>> FWIW, here are the changes I am using which, besides the sparc backend
>> bits, has some temporary workarounds for the issues I brought up.
>
> To explain a little bit these unaligned stores.
> From what I understand LLVM will always put all the ASAN protected
> automatic vars into a single block and align the whole block to 32 bytes
> (on i?86/x86_64 in GCC that is the backend dynamic stack realignment,
> those andq $-32, %rsp etc. in the prologue, on other targets rth added
> a release or two ago handling of these via alloca that where the alloca
> allocates up to the alignment - 1 bytes more and we add alignment - 1
> to the result of alloca and mask it).
> But it seemed nothing on the libasan relies on it actually being 32-byte
> aligned,

I think your explanation is correct wrt the default mode, where the
memory to shadow ratio (mapping scale) is 8.

With greater values of mapping scale, you need greater alignment, but
these modes where never used except for experiment. So, don't bother.

As for the unaligned stores: aren't they 2x more expensive on x86
compared to properly aligned ones?

--kcc

> so what I've implemented was keep the vars as aligned as they
> were before (well, the base), and just put the padding in between as if
> the base was 32-byte aligned and I wanted to align them all to 32-bytes
> (ignoring here in the description more aligned vars etc.).
> That of course means the shadow memory stores may be unaligned, but as
> i?86/x86_64 were the only supported targets initially, and the unaligned
> stores are IMHO pretty fast there, it isn't a big deal.
>
> Now, for strict alignment targets, the question is what is the fastest
> way to implement this.  There is the cost of dynamically realigning the
> stack var block (which isn't just about the alloca at the beginning, but
> also pretty much having one extra register reserved for the result of
> the alloca, used as the base to access all the local vars, sure, you can
> spill it, but if it is used frequently, it will likely be assigned to a hard
> reg most of its lifetime), and on the other side there is the cost of
> accessing the shadow memory at smaller granularity.
>
> If some target has the virtual-stack-vars reg not even 8 byte aligned,
> some realignment is required, as the shadow shift is 3.
> If it is just 8 byte aligned and we don't realign the block, then yes,
> shadow memory needs to be accessed by bytes, if it is 16 byte aligned,
> perhaps we could use 16-bit stores instead of just 8-bit ones.
> And perhaps the decision (for strict alignment arches) whether to realign or
> not the whole block could be best dynamic, for small number of asan
> protected vars (i.e. small number of expected shadow memory stores)
> we could just not realign and use 8-bit or 16-bit shadow memory stores,
> for larger amounts of vars perhaps it might be cheaper to realign.
>
> So, concerning your patch, which implements basically the never realign,
> just sometimes use smaller size of shadow memory accesses, the decision
> shouldn't be done solely based on STRICT_ALIGNMENT, but needs to take
> into account the alignment of virtual_stack_vars_rtx in the current
> function, if it's byte alignment is >= (4 << ASAN_SHADOW_SHIFT), then
> you can use the same code as for !STRICT_ALIGNMENT, if it's byte alignment
> is >= (2 << ASAN_SHADOW_SHIFT), then you can use HImode stores, otherwise
> QImode.
>
>> --- a/gcc/asan.c
>> +++ b/gcc/asan.c
>> @@ -321,7 +321,10 @@ asan_emit_stack_protection (rtx base, HOST_WIDE_INT *offsets, tree *decls,
>>                             NULL_RTX, 1, OPTAB_DIRECT);
>>    gcc_assert (asan_shadow_set != -1
>>             && (ASAN_RED_ZONE_SIZE >> ASAN_SHADOW_SHIFT) == 4);
>> -  shadow_mem = gen_rtx_MEM (SImode, shadow_base);
>> +  if (STRICT_ALIGNMENT)
>> +    shadow_mem = gen_rtx_MEM (QImode, shadow_base);
>> +  else
>> +    shadow_mem = gen_rtx_MEM (SImode, shadow_base);
>>    set_mem_alias_set (shadow_mem, asan_shadow_set);
>>    prev_offset = base_offset;
>>    for (l = length; l; l -= 2)
>> @@ -349,7 +352,20 @@ asan_emit_stack_protection (rtx base, HOST_WIDE_INT *offsets, tree *decls,
>>             }
>>           else
>>             shadow_bytes[i] = ASAN_STACK_MAGIC_PARTIAL;
>> -       emit_move_insn (shadow_mem, asan_shadow_cst (shadow_bytes));
>> +       if (STRICT_ALIGNMENT)
>> +         {
>> +           for (i = 0; i < 4; i++)
>> +             {
>> +               rtx mem = adjust_address (shadow_mem, VOIDmode, i);
>> +               rtx val;
>> +
>> +               val = GEN_INT (trunc_int_for_mode (shadow_bytes[i],
>> +                                                  QImode));
>> +               emit_move_insn (mem, val);
>> +             }
>> +         }
>> +       else
>> +         emit_move_insn (shadow_mem, asan_shadow_cst (shadow_bytes));
>>         offset = aoff;
>>       }
>>        while (offset <= offsets[l - 2] - ASAN_RED_ZONE_SIZE)
>> @@ -359,7 +375,22 @@ asan_emit_stack_protection (rtx base, HOST_WIDE_INT *offsets, tree *decls,
>>                                      >> ASAN_SHADOW_SHIFT);
>>         prev_offset = offset;
>>         memset (shadow_bytes, cur_shadow_byte, 4);
>> -       emit_move_insn (shadow_mem, asan_shadow_cst (shadow_bytes));
>> +       if (STRICT_ALIGNMENT)
>> +         {
>> +           int i;
>> +
>> +           for (i = 0; i < 4; i++)
>> +             {
>> +               rtx mem = adjust_address (shadow_mem, VOIDmode, i);
>> +               rtx val;
>> +
>> +               val = GEN_INT (trunc_int_for_mode (shadow_bytes[i],
>> +                                                  QImode));
>> +               emit_move_insn (mem, val);
>> +             }
>> +         }
>> +       else
>> +         emit_move_insn (shadow_mem, asan_shadow_cst (shadow_bytes));
>>         offset += ASAN_RED_ZONE_SIZE;
>>       }
>>        cur_shadow_byte = ASAN_STACK_MAGIC_MIDDLE;
>
>         Jakub

Re: Sparc ASAN

[Peter Bergner]

On Tue, 2012-11-20 at 23:19 -0500, David Miller wrote:
> The address violation detection seems to work properly and the only
> thing that seems to be left are some backtrace/unwind issues.  These
> are perhaps similar to the unwind bits that the powerpc folks ran
> into.

David, does the following patch (will have some fuzz since I removed
one ppc only hunk from the patch) fix your backtrace issue?  I'll note
you'll have to add "|| defined(__sparc__)" to the #if ... or as
it's probably going to turn out, just replace the whole thing
with a "#if !defined(__i386__) && !defined(__x86_64__)".

Peter


Index: libsanitizer/asan/asan_linux.cc
===================================================================
--- libsanitizer/asan/asan_linux.cc	(revision 193678)
+++ libsanitizer/asan/asan_linux.cc	(working copy)
@@ -134,11 +141,27 @@
 #endif
 }
 
+uptr Unwind_GetBP(struct _Unwind_Context *ctx) {
+  return _Unwind_GetCFA(ctx);
+}
+
+struct Unwind_Trace_Info {
+  StackTrace *stack;
+  uptr bp;
+};
+
 _Unwind_Reason_Code Unwind_Trace(struct _Unwind_Context *ctx,
     void *param) {
-  StackTrace *b = (StackTrace*)param;
+  Unwind_Trace_Info *p = (Unwind_Trace_Info *)param;
+  StackTrace *b = p->stack;
+  uptr pc = Unwind_GetIP(ctx);
+  if (Unwind_GetBP(ctx) == p->bp) {
+    // We just encountered the frame pointer we want to start
+    // our backtrace with, so empty the backtrace before adding
+    // this frame to the backtrace.
+    b->size = 0;
+  }
   CHECK(b->size < b->max_size);
-  uptr pc = Unwind_GetIP(ctx);
   b->trace[b->size++] = pc;
   if (b->size == b->max_size) return UNWIND_STOP;
   return UNWIND_CONTINUE;
@@ -149,8 +172,11 @@
   stack->trace[0] = pc;
   if ((max_s) > 1) {
     stack->max_size = max_s;
-#ifdef __arm__
-    _Unwind_Backtrace(Unwind_Trace, stack);
+#if defined(__arm__) || defined(__powerpc__) || defined(__powerpc64__)
+    Unwind_Trace_Info param;
+    param.stack = stack;
+    param.bp = bp;
+    _Unwind_Backtrace(Unwind_Trace, &param);
 #else
     if (!asan_inited) return;
     if (AsanThread *t = asanThreadRegistry().GetCurrent())

Re: Sparc ASAN

[David Miller]

From: Peter Bergner <bergner@vnet.ibm.com>
Date: Wed, 21 Nov 2012 11:28:51 -0600

> On Tue, 2012-11-20 at 23:19 -0500, David Miller wrote:
>> The address violation detection seems to work properly and the only
>> thing that seems to be left are some backtrace/unwind issues.  These
>> are perhaps similar to the unwind bits that the powerpc folks ran
>> into.
> 
> David, does the following patch (will have some fuzz since I removed
> one ppc only hunk from the patch) fix your backtrace issue?  I'll note
> you'll have to add "|| defined(__sparc__)" to the #if ... or as
> it's probably going to turn out, just replace the whole thing
> with a "#if !defined(__i386__) && !defined(__x86_64__)".

This patch works well but I have some unrelated sanitizer sparc
issues to resolve before the testcase will pass properly.

Feel free to submit this with the __sparc__ cpp test added, or
the !x86 variant, at your discretion.

Re: Sparc ASAN

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 21 Nov 2012 12:54:17 -0500 (EST)

> From: Peter Bergner <bergner@vnet.ibm.com>
> Date: Wed, 21 Nov 2012 11:28:51 -0600
> 
>> On Tue, 2012-11-20 at 23:19 -0500, David Miller wrote:
>>> The address violation detection seems to work properly and the only
>>> thing that seems to be left are some backtrace/unwind issues.  These
>>> are perhaps similar to the unwind bits that the powerpc folks ran
>>> into.
>> 
>> David, does the following patch (will have some fuzz since I removed
>> one ppc only hunk from the patch) fix your backtrace issue?  I'll note
>> you'll have to add "|| defined(__sparc__)" to the #if ... or as
>> it's probably going to turn out, just replace the whole thing
>> with a "#if !defined(__i386__) && !defined(__x86_64__)".
> 
> This patch works well but I have some unrelated sanitizer sparc
> issues to resolve before the testcase will pass properly.
> 
> Feel free to submit this with the __sparc__ cpp test added, or
> the !x86 variant, at your discretion.

Actually I looked more closely at this, and the trigger is hit one
stack frame too late on sparc.

The BP computed in the memcmp() interceptor is the frame pointer
%fp, but on sparc that's the CFA of the caller, main() in the
case of the memcmp-1.c testcase.

So only main() appears in the backtrace.

It might be easier to implement this by comparing the PC instead.

And it also occurs to me that we probably need to be using
__builtin_extract_return_addr() when recording the PC at the
error trigger point.

Re: Sparc ASAN

[Jakub Jelinek]

On Wed, Nov 21, 2012 at 03:27:16PM -0500, David Miller wrote:
> Actually I looked more closely at this, and the trigger is hit one
> stack frame too late on sparc.

Are you testing with -fno-builtin-memcmp?  Without it the check is done
directly in main...

	Jakub

Re: Sparc ASAN

[David Miller]

From: Jakub Jelinek <jakub@redhat.com>
Date: Wed, 21 Nov 2012 21:30:37 +0100

> On Wed, Nov 21, 2012 at 03:27:16PM -0500, David Miller wrote:
>> Actually I looked more closely at this, and the trigger is hit one
>> stack frame too late on sparc.
> 
> Are you testing with -fno-builtin-memcmp?  Without it the check is done
> directly in main...

I made sure that the error triggered in the ASAN memcmp interceptor,
please read the paragraph after the one you are quoting.

Re: Sparc ASAN

[Peter Bergner]

On Wed, 2012-11-21 at 15:27 -0500, David Miller wrote:
> Actually I looked more closely at this, and the trigger is hit one
> stack frame too late on sparc.
> 
> The BP computed in the memcmp() interceptor is the frame pointer
> %fp, but on sparc that's the CFA of the caller, main() in the
> case of the memcmp-1.c testcase.
> 
> So only main() appears in the backtrace.
> 
> It might be easier to implement this by comparing the PC instead.
> 
> And it also occurs to me that we probably need to be using
> __builtin_extract_return_addr() when recording the PC at the
> error trigger point.

If you have a suggested change/patch that does that, let me know
and I can try it out on powerpc to make sure it works for us too.

Peter

Re: Sparc ASAN

[David Miller]

From: Peter Bergner <bergner@vnet.ibm.com>
Date: Wed, 21 Nov 2012 17:46:57 -0600

> If you have a suggested change/patch that does that, let me know
> and I can try it out on powerpc to make sure it works for us too.

I will try to do so, but I am pretty sure at this point that
it will end up being some time early next week.

Re: Sparc ASAN

[Richard Henderson]

On 2012-11-21 11:28, Peter Bergner wrote:
> +  if (Unwind_GetBP(ctx) == p->bp) {

I've mentioned multiple times that BP is unusable on most RISC.
You need to be looking at SP.


r~

Re: Sparc ASAN (was Re: sparc bootstrap still broken)

[Konstantin Serebryany]

On Wed, Nov 21, 2012 at 6:20 AM, David Miller <davem@davemloft.net> wrote:
> From: David Miller <davem@davemloft.net>
> Date: Tue, 20 Nov 2012 14:59:10 -0500 (EST)
>
>> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
>> Date: Tue, 20 Nov 2012 23:52:48 +0400
>>
>>> Please apply whatever minimal patch required to unbreak the SPARC
>>> build.  We will not be accepting any non-trivial patches until we
>>> set up semi-automated way to pull the upstream sources.
>>
>> Will do.
>
> I tossed together a quick sparc implementation and there seems to
> be only two issues to fix:
>
> 1) As noticed by the powerpc people, you have to probe the page
>    size of the system properly.  It's variable even within a
>    target/OS.
>
>    Probably a new hook, implemented in asan_linux.cc via:
>
>         #include <unistd.h>
>
>         uptr GetPageSize()
>         {
>           return getpagesize();
>         }

Today, kPageSize is used in several places where it is expected to be
a compile-time constant.
Even if it seems like replacing it with GetPageSize() is safe, it
would need very significant testing (including performance testing).
Can we just define kPageSize=1UL<<13 under ifdef __sparc__?

What are the possible page size values for SPARC?

>
>    I would just get rid of kPageSizeBits,

Indeed, this is not used any more. Removed.
http://llvm.org/viewvc/llvm-project?rev=168426&view=rev

--kcc

>  rather than compute it
>    dynamically as well, as it's really not used as far as I can tell.
>
>    The mmap of the shadow area won't work on sparc without this being
>    resolved.
>
> 2) The current code emitted to set the shadow values results in
>    unaligned stores.  For example, for the memcmp-1.c test on 32-bit
>    we get:
>
>    0x10488 <main+8>:    add  %fp, -160, %i5
>  ...
>    0x10490 <main+16>:   sethi  %hi(0xf1f1f000), %g2
>  ...
>    0x104a0 <main+32>:   srl  %i5, 3, %i5
>  ...
>    0x104a8 <main+40>:   or  %g2, 0x1f1, %g2
>    0x104ac <main+44>:   sethi  %hi(0x20000000), %g1
>  ...
> => 0x104b4 <main+52>:   st  %g2, [ %i5 + %g1 ]
>
>    Here %fp is 0xffffd538, and this %i5 + %g1 is 0x3ffffa93, which
>    is not aligned properly for a 32-bit store.
>
>    Therefore, this won't work for any STRICT_ALIGNMENT target.
>
> Those seem to be the only problems that need to be resolved for this
> feature to be fully working.

Re: Sparc ASAN

[David Miller]

From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
Date: Wed, 21 Nov 2012 17:39:05 +0400

> Today, kPageSize is used in several places where it is expected to be
> a compile-time constant.
> Even if it seems like replacing it with GetPageSize() is safe, it
> would need very significant testing (including performance testing).
> Can we just define kPageSize=1UL<<13 under ifdef __sparc__?
> 
> What are the possible page size values for SPARC?

4K, 8K, 64K, 512K

It's not a constant and the library really cannot expect it to be one.

Re: Sparc ASAN

[Konstantin Serebryany]

On Wed, Nov 21, 2012 at 7:29 PM, David Miller <davem@davemloft.net> wrote:
> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
> Date: Wed, 21 Nov 2012 17:39:05 +0400
>
>> Today, kPageSize is used in several places where it is expected to be
>> a compile-time constant.
>> Even if it seems like replacing it with GetPageSize() is safe, it
>> would need very significant testing (including performance testing).
>> Can we just define kPageSize=1UL<<13 under ifdef __sparc__?
>>
>> What are the possible page size values for SPARC?
>
> 4K, 8K, 64K, 512K
>
> It's not a constant and the library really cannot expect it to be one.

How often 64K and 512K are used?
If we use kPageSize=8K, will this cover most of the use cases?

x86 also supports 2M and 1G pages. asan does not support those and no
one complained so far.
There are various other things that asan library does not support.

--kcc

Re: Sparc ASAN

[Peter Bergner]

On Wed, 2012-11-21 at 19:39 +0400, Konstantin Serebryany wrote:
> On Wed, Nov 21, 2012 at 7:29 PM, David Miller <davem@davemloft.net> wrote:
> > From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
> > Date: Wed, 21 Nov 2012 17:39:05 +0400
> >
> >> Today, kPageSize is used in several places where it is expected to be
> >> a compile-time constant.
> >> Even if it seems like replacing it with GetPageSize() is safe, it
> >> would need very significant testing (including performance testing).
> >> Can we just define kPageSize=1UL<<13 under ifdef __sparc__?
> >>
> >> What are the possible page size values for SPARC?
> >
> > 4K, 8K, 64K, 512K
> >
> > It's not a constant and the library really cannot expect it to be one.
> 
> How often 64K and 512K are used?
> If we use kPageSize=8K, will this cover most of the use cases?

To answer this for PowerPC, most current 64-bit distro kernels are
compiled with 64K pages, although some older kernel still out there
are compiled with 4K pages.  For 32-bit kernels, I believe they
usually default to 4K pages.  As with SPARC, the kernel can be
configured to use any number of different page sizes.

An 8K page size won't work for us or a SPARC system with page size
above 8K, since the minimum mmap size is a page size, so if you
attempt to mmap something smaller than a page size (eg, 8K on a
64K page size system), mmap returns an error.  If this stays a
constant, you have to err on the large side of the potential
page sizes.

I agree with David, that this has to be runtime generated value.
I'll play with his GetPageSize() suggestion and see whether it
works for us.

Peter

Re: Sparc ASAN

[Konstantin Serebryany]

On Wed, Nov 21, 2012 at 8:05 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> On Wed, 2012-11-21 at 19:39 +0400, Konstantin Serebryany wrote:
>> On Wed, Nov 21, 2012 at 7:29 PM, David Miller <davem@davemloft.net> wrote:
>> > From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
>> > Date: Wed, 21 Nov 2012 17:39:05 +0400
>> >
>> >> Today, kPageSize is used in several places where it is expected to be
>> >> a compile-time constant.
>> >> Even if it seems like replacing it with GetPageSize() is safe, it
>> >> would need very significant testing (including performance testing).
>> >> Can we just define kPageSize=1UL<<13 under ifdef __sparc__?
>> >>
>> >> What are the possible page size values for SPARC?
>> >
>> > 4K, 8K, 64K, 512K
>> >
>> > It's not a constant and the library really cannot expect it to be one.
>>
>> How often 64K and 512K are used?
>> If we use kPageSize=8K, will this cover most of the use cases?
>
> To answer this for PowerPC, most current 64-bit distro kernels are
> compiled with 64K pages, although some older kernel still out there
> are compiled with 4K pages.  For 32-bit kernels, I believe they
> usually default to 4K pages.  As with SPARC, the kernel can be
> configured to use any number of different page sizes.
>
> An 8K page size won't work for us or a SPARC system with page size
> above 8K, since the minimum mmap size is a page size, so if you
> attempt to mmap something smaller than a page size (eg, 8K on a
> 64K page size system), mmap returns an error.  If this stays a
> constant, you have to err on the large side of the potential
> page sizes.
>
> I agree with David, that this has to be runtime generated value.
> I'll play with his GetPageSize() suggestion and see whether it
> works for us.

We'll need to be very careful with such change (e.g. there are other
constants defined using kPageSize,
there are other places where it is used in performance critical code).
And such change should go only via upstream.

Maybe we can solve the problem by changing kMmapGranularity? (On
Win32, we have 4k pages but larger kMmapGranularity)
Changing kMmapGranularity to a non-constant is much less intrusive.

--kcc

>
> Peter
>
>
>
>

Re: Sparc ASAN

[Andrew Pinski]

On Wed, Nov 21, 2012 at 8:21 AM, Konstantin Serebryany
<konstantin.s.serebryany@gmail.com> wrote:
> On Wed, Nov 21, 2012 at 8:05 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
>> On Wed, 2012-11-21 at 19:39 +0400, Konstantin Serebryany wrote:
>>> On Wed, Nov 21, 2012 at 7:29 PM, David Miller <davem@davemloft.net> wrote:
>>> > From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
>>> > Date: Wed, 21 Nov 2012 17:39:05 +0400
>>> >
>>> >> Today, kPageSize is used in several places where it is expected to be
>>> >> a compile-time constant.
>>> >> Even if it seems like replacing it with GetPageSize() is safe, it
>>> >> would need very significant testing (including performance testing).
>>> >> Can we just define kPageSize=1UL<<13 under ifdef __sparc__?
>>> >>
>>> >> What are the possible page size values for SPARC?
>>> >
>>> > 4K, 8K, 64K, 512K
>>> >
>>> > It's not a constant and the library really cannot expect it to be one.
>>>
>>> How often 64K and 512K are used?
>>> If we use kPageSize=8K, will this cover most of the use cases?
>>
>> To answer this for PowerPC, most current 64-bit distro kernels are
>> compiled with 64K pages, although some older kernel still out there
>> are compiled with 4K pages.  For 32-bit kernels, I believe they
>> usually default to 4K pages.  As with SPARC, the kernel can be
>> configured to use any number of different page sizes.
>>
>> An 8K page size won't work for us or a SPARC system with page size
>> above 8K, since the minimum mmap size is a page size, so if you
>> attempt to mmap something smaller than a page size (eg, 8K on a
>> 64K page size system), mmap returns an error.  If this stays a
>> constant, you have to err on the large side of the potential
>> page sizes.
>>
>> I agree with David, that this has to be runtime generated value.
>> I'll play with his GetPageSize() suggestion and see whether it
>> works for us.
>
> We'll need to be very careful with such change (e.g. there are other
> constants defined using kPageSize,
> there are other places where it is used in performance critical code).
> And such change should go only via upstream.
>
> Maybe we can solve the problem by changing kMmapGranularity? (On
> Win32, we have 4k pages but larger kMmapGranularity)
> Changing kMmapGranularity to a non-constant is much less intrusive.


Just to follow up, MIPS64 have the following (selectable at kernel
compile time) page sizes:
4k, 8k, 16k, 32k and 64k.  So is another target where the page size is
not constant just like PPC.  Some people use 16k page sizes if they
are also going to use huge pages too.  Some people use 64k pages if
they want good performance without huge pages (though the use of 64k
pages might be less of a performance gain with the support of
transparent huge pages).

Thanks,
Andrew

>
> --kcc
>
>>
>> Peter
>>
>>
>>
>>

Re: Sparc ASAN

[Andreas Schwab]

Andrew Pinski <pinskia@gmail.com> writes:

> Just to follow up, MIPS64 have the following (selectable at kernel
> compile time) page sizes:
> 4k, 8k, 16k, 32k and 64k.  So is another target where the page size is
> not constant just like PPC.

As does ia64 (common page sizes are 16k and 64k).

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Sparc ASAN

[Ramana Radhakrishnan]

On 11/21/12 16:27, Andrew Pinski wrote:
> On Wed, Nov 21, 2012 at 8:21 AM, Konstantin Serebryany
> <konstantin.s.serebryany@gmail.com> wrote:
>> On Wed, Nov 21, 2012 at 8:05 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
>>> On Wed, 2012-11-21 at 19:39 +0400, Konstantin Serebryany wrote:
>>>> On Wed, Nov 21, 2012 at 7:29 PM, David Miller <davem@davemloft.net> wrote:
>>>>> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
>>>>> Date: Wed, 21 Nov 2012 17:39:05 +0400
>>>>>
>>>>>> Today, kPageSize is used in several places where it is expected to be
>>>>>> a compile-time constant.
>>>>>> Even if it seems like replacing it with GetPageSize() is safe, it
>>>>>> would need very significant testing (including performance testing).
>>>>>> Can we just define kPageSize=1UL<<13 under ifdef __sparc__?
>>>>>>
>>>>>> What are the possible page size values for SPARC?
>>>>>
>>>>> 4K, 8K, 64K, 512K
>>>>>
>>>>> It's not a constant and the library really cannot expect it to be one.
>>>>
>>>> How often 64K and 512K are used?
>>>> If we use kPageSize=8K, will this cover most of the use cases?
>>>
>>> To answer this for PowerPC, most current 64-bit distro kernels are
>>> compiled with 64K pages, although some older kernel still out there
>>> are compiled with 4K pages.  For 32-bit kernels, I believe they
>>> usually default to 4K pages.  As with SPARC, the kernel can be
>>> configured to use any number of different page sizes.
>>>
>>> An 8K page size won't work for us or a SPARC system with page size
>>> above 8K, since the minimum mmap size is a page size, so if you
>>> attempt to mmap something smaller than a page size (eg, 8K on a
>>> 64K page size system), mmap returns an error.  If this stays a
>>> constant, you have to err on the large side of the potential
>>> page sizes.
>>>
>>> I agree with David, that this has to be runtime generated value.
>>> I'll play with his GetPageSize() suggestion and see whether it
>>> works for us.
>>
>> We'll need to be very careful with such change (e.g. there are other
>> constants defined using kPageSize,
>> there are other places where it is used in performance critical code).
>> And such change should go only via upstream.
>>
>> Maybe we can solve the problem by changing kMmapGranularity? (On
>> Win32, we have 4k pages but larger kMmapGranularity)
>> Changing kMmapGranularity to a non-constant is much less intrusive.
>
>
> Just to follow up, MIPS64 have the following (selectable at kernel
> compile time) page sizes:
> 4k, 8k, 16k, 32k and 64k.


And ... AArch allows for 4k and 64k page sizes, AArch64 currently 
supports both 4k and 64k page sizes in the Linux kernel, AArch32 (ARM 
GNU/Linux) currently supports only 4K.

I also notice that you've hard coded cache line size. Why is that 
necessary and what's it used for ?

While trying to add support for ARM (AArch32 GNU / Linux) implementation 
for GCC after-hours but still keep seeing failures on my chromebook 
running an ubuntu fs on a ChrOS kernel, because the shadow memory 
apparently overlaps with normal memory. Has anyone else hit this while 
porting ? Turning off address space randomization doesn't seem to help 
either.

regards,
Ramana

Re: Sparc ASAN

[Konstantin Serebryany]

On Wed, Nov 21, 2012 at 9:13 PM, Ramana Radhakrishnan <ramrad01@arm.com> wrote:
> On 11/21/12 16:27, Andrew Pinski wrote:
>>
>> On Wed, Nov 21, 2012 at 8:21 AM, Konstantin Serebryany
>> <konstantin.s.serebryany@gmail.com> wrote:
>>>
>>> On Wed, Nov 21, 2012 at 8:05 PM, Peter Bergner <bergner@vnet.ibm.com>
>>> wrote:
>>>>
>>>> On Wed, 2012-11-21 at 19:39 +0400, Konstantin Serebryany wrote:
>>>>>
>>>>> On Wed, Nov 21, 2012 at 7:29 PM, David Miller <davem@davemloft.net>
>>>>> wrote:
>>>>>>
>>>>>> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
>>>>>> Date: Wed, 21 Nov 2012 17:39:05 +0400
>>>>>>
>>>>>>> Today, kPageSize is used in several places where it is expected to be
>>>>>>> a compile-time constant.
>>>>>>> Even if it seems like replacing it with GetPageSize() is safe, it
>>>>>>> would need very significant testing (including performance testing).
>>>>>>> Can we just define kPageSize=1UL<<13 under ifdef __sparc__?
>>>>>>>
>>>>>>> What are the possible page size values for SPARC?
>>>>>>
>>>>>>
>>>>>> 4K, 8K, 64K, 512K
>>>>>>
>>>>>> It's not a constant and the library really cannot expect it to be one.
>>>>>
>>>>>
>>>>> How often 64K and 512K are used?
>>>>> If we use kPageSize=8K, will this cover most of the use cases?
>>>>
>>>>
>>>> To answer this for PowerPC, most current 64-bit distro kernels are
>>>> compiled with 64K pages, although some older kernel still out there
>>>> are compiled with 4K pages.  For 32-bit kernels, I believe they
>>>> usually default to 4K pages.  As with SPARC, the kernel can be
>>>> configured to use any number of different page sizes.
>>>>
>>>> An 8K page size won't work for us or a SPARC system with page size
>>>> above 8K, since the minimum mmap size is a page size, so if you
>>>> attempt to mmap something smaller than a page size (eg, 8K on a
>>>> 64K page size system), mmap returns an error.  If this stays a
>>>> constant, you have to err on the large side of the potential
>>>> page sizes.
>>>>
>>>> I agree with David, that this has to be runtime generated value.
>>>> I'll play with his GetPageSize() suggestion and see whether it
>>>> works for us.
>>>
>>>
>>> We'll need to be very careful with such change (e.g. there are other
>>> constants defined using kPageSize,
>>> there are other places where it is used in performance critical code).
>>> And such change should go only via upstream.
>>>
>>> Maybe we can solve the problem by changing kMmapGranularity? (On
>>> Win32, we have 4k pages but larger kMmapGranularity)
>>> Changing kMmapGranularity to a non-constant is much less intrusive.
>>
>>
>>
>> Just to follow up, MIPS64 have the following (selectable at kernel
>> compile time) page sizes:
>> 4k, 8k, 16k, 32k and 64k.
>
>
>
> And ... AArch allows for 4k and 64k page sizes, AArch64 currently supports
> both 4k and 64k page sizes in the Linux kernel, AArch32 (ARM GNU/Linux)
> currently supports only 4K.



>
> I also notice that you've hard coded cache line size. Why is that necessary
> and what's it used for ?

They are used to do padding in structures to avoid false sharing.


>
> While trying to add support for ARM (AArch32 GNU / Linux) implementation for
> GCC after-hours but still keep seeing failures on my chromebook running an
> ubuntu fs on a ChrOS kernel, because the shadow memory apparently overlaps
> with normal memory. Has anyone else hit this while porting ?

I've heard someone complain about this recently.
glider?

Running asan on *huge* binaries in 32-bit address space is a challenge indeed.
My suggestion was to link with -pie.

--kcc

> Turning off
> address space randomization doesn't seem to help either.
>
> regards,
> Ramana
>
>

Re: Sparc ASAN

[Ramana Radhakrishnan]

>>
>> While trying to add support for ARM (AArch32 GNU / Linux) implementation for
>> GCC after-hours but still keep seeing failures on my chromebook running an
>> ubuntu fs on a ChrOS kernel, because the shadow memory apparently overlaps
>> with normal memory. Has anyone else hit this while porting ?
>
> I've heard someone complain about this recently.
> glider?
>
> Running asan on *huge* binaries in 32-bit address space is a challenge indeed.
> My suggestion was to link with -pie.

No, this was just with one of the tests in the testsuite. So I'm not 
sure that it's that big to warrant pie.

Oh and funnily enough it worked fine from running outside the GCC 
testsuite harness but fails when running inside it ! I'll try and get 
more details out later today or tomorrow.

regards,
Ramana


>
> --kcc
>
>> Turning off
>> address space randomization doesn't seem to help either.
>>
>> regards,
>> Ramana
>>
>>
>

Re: Sparc ASAN

[Alexander Potapenko]

>> While trying to add support for ARM (AArch32 GNU / Linux) implementation for
>> GCC after-hours but still keep seeing failures on my chromebook running an
>> ubuntu fs on a ChrOS kernel, because the shadow memory apparently overlaps
>> with normal memory. Has anyone else hit this while porting ?
>
> I've heard someone complain about this recently.
> glider?
>
> Running asan on *huge* binaries in 32-bit address space is a challenge indeed.
> My suggestion was to link with -pie.
>
> --kcc
Yes, there were complaints about that recently: http://crbug.com/159816
-pie MAY help, but not if the program occupies too much address space.

Re: Sparc ASAN

[David Miller]

From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
Date: Wed, 21 Nov 2012 19:39:52 +0400

> There are various other things that asan library does not support.

I'm trying to understand why making the page size variable
is such a difficult endeavour.

Re: Sparc ASAN

[Konstantin Serebryany]

On Wed, Nov 21, 2012 at 8:40 PM, David Miller <davem@davemloft.net> wrote:
> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
> Date: Wed, 21 Nov 2012 19:39:52 +0400
>
>> There are various other things that asan library does not support.
>
> I'm trying to understand why making the page size variable
> is such a difficult endeavour.

Maybe it's not *that* difficult.
Looking at it carefully, the major problem I can see is that some
other constants are defined based on this one.
Give me a few days to resolve it...
http://code.google.com/p/address-sanitizer/issues/detail?id=128

Re: Sparc ASAN

[Konstantin Serebryany]

On Wed, Nov 21, 2012 at 9:05 PM, Konstantin Serebryany
<konstantin.s.serebryany@gmail.com> wrote:
> On Wed, Nov 21, 2012 at 8:40 PM, David Miller <davem@davemloft.net> wrote:
>> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
>> Date: Wed, 21 Nov 2012 19:39:52 +0400
>>
>>> There are various other things that asan library does not support.
>>
>> I'm trying to understand why making the page size variable
>> is such a difficult endeavour.
>
> Maybe it's not *that* difficult.
> Looking at it carefully, the major problem I can see is that some
> other constants are defined based on this one.
> Give me a few days to resolve it...
> http://code.google.com/p/address-sanitizer/issues/detail?id=128

 http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193849 removes the
kPageSize constant in favor of a function call.
Please give it a try.

BTW, libsanitizer now has a usable process to quickly pull the upstream changes.
It should make it easier for us to iterate on platform-specific patches.

--kcc

Re: Sparc ASAN

[David Miller]

From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
Date: Tue, 27 Nov 2012 18:12:00 +0400

> On Wed, Nov 21, 2012 at 9:05 PM, Konstantin Serebryany
> <konstantin.s.serebryany@gmail.com> wrote:
>> On Wed, Nov 21, 2012 at 8:40 PM, David Miller <davem@davemloft.net> wrote:
>>> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
>>> Date: Wed, 21 Nov 2012 19:39:52 +0400
>>>
>>>> There are various other things that asan library does not support.
>>>
>>> I'm trying to understand why making the page size variable
>>> is such a difficult endeavour.
>>
>> Maybe it's not *that* difficult.
>> Looking at it carefully, the major problem I can see is that some
>> other constants are defined based on this one.
>> Give me a few days to resolve it...
>> http://code.google.com/p/address-sanitizer/issues/detail?id=128
> 
>  http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193849 removes the
> kPageSize constant in favor of a function call.
> Please give it a try.
> 
> BTW, libsanitizer now has a usable process to quickly pull the upstream changes.
> It should make it easier for us to iterate on platform-specific patches.

So, with the hacks for unaligned accesses, Sparc seems to be working
here.

The only changes to libsantizier is to put __sparc__ checks where
__powerpc__ checks exist in the unwind code.

Are there any clear thoughts about what we should do in the end
wrt. the stack ASAN alignment issues?  Do we plan to 32-byte
align the stack variables or similar?  Otherwise we need to add
some ugly code to do half-word/byte at a time accesses, as needed.

Re: Sparc ASAN

[Konstantin Serebryany]

On Mon, Dec 3, 2012 at 10:02 PM, David Miller <davem@davemloft.net> wrote:
> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
> Date: Tue, 27 Nov 2012 18:12:00 +0400
>
>> On Wed, Nov 21, 2012 at 9:05 PM, Konstantin Serebryany
>> <konstantin.s.serebryany@gmail.com> wrote:
>>> On Wed, Nov 21, 2012 at 8:40 PM, David Miller <davem@davemloft.net> wrote:
>>>> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
>>>> Date: Wed, 21 Nov 2012 19:39:52 +0400
>>>>
>>>>> There are various other things that asan library does not support.
>>>>
>>>> I'm trying to understand why making the page size variable
>>>> is such a difficult endeavour.
>>>
>>> Maybe it's not *that* difficult.
>>> Looking at it carefully, the major problem I can see is that some
>>> other constants are defined based on this one.
>>> Give me a few days to resolve it...
>>> http://code.google.com/p/address-sanitizer/issues/detail?id=128
>>
>>  http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193849 removes the
>> kPageSize constant in favor of a function call.
>> Please give it a try.
>>
>> BTW, libsanitizer now has a usable process to quickly pull the upstream changes.
>> It should make it easier for us to iterate on platform-specific patches.
>
> So, with the hacks for unaligned accesses, Sparc seems to be working
> here.
>
> The only changes to libsantizier is to put __sparc__ checks where
> __powerpc__ checks exist in the unwind code.

Like this?

===================================================================
--- asan/asan_linux.cc  (revision 169136)
+++ asan/asan_linux.cc  (working copy)
@@ -158,7 +158,9 @@
   stack->trace[0] = pc;
   if ((max_s) > 1) {
     stack->max_size = max_s;
-#if defined(__arm__) || defined(__powerpc__) || defined(__powerpc64__)
+#if defined(__arm__) || \
+    defined(__powerpc__) || defined(__powerpc64__) || \
+    defined(__sparc__)
     _Unwind_Backtrace(Unwind_Trace, stack);
     // Pop off the two ASAN functions from the backtrace.
     stack->PopStackFrames(2);




>
> Are there any clear thoughts about what we should do in the end
> wrt. the stack ASAN alignment issues?  Do we plan to 32-byte
> align the stack variables or similar?  Otherwise we need to add
> some ugly code to do half-word/byte at a time accesses, as needed.

The LLVM implementation always used 32-byte alignment for stack redzones.
I never actually did any performance checking on x86 (32-byte aligned
vs 8-byte aligned),
although I suspect 32-byte aligned redzones should be ~2x faster.
8-byte aligned redzones trade some CPU for some stack memory and
probably slightly increase
the chances of catching large (33+ bytes off) buffer overflows.

For strict alignment architectures 8-byte aligned redzones is
obviously not a choice.
We either need to align the redzones by 32 always, or for some platforms.
Either is fine for me.

--kcc

Re: Sparc ASAN

[David Miller]

From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
Date: Mon, 3 Dec 2012 22:18:56 +0400

> On Mon, Dec 3, 2012 at 10:02 PM, David Miller <davem@davemloft.net> wrote:
>> The only changes to libsantizier is to put __sparc__ checks where
>> __powerpc__ checks exist in the unwind code.
> 
> Like this?
> 
> ===================================================================
> --- asan/asan_linux.cc  (revision 169136)
> +++ asan/asan_linux.cc  (working copy)
> @@ -158,7 +158,9 @@
>    stack->trace[0] = pc;
>    if ((max_s) > 1) {
>      stack->max_size = max_s;
> -#if defined(__arm__) || defined(__powerpc__) || defined(__powerpc64__)
> +#if defined(__arm__) || \
> +    defined(__powerpc__) || defined(__powerpc64__) || \
> +    defined(__sparc__)
>      _Unwind_Backtrace(Unwind_Trace, stack);
>      // Pop off the two ASAN functions from the backtrace.
>      stack->PopStackFrames(2);

Yes, that's perfect.

We could also add a __sparc__ block to sanitizer_stacktrace.cc:patch_pc().
The Sparc PC is actually 8 bytes after the caller's jump.  Sparc has
a delay slot, the place to return to is 2 instructions after the call/jump,
and instructions are all 4 bytes long.

> We either need to align the redzones by 32 always, or for some platforms.
> Either is fine for me.

I'm ambivalent as well.

Re: Sparc ASAN

[Konstantin Serebryany]

On Mon, Dec 3, 2012 at 10:29 PM, David Miller <davem@davemloft.net> wrote:
> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
> Date: Mon, 3 Dec 2012 22:18:56 +0400
>
>> On Mon, Dec 3, 2012 at 10:02 PM, David Miller <davem@davemloft.net> wrote:
>>> The only changes to libsantizier is to put __sparc__ checks where
>>> __powerpc__ checks exist in the unwind code.
>>
>> Like this?
>>
>> ===================================================================
>> --- asan/asan_linux.cc  (revision 169136)
>> +++ asan/asan_linux.cc  (working copy)
>> @@ -158,7 +158,9 @@
>>    stack->trace[0] = pc;
>>    if ((max_s) > 1) {
>>      stack->max_size = max_s;
>> -#if defined(__arm__) || defined(__powerpc__) || defined(__powerpc64__)
>> +#if defined(__arm__) || \
>> +    defined(__powerpc__) || defined(__powerpc64__) || \
>> +    defined(__sparc__)
>>      _Unwind_Backtrace(Unwind_Trace, stack);
>>      // Pop off the two ASAN functions from the backtrace.
>>      stack->PopStackFrames(2);
>
> Yes, that's perfect.
>
> We could also add a __sparc__ block to sanitizer_stacktrace.cc:patch_pc().
> The Sparc PC is actually 8 bytes after the caller's jump.  Sparc has
> a delay slot, the place to return to is 2 instructions after the call/jump,
> and instructions are all 4 bytes long.

Like this?

--- sanitizer_common/sanitizer_stacktrace.cc    (revision 169136)
+++ sanitizer_common/sanitizer_stacktrace.cc    (working copy)
@@ -36,6 +36,8 @@
 #if defined(__powerpc__) || defined(__powerpc64__)
   // PCs are always 4 byte aligned.
   return pc - 4;
+#elif defined(__sparc__)
+  return pc - 8;
 #else
   return pc - 1;
 #endif



>
>> We either need to align the redzones by 32 always, or for some platforms.
>> Either is fine for me.
>
> I'm ambivalent as well.

Re: Sparc ASAN

[David Miller]

From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
Date: Mon, 3 Dec 2012 22:33:12 +0400

> On Mon, Dec 3, 2012 at 10:29 PM, David Miller <davem@davemloft.net> wrote:
>> We could also add a __sparc__ block to sanitizer_stacktrace.cc:patch_pc().
>> The Sparc PC is actually 8 bytes after the caller's jump.  Sparc has
>> a delay slot, the place to return to is 2 instructions after the call/jump,
>> and instructions are all 4 bytes long.
> 
> Like this?
> 
> --- sanitizer_common/sanitizer_stacktrace.cc    (revision 169136)
> +++ sanitizer_common/sanitizer_stacktrace.cc    (working copy)
> @@ -36,6 +36,8 @@
>  #if defined(__powerpc__) || defined(__powerpc64__)
>    // PCs are always 4 byte aligned.
>    return pc - 4;
> +#elif defined(__sparc__)
> +  return pc - 8;
>  #else
>    return pc - 1;
>  #endif
> 

Perfect.

Re: Sparc ASAN

[Konstantin Serebryany]

On Mon, Dec 3, 2012 at 10:37 PM, David Miller <davem@davemloft.net> wrote:
> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
> Date: Mon, 3 Dec 2012 22:33:12 +0400
>
>> On Mon, Dec 3, 2012 at 10:29 PM, David Miller <davem@davemloft.net> wrote:
>>> We could also add a __sparc__ block to sanitizer_stacktrace.cc:patch_pc().
>>> The Sparc PC is actually 8 bytes after the caller's jump.  Sparc has
>>> a delay slot, the place to return to is 2 instructions after the call/jump,
>>> and instructions are all 4 bytes long.
>>
>> Like this?
>>
>> --- sanitizer_common/sanitizer_stacktrace.cc    (revision 169136)
>> +++ sanitizer_common/sanitizer_stacktrace.cc    (working copy)
>> @@ -36,6 +36,8 @@
>>  #if defined(__powerpc__) || defined(__powerpc64__)
>>    // PCs are always 4 byte aligned.
>>    return pc - 4;
>> +#elif defined(__sparc__)
>> +  return pc - 8;
>>  #else
>>    return pc - 1;
>>  #endif
>>
>
> Perfect.

http://llvm.org/viewvc/llvm-project?view=rev&revision=169141
Will reach gcc with the next libsanitizer merge (or feel free to
commit the same patch directly to gcc).

--kcc

Re: Sparc ASAN

[David Miller]

From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
Date: Mon, 3 Dec 2012 22:44:15 +0400

> On Mon, Dec 3, 2012 at 10:37 PM, David Miller <davem@davemloft.net> wrote:
>> From: Konstantin Serebryany <konstantin.s.serebryany@gmail.com>
>> Date: Mon, 3 Dec 2012 22:33:12 +0400
>>
>>> On Mon, Dec 3, 2012 at 10:29 PM, David Miller <davem@davemloft.net> wrote:
>>>> We could also add a __sparc__ block to sanitizer_stacktrace.cc:patch_pc().
>>>> The Sparc PC is actually 8 bytes after the caller's jump.  Sparc has
>>>> a delay slot, the place to return to is 2 instructions after the call/jump,
>>>> and instructions are all 4 bytes long.
>>>
>>> Like this?
>>>
>>> --- sanitizer_common/sanitizer_stacktrace.cc    (revision 169136)
>>> +++ sanitizer_common/sanitizer_stacktrace.cc    (working copy)
>>> @@ -36,6 +36,8 @@
>>>  #if defined(__powerpc__) || defined(__powerpc64__)
>>>    // PCs are always 4 byte aligned.
>>>    return pc - 4;
>>> +#elif defined(__sparc__)
>>> +  return pc - 8;
>>>  #else
>>>    return pc - 1;
>>>  #endif
>>>
>>
>> Perfect.
> 
> http://llvm.org/viewvc/llvm-project?view=rev&revision=169141
> Will reach gcc with the next libsanitizer merge (or feel free to
> commit the same patch directly to gcc).

Thanks for taking care of this.

Re: Sparc ASAN

[Jakub Jelinek]

On Mon, Dec 03, 2012 at 10:18:56PM +0400, Konstantin Serebryany wrote:
> The LLVM implementation always used 32-byte alignment for stack redzones.
> I never actually did any performance checking on x86 (32-byte aligned
> vs 8-byte aligned),
> although I suspect 32-byte aligned redzones should be ~2x faster.

Why?  The 32-byte realigning has significant cost, plus often one
extra register is eaten (the DRAP register), even bigger cost on
non-i?86/x86_64 targets.

	Jakub

Re: Sparc ASAN

[Konstantin Serebryany]

On Mon, Dec 3, 2012 at 10:31 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Dec 03, 2012 at 10:18:56PM +0400, Konstantin Serebryany wrote:
>> The LLVM implementation always used 32-byte alignment for stack redzones.
>> I never actually did any performance checking on x86 (32-byte aligned
>> vs 8-byte aligned),
>> although I suspect 32-byte aligned redzones should be ~2x faster.
>
> Why?  The 32-byte realigning has significant cost, plus often one
> extra register is eaten (the DRAP register), even bigger cost on
> non-i?86/x86_64 targets.

Maybe because my understanding of x86 is rather old (or plain wrong).
I tried a micro benchmark on Xeon E5-2690 and unaligned strores are
just slightly more expensive (< 10%).
I'll do more benchmarks with the actual asan instrumentation ~ tomorrow.

So, I guess we need to align the redzones conditionally for sparc, etc.

--kcc

>
>         Jakub

Re: Sparc ASAN

[Jakub Jelinek]

On Mon, Dec 03, 2012 at 10:18:56PM +0400, Konstantin Serebryany wrote:
> The LLVM implementation always used 32-byte alignment for stack redzones.
> I never actually did any performance checking on x86 (32-byte aligned
> vs 8-byte aligned),
> although I suspect 32-byte aligned redzones should be ~2x faster.

If the ~2x faster comes from unaligned vs. aligned integer stores, I can't
spot anything like that on e.g.

__attribute__((noinline, noclone)) void
foo (int *p)
{
  int i;
  for (i = 0; i < 32; i++)
    p[i] = 0x12345678;
}

int
main (int argc, const char **argv)
{
  char buf[1024];
  int *p = &buf[argc - 1];
  int i;
  __builtin_printf ("%p\n", p);
  for (i = 0; i < 100000000; i++)
    foo (p);
  return 0;
}

Time with zero arguments (i.e. argc 1) is the same as time with 1, 2 or 3
arguments on SandyBridge CPU.  I guess there could be penalties on page
boundaries, etc., but I think hot caches is the usual operation on the
stack.

	Jakub

Re: Sparc ASAN

[Konstantin Serebryany]

I've committed a flag to the LLVM implementation to not realign the
stack (-mllvm -asan-realign-stack=0).
On Xeon W3690 I've measured no performance difference (tried C/C++
part of SPEC2006).
So, on x86 it's probably the right thing to not realign the stack.

--kcc

On Mon, Dec 3, 2012 at 10:41 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Dec 03, 2012 at 10:18:56PM +0400, Konstantin Serebryany wrote:
>> The LLVM implementation always used 32-byte alignment for stack redzones.
>> I never actually did any performance checking on x86 (32-byte aligned
>> vs 8-byte aligned),
>> although I suspect 32-byte aligned redzones should be ~2x faster.
>
> If the ~2x faster comes from unaligned vs. aligned integer stores, I can't
> spot anything like that on e.g.
>
> __attribute__((noinline, noclone)) void
> foo (int *p)
> {
>   int i;
>   for (i = 0; i < 32; i++)
>     p[i] = 0x12345678;
> }
>
> int
> main (int argc, const char **argv)
> {
>   char buf[1024];
>   int *p = &buf[argc - 1];
>   int i;
>   __builtin_printf ("%p\n", p);
>   for (i = 0; i < 100000000; i++)
>     foo (p);
>   return 0;
> }
>
> Time with zero arguments (i.e. argc 1) is the same as time with 1, 2 or 3
> arguments on SandyBridge CPU.  I guess there could be penalties on page
> boundaries, etc., but I think hot caches is the usual operation on the
> stack.
>
>         Jakub