Page MenuHomePhabricator

Handle /align option.
ClosedPublic

Authored by ruiu on Aug 5 2019, 1:49 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ruiu created this revision.Aug 5 2019, 1:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2019, 1:49 AM
ruiu edited the summary of this revision. (Show Details)Aug 6 2019, 5:42 AM
ruiu added a reviewer: rnk.
MaskRay added a subscriber: MaskRay.Aug 6 2019, 5:50 AM
MaskRay added inline comments.
lld/test/COFF/align.s
28 ↗(On Diff #213288)

Align the values?

ruiu marked an inline comment as done.Aug 6 2019, 5:57 AM
ruiu added inline comments.
lld/test/COFF/align.s
28 ↗(On Diff #213288)

This is as-is, and I guess that this is actually aligned vertically by some rule, as it looks like this line is aligned vertically with the following "CheckSum" and "Number" lines.

rnk accepted this revision.Aug 6 2019, 10:51 AM

lgtm with question, no need to re-review

lld/COFF/Writer.cpp
1201 ↗(On Diff #213288)

Should this be alignTo(sizeOfHeaders, config->align)? I assume sizeOfHeaders was less than 4096, so this shouldn't be a behavior change for a default alignment.

This revision is now accepted and ready to land.Aug 6 2019, 10:51 AM
Closed by commit rL368145: Handle /align option. (authored by ruiu, committed by ). · Explain WhyAug 7 2019, 3:15 AM
This revision was automatically updated to reflect the committed changes.
ruiu marked an inline comment as done.
ruiu added inline comments.Aug 7 2019, 3:16 AM
lld/COFF/Writer.cpp
1201 ↗(On Diff #213288)

Yeah, I think you are right. Fixed.