[PATCH] D28589: [asan] Refactor instrumentation of globals.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc accepted this revision.
pcc added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1621-1628
+  unsigned SizeOfGlobalStruct = DL.getTypeAllocSize(Initializer->getType());
+  assert(isPowerOf2_32(SizeOfGlobalStruct) &&
+         "global metadata will not be padded appropriately");
+
+  // The MSVC linker always inserts padding when linking incrementally. We
+  // cope with that by aligning each struct to its size, which must be a power
+  // of two.
----------------
eugenis wrote:
> pcc wrote:
> > eugenis wrote:
> > > pcc wrote:
> > > > eugenis wrote:
> > > > > pcc wrote:
> > > > > > I guess we can move this part into `InstrumentGlobalsCOFF` as a separate functional change.
> > > > > Actually this is not COFF-specific. See https://reviews.llvm.org/D28573. The comment update got lost somehow.
> > > > Can you please update the comment, then?
> > > I think I've done that.
> > The way the comment is currently worded, it seems to be about just MSVC. I would explain why the alignment is needed in the first sentence, and reword the second sentence to something like "Also, the MSVC linker..."
> In fact, I don't see why we can not simply use the default alignment on non-COFF targets. As long as the size of the global is divisible by the alignment, there should be no padding.  By over-aligning, we waste up to 56 bytes.
> 
Makes sense to me. I'd make a separate change to use default alignment on non-COFF and see what the bots think.


Repository:
  rL LLVM

https://reviews.llvm.org/D28589