[PATCH] D26413: [XRay] Support AArch64 in compiler-rt

[Renato Golin via llvm-commits]

rengolin added inline comments.


================
Comment at: lib/xray/xray_trampoline_AArch64.S:32
+    /* Handler address is nullptr if handler is not set */
+    CMP X2, #0
+    BEQ FunctionEntry_restore
----------------
rSerge wrote:
> rengolin wrote:
> > can't you compare the address before saving and restoring all those registers?
> This situation is rare. Usually if the handler is set to `nullptr`, then the user code is unpatched, so the trampolines are not called. So there is no performance issue here.
> However, there is a race condition issue. It is important to check for `nullptr` handler as late as possible before its call. If we push the registers after checking for `nullptr` handler, we increase the chances that handler is not `nullptr` when it's checked, but `nullptr` when it's called.
> Though anyway we currently require that the handler function works correctly even if called after its removal via XRay API.
> This situation is rare. Usually if the handler is set to nullptr, then the user code is unpatched, so the trampolines are not called. So there is no performance issue here.

Right, makes sense.

> However, there is a race condition issue. It is important to check for nullptr handler as late as possible before its call. If we push the registers after checking for nullptr handler, we increase the chances that handler is not nullptr when it's checked, but nullptr when it's called.

I'm not sure I follow.

You have added the last update to be the first branch atomically, to make sure there are no race conditions. 

Is this about the users calling the thunk while the update is ongoing, which can see the code but still don't have a function pointer?

In any case, if there is a race condition, it needs to be fixed, not work by chance.


https://reviews.llvm.org/D26413