[PATCH] D29586: [lsan] Enable LSan for arm Linux

[Maxim Ostapenko via Phabricator via llvm-commits]

m.ostapenko added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_linux.cc:1253
+  ((unsigned int *)child_stack)[1] = (uptr)arg;
+  register int r0 __asm__("r0") = flags;
+  register void *r1 __asm__("r1") = child_stack;
----------------
ygribov wrote:
> rengolin wrote:
> > ygribov wrote:
> > > rengolin wrote:
> > > > This sequence doesn't coincide with the order of parameters above, and can lead to unexpected results.
> > > > 
> > > > Why do you need to reserve the registers in this code at all?
> > > Why should it? The sole purpose of register annotations is to pass arguments to swi call below.
> > Right, I just realised the compiler will do the necessary moves.
> > 
> > This code is really opaque, can you add some comments like the ones above?
> Note that that's how it's done for other cores, including AArch64. All comments is given for x86 internal_clone implementation above (it basically says that this hairy code is inspired by Glibc clone implementation).
Yeah, the reason we have all that stuff is explained in x86_64 port. Anyway, I've added several comments to inline asm to improve readability. Looks better now? 


================
Comment at: lib/sanitizer_common/sanitizer_linux.cc:1268
+                       "bx      ip\n"
+#else
+                       "ldr     lr, [sp], #8\n"
----------------
rengolin wrote:
> If this is ARMv4T and no interwork, this will fail, as `blx` was introduced in ARMv5.
> 
> Check `compiler-rt/lib/builtins/assembly.h` for more info on the multiple ways to return.
Ah, OK, thanks. Fixed now.


Repository:
  rL LLVM

https://reviews.llvm.org/D29586