[Bug sanitizer/63850] New: Building TSAN for Aarch64 results in assembler

[Bug sanitizer/63850] New: Building TSAN for Aarch64 results in assembler

[venkataramanan.kumar at amd dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63850

            Bug ID: 63850
           Summary: Building TSAN for Aarch64 results in assembler
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: sanitizer
          Assignee: unassigned at gcc dot gnu.org
          Reporter: venkataramanan.kumar at amd dot com
                CC: dodji at gcc dot gnu.org, dvyukov at gcc dot gnu.org,
                    jakub at gcc dot gnu.org, kcc at gcc dot gnu.org

After enabling TSAN for Aarch64, building it results in Assembler errors.

tmp/cc02YOZf.s:22856: Error: unknown mnemonic `call' -- `call
__tsan_trace_switch_thunk'
/tmp/cc02YOZf.s:22856: Error: operand 1 should be an integer or stack
pointer register -- `add $1024,%rsp'
/tmp/cc02YOZf.s:23297: Error: operand 1 should be an integer or stack
pointer register -- `sub $1024,%rsp'


The function "TraceAddEvent" adds "HACKY_CALL" which is defined for X86_64.

void ALWAYS_INLINE TraceAddEvent(ThreadState *thr, FastState fs,
                                        EventType typ, u64 addr) {
  if (!kCollectHistory)
    return;
  DCHECK_GE((int)typ, 0);
  DCHECK_LE((int)typ, 7);
  DCHECK_EQ(GetLsb(addr, 61), addr);
  StatInc(thr, StatEvents);
  u64 pos = fs.GetTracePos();
  if (UNLIKELY((pos % kTracePartSize) == 0)) {
#ifndef TSAN_GO
    HACKY_CALL(__tsan_trace_switch);
#else
    TraceSwitch(thr);
#endif
  }
  Event *trace = (Event*)GetThreadTrace(fs.tid());
  Event *evp = &trace[pos];
  Event ev = (u64)addr | ((u64)typ << 61);
  *evp = ev;
}

}  // namespace __tsan


#if TSAN_DEBUG == 0
// The caller may not create the stack frame for itself at all,
// so we create a reserve stack frame for it (1024b must be enough).
#define HACKY_CALL(f) \
  __asm__ __volatile__("sub $1024, %%rsp;" \
                       CFI_INL_ADJUST_CFA_OFFSET(1024) \
                       ".hidden " #f "_thunk;" \
                       "call " #f "_thunk;" \
                       "add $1024, %%rsp;" \
                       CFI_INL_ADJUST_CFA_OFFSET(-1024) \
                       ::: "memory", "cc");
#else
#define HACKY_CALL(f) f()
#endif

Making #if 0 & TSAN_DEBUG == 0 also results in Assembler errors. TSAN make
system includes x86_64 assembly file.

I am looking at ways to fix it and functionally build TSAN for Aarch64

[Bug sanitizer/63850] Building TSAN for Aarch64 results in assembler Error

[dvyukov at google dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63850

Dmitry Vyukov <dvyukov at google dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dvyukov at google dot com

--- Comment #1 from Dmitry Vyukov <dvyukov at google dot com> ---
Hi,

The easiest way to disable disable the assembly is to switch hacky call to
normal call:

#if TSAN_DEBUG == 0
#define HACKY_CALL(f) \
  __asm__ __volatile__("sub $1024, %%rsp;" \
                       CFI_INL_ADJUST_CFA_OFFSET(1024) \
                       ".hidden " #f "_thunk;" \
                       "call " #f "_thunk;" \
                       "add $1024, %%rsp;" \
                       CFI_INL_ADJUST_CFA_OFFSET(-1024) \
                       ::: "memory", "cc");
#else
#define HACKY_CALL(f) f()
#endif

It should work, but just will be a bit slower.
We use this mode for Go language:

#ifndef TSAN_GO
    HACKY_CALL(__tsan_trace_switch);
#else
    TraceSwitch(thr);
#endif


But note that tsan is tested only on amd64 so far. So there can be other
issues. In particular, shadow memory layout and atomic operations.

Also any changes to tsan must go to llvm repo first and then be integrated into
gcc. Please refer to:
https://code.google.com/p/address-sanitizer/wiki/HowToContribute

[Bug sanitizer/63850] Building TSAN for Aarch64 results in assembler Error

[clyon at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63850

clyon at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2014-11-13
     Ever confirmed|0                           |1

--- Comment #2 from clyon at gcc dot gnu.org ---
Thanks for your comment. Since we are GCC developers, our plan is to work in
the GCC tree, then update our patches for submission in llvm. (Same way I did
for ASAN on ARM/AArch64).

As a side note, making HACKY_CALL be a normal call is not sufficient: a patch
in the Makefile is also needed, as it unconditionally involves an x86_64
assembly file.

[Bug sanitizer/63850] Building TSAN for Aarch64 results in assembler Error

[dvyukov at google dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63850

--- Comment #3 from Dmitry Vyukov <dvyukov at google dot com> ---
Sure, you can do local experimentation in gcc.
Yes, gcc Makefile will need to be updated as well.

But I am more concerned about shadow memory layout. Tsan mapping is somewhat
different from asan.

[Bug sanitizer/63850] Building TSAN for Aarch64 results in assembler Error

[vekumar at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63850

vekumar at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |vekumar at gcc dot gnu.org

--- Comment #4 from vekumar at gcc dot gnu.org ---

We did some changes in Makefile and configure under libsantizers to make it
build for Aarch64.

As clyon said local experiments are done in GCC tree. Just capturing the
changes here. 

Ref:
https://git.linaro.org/toolchain/gcc.git/commit/07178f9e98be4fc1efad5c5d7c4fed7627c17e1f

[Bug sanitizer/63850] Building TSAN for Aarch64 results in assembler Error

[dvyukov at google dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63850

--- Comment #5 from Dmitry Vyukov <dvyukov at google dot com> ---
I suspect that configure and Makefiles are gcc-specific, that is needs to be
submitted to gcc diredctly.
And for tsan_rtl.h we have a change in flight that does essentially the same
(for mips64 support):
http://reviews.llvm.org/D6291
I've asked the author to disable HACKY_CALL for everything except __x86_64__.
So it seems it can be a pure gcc change.

[Bug sanitizer/63850] Building TSAN for Aarch64 results in assembler Error

[clyon at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63850

--- Comment #6 from clyon at gcc dot gnu.org ---
Venkat,
Can you submit your GCC patch, in an accepable way? (no change to sanitizer
libs code, and obviously do not activate tsan by default)

[Bug sanitizer/63850] Building TSAN for Aarch64 results in assembler Error

[vekumar at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63850

--- Comment #7 from vekumar at gcc dot gnu.org ---
(In reply to clyon from comment #6)
> Venkat,
> Can you submit your GCC patch, in an accepable way? (no change to sanitizer
> libs code, and obviously do not activate tsan by default)

Okay I will send out a patch that modifies libsanitizer/configure.ac and
libsanitizer/tsan/Makefile.am that will separate out tsan_rtl_amd64.S from
getting build for other targets.


--- a/libsanitizer/tsan/tsan_rtl.h
+++ b/libsanitizer/tsan/tsan_rtl.h
@@ -679,7 +679,7 @@ void AcquireReleaseImpl(ThreadState *thr, uptr pc,
SyncClock *c);
 // The trick is that the call preserves all registers and the compiler
 // does not treat it as a call.
 // If it does not work for you, use normal call.
-#if TSAN_DEBUG == 0
+#if defined(__x86_64__) && TSAN_DEBUG == 0

This change is also acceptable?

[Bug sanitizer/63850] Building TSAN for Aarch64 results in assembler Error

[dvyukov at google dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63850

--- Comment #8 from Dmitry Vyukov <dvyukov at google dot com> ---
On Tue, Jan 20, 2015 at 9:06 AM, vekumar at gcc dot gnu.org
<gcc-bugzilla@gcc.gnu.org> wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63850
>
> --- Comment #7 from vekumar at gcc dot gnu.org ---
> (In reply to clyon from comment #6)
>> Venkat,
>> Can you submit your GCC patch, in an accepable way? (no change to sanitizer
>> libs code, and obviously do not activate tsan by default)
>
> Okay I will send out a patch that modifies libsanitizer/configure.ac and
> libsanitizer/tsan/Makefile.am that will separate out tsan_rtl_amd64.S from
> getting build for other targets.
>
>
> --- a/libsanitizer/tsan/tsan_rtl.h
> +++ b/libsanitizer/tsan/tsan_rtl.h
> @@ -679,7 +679,7 @@ void AcquireReleaseImpl(ThreadState *thr, uptr pc,
> SyncClock *c);
>  // The trick is that the call preserves all registers and the compiler
>  // does not treat it as a call.
>  // If it does not work for you, use normal call.
> -#if TSAN_DEBUG == 0
> +#if defined(__x86_64__) && TSAN_DEBUG == 0
>
> This change is also acceptable?

This change will be overwritten on the next integrate from upstream.
Changes to runtimes need to go to llvm repo. However, in this case we
just need to wait for mips64 change to be landed and it will fix this
define as well.