[PATCH] D143159: [SanitizerBinaryMetadata] Treat constant globals and non-escaping addresses specially

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp:383
+      !PointerMayBeCaptured(Addr, true, true)) {
+    // The addressable object is on the stack and does not escape.
+    return false;
----------------
melver wrote:
> dvyukov wrote:
> > melver wrote:
> > > dvyukov wrote:
> > > > What if we pretend these are atomic as well?
> > > Marking things as atomic increases binary size. It's better to just not emit any metadata (less space used).
> > > 
> > > There are plenty function-local objects and if we mark all accesses to them as atomic, we'll end up with larger binaries.
> > > 
> > > Of course this will only help if all accesses in a function are non-escaping. If there's at least e.g. 1 global access, the function will end up being "covered" with atomics metadata again.
> > But I think it's highly-likely a function also contains non-local accesses (esp for optimized builds with inlining where functions tend to be larger).
> > Maybe we should skip functions that contain only atomics accesses and local/non-interesting accesses (contains at least 1 interesting access).
> > 
> Changed it to look for shared+mutable.
> 
> So functions that access only non-escaping addresses and constant globals will be excluded entirely.
> 
> If a function is already covered by atomics metadata, constant globals will be considered atomic.
> 
> However, I think we still shouldn't pretend accesses to non-escaping memory are atomic because we'll increase binary size. Constant accesses should be relatively rare and can easily be optimized.
Why this difference between global consts and non-escaping memory? Doesn't treating global consts as atomics also increase metadata size?
We marked coverage accesses as atomic to avoid "false" positives. But we don't need to do this for global consts.
If for non-escaping accesses it's profitable not mark them as atomics, shouldn't we do the same for global consts as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143159