-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[Object][COFF][NFC] Make writeImportLibrary NativeExports argument optional. #81600
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.
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
Conversation
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-llvm-binary-utilities Author: Jacek Caban (cjacek) ChangesAs suggested in #81426. CC @zmodem Full diff: https://github.com/llvm/llvm-project/pull/81600.diff 1 Files Affected:
diff --git a/llvm/include/llvm/Object/COFFImportFile.h b/llvm/include/llvm/Object/COFFImportFile.h
index 23c3e6a1f0784a..f4671d7405dbda 100644
--- a/llvm/include/llvm/Object/COFFImportFile.h
+++ b/llvm/include/llvm/Object/COFFImportFile.h
@@ -135,6 +135,18 @@ struct COFFShortExport {
}
};
+/// Writes a COFF import library containing entries described by the Exports
+/// array.
+///
+/// For hybrid targets such as ARM64EC, additional native entry points can be
+/// exposed using the NativeExports parameter. When NativeExports is used, the
+/// output import library will expose these native ARM64 imports alongside the
+/// entries described in the Exports array. Such a library can be used for
+/// linking both ARM64EC and pure ARM64 objects, and the linker will pick only
+/// the exports relevant to the target platform.
+///
+/// For non-hybrid targets, the NativeExports parameter should not be used.
+/// Instead, pass std::nullopt or an empty array to this parameter.
Error writeImportLibrary(StringRef ImportName, StringRef Path,
ArrayRef<COFFShortExport> Exports,
ArrayRef<COFFShortExport> NativeExports,
|
/// the exports relevant to the target platform. | ||
/// | ||
/// For non-hybrid targets, the NativeExports parameter should not be used. | ||
/// Instead, pass std::nullopt or an empty array to this parameter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, as most users won't need to care about this parameter at this point, and as it seems like this change can be disruptive to users, what do you think about moving the new parameter to the end of the parameter list, and adding a default = std::nullopt
to it, to avoid forcing users to change right away?
I guess the usual LLVM policy is that we don't need to worry about downstream users, but IMO if we can avoid breaking them, that's of course even better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, I did that in the new version, thanks! As I mentioned in #81426, I'm ultimately planning to use it in all in-tree callers. But indeed, I don't expect it to be interesting for majority of downstream users and making it optional seems like a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think this seems reasonable. Do you agree @zmodem? It causes a bit of extra unnecessary back-and-forth here, but the change landed like <24h ago, so it's perhaps best to just back out of the unnecessary change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this lgtm.
As suggested in #81426.
CC @zmodem