r298851 breaks code that tries to initialise TLS on aarch64

[EdSchouten]

Bugzilla Link	35317
Resolution	FIXED
Resolved on	Nov 22, 2017 10:17
Version	5.0
OS	All
Blocks	#33840
CC	@rengolin,@TNorthover,@tstellar
Fixed by commit(s)	r318788 r318854

Extended Description

On CloudABI, I've observed crashes of processes on AArch64 when using Clang 5.0, though Clang 4.0 works perfectly fine. This is due to the changes made in SVN r298851.

On CloudABI, crt0 is responsible for allocating TLS and updating TPIDR_EL0 accordingly. Once completed, it will initialise some of the objects stored in TLS for the initial thread.

Setting up TLS:

• https://github.com/NuxiNL/cloudlibc/blob/master/src/crt/crt0.c#L381-L387
• https://github.com/NuxiNL/cloudlibc/blob/master/src/common/tls.h#L18-L28

Initialising TLS objects for the initial thread:

• https://github.com/NuxiNL/cloudlibc/blob/master/src/crt/crt0.c#L389-L401

With the changes made in r298851, mrs TPIDR_EL0 is marked as having no side-effects whatsoever. This is too strong, because an msr TPIDR_EL0 can affect the register's value. In other words, it is all right to combine and reorder mrs TPIDR_EL0 instructions, but we shouldn't be doing this across the msr.

I can imagine that the optimisation provided by r298851 would be worth having. If we want to leave it intact, wouldn't it make more sense to revert to the old behaviour, but assume that functions called in other compilation units will not clobber TPIDR_EL0?

If it is hard to fix this on short notice, could we please revert this change for 5.0.1?

[EdSchouten]

assigned to @tstellar

[tstellar]

Ed, are you able to provide a reduced LLVM IR test case?

[tstellar]

Chad, can you take a look at this?

[llvmbot]

Chad, can you take a look at this?

Yes. I'll start digging, but I don't know of a quick fix.

IIUC, the underlying issue is that we don't model the dependency between mrs (read system register) and msr (write system register) instructions whos source and destination register, respectively, match. Thus, something like the below should not be reordered:

msr TPIDR_EL0, x0 ;; set thread pointer
mrs x8, TPIDR_EL0 ;; read thread pointer

We would also need to add a dependency on all calls (which includes inline assembly) to avoid the case where the callee reads/writes a system register and the callsite is hoisted above/below the instruction that writes/reads the system register.

Tim/All,
I'm not aware of a simple way to model these dependencies, short of reverting the original change (r298851). Does anyone have any thoughts?

In discussing this with Geoff he pointed out this is only a problem because crt0 is being written in C (while most other targets use inline assembly that doesn't get reordered).

[tstellar]

Would it work to add implicit uses/defs of the system registers to the instructions?

[llvmbot]

Would it work to add implicit uses/defs of the system registers to the
instructions?

I think that would effectively inhibit all the optimizations so would be equivalent to just reverting the original patch. E.g. I looked at MachineLICM and it will not hoist any instructions that def/use physical registers.

We might be able to get some of the optimizations back by treating mrs as a load and msr as a store to a global memory location, though that would not optimize as aggressively in cases where there were e.g. regular stores and TLS pointer loads in the same loop since MachineCSE/MachineLICM don't do any real alias analysis.

[tstellar]

We are getting closer to the 5.0.1 release. Is everyone OK with reverting r298851?

[rengolin]

Tom, Chad,

I can't see any better solution for now.

I haven't dug that deep, but from what I can see, some kind of dependency model between MRS/MSR beyond load/store emulation may be needed, and that's not something we can do for 5.0.1. Adding a simplified hack would also not be a good idea.

Since the other solutions are akin to disabling the whole thing, I think the sanest thing we can do is to just revert it, and discuss a better implementation on trunk.

cheers,
--renato

[llvmbot]

Committed r318788 which marks a 'mrs' of TPIDR_EL0 (thread pointer) as having side effects. There was a dependency on part of r298851 due to r298851, so I thought this would be the least invasive solution.

[llvmbot]

Assigning to Tom for cherry-picking. I'm also happy to do it myself, so feel free to just send it back my way, Tom; whatever works best for you.

[tstellar]

Merged: 318854