[PATCH] D70762: scudo: Add initial memory tagging support.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc marked 2 inline comments as done.
pcc added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:35
+inline void disableMemoryTagChecks() {
+  __asm__ __volatile__(".arch_extension mte; msr tco, #1");
+}
----------------
eugenis wrote:
> I don't think it is the job of a memory allocator to mess with PSTATE.TCO.
> Let the caller deal with it?
> 
I can see the argument either way.

On one hand, TCO is a thread-wide property which the caller could arguably be considered responsible for.

On the other, disabling tag checks is required in order to avoid tag check failures for future allocations which reuse previously tagged chunks, so to a certain extent it makes sense to perform an operation that is required in order to keep the allocator working, even if it involves setting what is technically a thread-wide property. The operation that we perform depends on the allocator's implementation, so arguably it belongs with the allocator. If for example we switched to mprotecting without PROT_MTE when tag checks are disabled, there would be no need to set TCO here.

If you still think that it belongs in the caller, maybe we could document that setting TCO is required, but this may change in the future.


================
Comment at: compiler-rt/lib/scudo/standalone/wrappers_c.inc:180
+
+INTERFACE WEAK void SCUDO_PREFIX(malloc_disable_memory_tagging)() {
+  SCUDO_ALLOCATOR.disableMemoryTagging();
----------------
eugenis wrote:
> This api is not thread safe. TCO is per-thread, I think, so this would not work in a multi-threaded program at all. Either document this fact, or, preferably, remove the TCO code and use relaxed atomic for UseMemoryTagging.
Let's not use atomics for this without a use case. Due to the complexity of disabling MTE in a multi-threaded program, I don't think we should even attempt to support it for now.

I will document that this requires the program to be single threaded at the point when the function is called.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70762/new/

https://reviews.llvm.org/D70762