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

[Marco Elver via Phabricator via llvm-commits]

melver 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;
----------------
dvyukov wrote:
> 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?
Right, I think right now our biggest risk is binary size, so I'll change it to not add unnecessary atomics metadata. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143159