[PATCH] D34072: Fix alignment bug in COFF emission.

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann added inline comments.


================
Comment at: llvm/lib/Object/WindowsResource.cpp:511
+
+  Current += SectionOnePadding;
 }
----------------
zturner wrote:
> ecbeckmann wrote:
> > ruiu wrote:
> > > Do you have to memorize how large was a padding? I mean you may be able to do this.
> > > 
> > >   Current = alignTo(Current, 8)
> > Hmm doesn't that depend on the way the loader aligns cvtres in memory?  For example, it could be possible for the system to align the FileOutputBuffer on 4 bytes instead of 8 bytes.  So aligning to 8 bytes would actually be aligning to 4 bytes.
> Not sure I follow your comment here.  If the system aligns the `FileOutputBuffer` to 4 bytes (let's be explicit and say it puts it at address 4), and you call `Current = alignTo(Current, 8)` then after that call `Current` will be equal to 8, which is what you want, isn't it?
> 
> Seems to me like calling 
> 
> ```
> X = OffsetToAlignment(Number, Align)
> Number += X;
> ```
> 
> is mathematically equivalent to:
> 
> ```
> Number = alignTo(Number, Align);
> ```
No....this won't be what we want to do.

The end goal is to have the final COFF file be aligned on 8 bytes.  Therefore, we want to align with 8 bytes //relative to the FileOutputBuffer base address//, without regard to the buffer starting alignment.  If the address of the buffer starts at 4 and then we say
`Current = alignTo(Current, 8)`, then yes Current will have an absolute aligned-8 address //relative to llvm-cvtres memory//.  However, relative to the COFF file itself it will have alignment 4.


https://reviews.llvm.org/D34072