[compiler-rt][lsan] Add backup AArch64 register for use_registers test

[DavidSpickett]

Summary

On Ubuntu Focal x13 is used by something in the process of calling
sched_yield. Causing the test to fail depending on when the thread
is stopped.

Adding x14 works around this and the test passes consistently.

Not switching to only x14 because that could make other platforms
fail. With both we'll always find at least one and even if both
values are present we'll only get one report.

[DavidSpickett]

See: https://lab.llvm.org/staging/#/builders/158/builds/212

This silent bot is running focal vs bionic on the normal bots. The test fails when we're supposed to not find a leak, "registers=1".

Which seems backwards to me, wouldn't using registers allow us to find the leak? I'm sure I've got the wrong end of the stick but either way, this gets it passing again on focal.

I did check Apple's arm64 ABI they have the same temporary registers as the main AAPCS64 so it should be ok there too.

[oontvoo]

In D110931#3036366, @DavidSpickett wrote:

See: https://lab.llvm.org/staging/#/builders/158/builds/212

This silent bot is running focal vs bionic on the normal bots. The test fails when we're supposed to not find a leak, "registers=1".

Which seems backwards to me, wouldn't using registers allow us to find the leak? I'm sure I've got the wrong end of the stick but either way, this gets it passing again on focal.

The point of this test was to stick the pointer into some unused(*) register - then let lsan try to determine if anything still has a reference to the pointer, if there is, then it's not a leak.

• when you set registers=1, it lets lsan scan registers - so if it sees that x13 still has the reference then it's not a leak.
• when you set registers=0, it won't scan registers (as such it'd think that no one has reference to the memory, hence it must be a leak).

(*) this is where guest work kind of comes in - we don't know for sure which register is actually unused here.

I did check Apple's arm64 ABI they have the same temporary registers as the main AAPCS64 so it should be ok there too.

[DavidSpickett]

if anything still has a reference to the pointer, if there is, then it's not a leak.

Thanks for the explanation, now I get it. For whatever reason I was thinking that if we still had a reference that made it a leak but it's the other way around.

[oontvoo]

In D110931#3039292, @DavidSpickett wrote:

if anything still has a reference to the pointer, if there is, then it's not a leak.

Thanks for the explanation, now I get it. For whatever reason I was thinking that if we still had a reference that made it a leak but it's the other way around.

P.S: it was suggested to me that we could set clobbers for those registers in test. Could I trouble you to test that instead of setting an additional x14? (Sorry, I dont have Ubuntu ready)

[DavidSpickett]

it was suggested to me that we could set clobbers for those registers in test

I tried this and at least for what clang generates now, this is the only change:

# diff /tmp/without_clobber /tmp/with_clobber
44175c44175
<   42cc38:     528008e2        mov     w2, #0x47                       // #71
---
>   42cc38:     52800902        mov     w2, #0x48                       // #72

Which is in fact, the source line for the assert in main, since we added one line before for the clobber. Behavior is the same as you'd expect when it comes to the test results.

For adding a clobber to change the code it would have to be a temporary register that clang has decided to use in registers_thread_func. Then you'd get a save/restore but only around the inline asm and only if it was used after the inline asm. This wouldn't prevent the call to __sync_fetch_and_xor or sched_yield using it in their code as they can assume the caller stashed anything important.

What adding clobbers would do is prevent a situation where a future clang decides to use one of these registers as temp after the inline asm and we clobber it. Which would give you a rather strange failure, vs. if we add clobbers the program would at least execute properly and we just wouldn't find the value in the register.

The chances of clang doing that are pretty low given this is -O0 and the only thing it does after the inline asm that needs registers is call __sync_fetch_and_xor. The first argument to which is a function argument and the latter is a constant, so very high chance that it just keeps the argument in an argument register.

I also thought about doing the extreme end of this, where we set every register that's temporary in the ABI. This works and you can put clobbers to prevent what I mentioned above but adds noise so I'd stick with 2 if that works.

[zatrazz]

Would avoid calling sched_yield() help in this situation? Depending of the system default, calling the glibc symbol might result in a lazy-resolution and thus invoke a lot of loader's code (which might clobber temporary registers).

[DavidSpickett]

That works too. I'd be happy with either idea.

[vitalybuka]

LGTM as-is
sched_yield is probably OK as well.

[oontvoo]

LGTM Thanks!

compiler-rt/test/lsan/TestCases/use_registers.cpp

#elif defined(__mips__)
   asm("move $16, %0"
       :
       : "r"(p));
 #elif defined(__arm__)
   asm("mov r5, %0"
       :
       : "r"(p));
 #elif defined(__aarch64__)
-  // x9-10are used. x11-12 are probably used.
-  // So we pick x13 to be safe.
-  asm("mov x13, %0"
+  // x9-10 are used. x11-12 are probably used.
+  // So we pick x13 to be safe and x14 as a backup.
+  // (x13 known to be used on Ubuntu Focal)
+  asm("mov x13, %0\n"
+      "mov x14, %0"
       :
       : "r"(p));
 #elif defined(__powerpc__)
   asm("mr 30, %0"
       :
       : "r"(p));
 #elif defined(__s390x__)
   asm("lgr %%r10, %0"