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

[Evgeniy Stepanov via Phabricator via llvm-commits]

eugenis added inline comments.


================
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.
----------------
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.



Repository:
  rL LLVM

https://reviews.llvm.org/D28589