[llvm-objcopy][MachO] Use alignToPowerOf2 instead of alignTo#204033
Merged
drodriguez merged 2 commits intoJun 20, 2026
Conversation
During the review of llvm#203680 I noticed that Mach-O objcopy files seems to use `alignTo` and import `Alignment.h` to align some offsets to page boundaries and similar requirements. However, the `alignTo` in `Alignment.h`, while being intended for powers of 2, requires using an alignment of type `llvm::Align`, and needs explicit conversion from `uint64_t` and similar. Single `Alignment.h` includes `MathExtras.h`, the `alignTo` being invoked ends up being a generic `alignTo` that does not require powers of 2, and perform divisions and multiplications. While some of those might be optimized by the compiler into efficient power of 2 operations, there's an explicit `alignToPowerOf2` version that is optimized and asserts the alignment is a power of 2 (with asserts enabled). Since all the alignments should be power of 2 for the Mach-O binary format, change from `alignTo` to `alignToPowerOf2` to make the fact more visible (and get the extra safety net of the assertions). As expected, the test suite of objcopy doesn't show any regressions, but I have not done a performance benchmark around this either.
|
@llvm/pr-subscribers-llvm-binary-utilities Author: Daniel Rodríguez Troitiño (drodriguez) ChangesDuring the review of #203680 I noticed that Mach-O objcopy files seems to use As expected, the test suite of objcopy doesn't show any regressions, but I have not done a performance benchmark around this either. Full diff: https://github.com/llvm/llvm-project/pull/204033.diff 3 Files Affected:
diff --git a/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp b/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp
index 8660c903c617d..9f5b0511c17b3 100644
--- a/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp
+++ b/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp
@@ -7,9 +7,9 @@
//===----------------------------------------------------------------------===//
#include "MachOLayoutBuilder.h"
-#include "llvm/Support/Alignment.h"
#include "llvm/Support/Errc.h"
#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/MathExtras.h"
using namespace llvm;
using namespace llvm::objcopy::macho;
@@ -179,11 +179,11 @@ uint64_t MachOLayoutBuilder::layoutSegments() {
if (IsObjectFile) {
Offset += SegFileSize;
} else {
- Offset = alignTo(Offset + SegFileSize, PageSize);
- SegFileSize = alignTo(SegFileSize, PageSize);
+ Offset = alignToPowerOf2(Offset + SegFileSize, PageSize);
+ SegFileSize = alignToPowerOf2(SegFileSize, PageSize);
// Use the original vmsize if the segment is __PAGEZERO.
VMSize =
- Segname == "__PAGEZERO" ? SegmentVmSize : alignTo(VMSize, PageSize);
+ Segname == "__PAGEZERO" ? SegmentVmSize : alignToPowerOf2(VMSize, PageSize);
}
switch (MLC.load_command_data.cmd) {
@@ -289,18 +289,18 @@ Error MachOLayoutBuilder::layoutTail(uint64_t Offset) {
uint64_t StartOfCodeSignature = Offset;
uint32_t CodeSignatureSize = 0;
if (O.CodeSignatureCommandIndex) {
- StartOfCodeSignature = alignTo(StartOfCodeSignature, 16);
+ StartOfCodeSignature = alignToPowerOf2(StartOfCodeSignature, 16);
// Note: These calculations are to be kept in sync with the same
// calculations performed in LLD's CodeSignatureSection.
const uint32_t AllHeadersSize =
- alignTo(CodeSignature.FixedHeadersSize + OutputFileName.size() + 1,
+ alignToPowerOf2(CodeSignature.FixedHeadersSize + OutputFileName.size() + 1,
CodeSignature.Align);
const uint32_t BlockCount =
(StartOfCodeSignature + CodeSignature.BlockSize - 1) /
CodeSignature.BlockSize;
const uint32_t Size =
- alignTo(AllHeadersSize + BlockCount * CodeSignature.HashSize,
+ alignToPowerOf2(AllHeadersSize + BlockCount * CodeSignature.HashSize,
CodeSignature.Align);
CodeSignature.StartOffset = StartOfCodeSignature;
@@ -321,13 +321,13 @@ Error MachOLayoutBuilder::layoutTail(uint64_t Offset) {
case MachO::LC_SEGMENT:
MLC->segment_command_data.cmdsize = sizeof(MachO::segment_command);
MLC->segment_command_data.fileoff = StartOfLinkEdit;
- MLC->segment_command_data.vmsize = alignTo(LinkEditSize, PageSize);
+ MLC->segment_command_data.vmsize = alignToPowerOf2(LinkEditSize, PageSize);
MLC->segment_command_data.filesize = LinkEditSize;
break;
case MachO::LC_SEGMENT_64:
MLC->segment_command_64_data.cmdsize = sizeof(MachO::segment_command_64);
MLC->segment_command_64_data.fileoff = StartOfLinkEdit;
- MLC->segment_command_64_data.vmsize = alignTo(LinkEditSize, PageSize);
+ MLC->segment_command_64_data.vmsize = alignToPowerOf2(LinkEditSize, PageSize);
MLC->segment_command_64_data.filesize = LinkEditSize;
break;
}
diff --git a/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.h b/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.h
index 8d8716df22bba..22ae92249c437 100644
--- a/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.h
+++ b/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.h
@@ -33,8 +33,8 @@ struct CodeSignatureInfo {
// For each block, a SHA256 hash (256 bits, 32 bytes) is written to
// the CodeSignature section.
static constexpr size_t HashSize = 256 / 8;
- static constexpr size_t BlobHeadersSize = llvm::alignTo<8>(
- sizeof(llvm::MachO::CS_SuperBlob) + sizeof(llvm::MachO::CS_BlobIndex));
+ static constexpr size_t BlobHeadersSize = llvm::alignToPowerOf2(
+ sizeof(llvm::MachO::CS_SuperBlob) + sizeof(llvm::MachO::CS_BlobIndex), 8);
// The size of the entire header depends upon the filename the binary is being
// written to, but the rest of the header is fixed in size.
static constexpr uint32_t FixedHeadersSize =
diff --git a/llvm/lib/ObjCopy/MachO/MachOObjcopy.cpp b/llvm/lib/ObjCopy/MachO/MachOObjcopy.cpp
index e9a371ffa926f..0e56e6a00ec32 100644
--- a/llvm/lib/ObjCopy/MachO/MachOObjcopy.cpp
+++ b/llvm/lib/ObjCopy/MachO/MachOObjcopy.cpp
@@ -157,7 +157,7 @@ static void updateLoadCommandPayloadString(LoadCommand &LC, StringRef S) {
assert(isLoadCommandWithPayloadString(LC) &&
"unsupported load command encountered");
- uint32_t NewCmdsize = alignTo(sizeof(LCType) + S.size() + 1, 8);
+ uint32_t NewCmdsize = alignToPowerOf2(sizeof(LCType) + S.size() + 1, 8);
LC.MachOLoadCommand.load_command_data.cmdsize = NewCmdsize;
LC.Payload.assign(NewCmdsize - sizeof(LCType), 0);
@@ -169,7 +169,7 @@ static LoadCommand buildRPathLoadCommand(StringRef Path) {
MachO::rpath_command RPathLC;
RPathLC.cmd = MachO::LC_RPATH;
RPathLC.path = sizeof(MachO::rpath_command);
- RPathLC.cmdsize = alignTo(sizeof(MachO::rpath_command) + Path.size() + 1, 8);
+ RPathLC.cmdsize = alignToPowerOf2(sizeof(MachO::rpath_command) + Path.size() + 1, 8);
LC.MachOLoadCommand.rpath_command_data = RPathLC;
LC.Payload.assign(RPathLC.cmdsize - sizeof(MachO::rpath_command), 0);
llvm::copy(Path, LC.Payload.begin());
@@ -351,7 +351,7 @@ static Error addSection(const NewSectionInfo &NewSection, Object &Obj) {
// There's no segment named TargetSegName. Create a new load command and
// Insert a new section into it.
LoadCommand &NewSegment =
- Obj.addSegment(TargetSegName, alignTo(Sec.Size, 16384));
+ Obj.addSegment(TargetSegName, alignToPowerOf2(Sec.Size, 16384));
NewSegment.Sections.push_back(std::make_unique<Section>(Sec));
NewSegment.Sections.back()->Addr = *NewSegment.getSegmentVMAddr();
return Error::success();
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
jh7370
approved these changes
Jun 18, 2026
jh7370
left a comment
Contributor
There was a problem hiding this comment.
SGTM. I checked and all the cases are derived from constants, so there's no risk of an assertion being hit e.g. due to invalid user input.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/36136 Here is the relevant piece of the build log for the reference |
Contributor
Author
|
Seems like a timeout running tests and the later build in the same buildbot succeeded. I will ignore the failure as flaky, but if it keeps appearing, I can have a longer look. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
During the review of #203680 I noticed that Mach-O objcopy files seems to use
alignToand importAlignment.hto align some offsets to page boundaries and similar requirements. However, thealignToinAlignment.h, while being intended for powers of 2, requires using an alignment of typellvm::Align, and needs explicit conversion fromuint64_tand similar. SingleAlignment.hincludesMathExtras.h, thealignTobeing invoked ends up being a genericalignTothat does not require powers of 2, and perform divisions and multiplications. While some of those might be optimized by the compiler into efficient power of 2 operations, there's an explicitalignToPowerOf2version that is optimized and asserts the alignment is a power of 2 (with asserts enabled). Since all the alignments should be power of 2 for the Mach-O binary format, change fromalignTotoalignToPowerOf2to make the fact more visible (and get the extra safety net of the assertions).As expected, the test suite of objcopy doesn't show any regressions, but I have not done a performance benchmark around this either.