Skip to content

[LLD][COFF] Fix ARM64 EC chunks comparator #75407

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[LLD][COFF] Fix ARM64 EC chunks comparator #75407

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lld/COFF/Writer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1480,7 +1480,8 @@ void Writer::sortECChunks() {
llvm::stable_sort(sec->chunks, [=](const Chunk *a, const Chunk *b) {
std::optional<chpe_range_type> aType = a->getArm64ECRangeType(),
bType = b->getArm64ECRangeType();
return !aType || (bType && *aType < *bType);
return (!aType ? 0 : (unsigned)*aType + 1) <
(!bType ? 0 : (unsigned)*bType + 1);
Comment on lines +1483 to +1484
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks! C-style casts are generally discouraged in LLVM, in this case I think we could entirely avoid casts by tweaking the expression:

Suggested change
return (!aType ? 0 : (unsigned)*aType + 1) <
(!bType ? 0 : (unsigned)*bType + 1);
return bType && (!aType || *aType < *bType);

It would be also nice to have a test. In this case, reproducing it depends on STL implementation detail, so a mistake like this may not be caught on some setups, but I was able to reproduce the problem on Linux+libstdc++ with: https://gist.github.com/cjacek/7cac304bb03583d90452425cc71101b3

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting back! Since you already have a diff ready do you mind sending out the PR & landing it? I can close this in that case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I created #75495.

});
}
}
Expand Down