Skip to content
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

[Object][COFF][NFC] Make writeImportLibrary NativeExports argument optional. #81600

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

cjacek
Copy link
Contributor

@cjacek cjacek commented Feb 13, 2024

As suggested in #81426.

CC @zmodem

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 13, 2024

@llvm/pr-subscribers-platform-windows
@llvm/pr-subscribers-lld-coff

@llvm/pr-subscribers-llvm-binary-utilities

Author: Jacek Caban (cjacek)

Changes

As suggested in #81426.

CC @zmodem


Full diff: https://github.com/llvm/llvm-project/pull/81600.diff

1 Files Affected:

  • (modified) llvm/include/llvm/Object/COFFImportFile.h (+12)
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.
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, this lgtm.

@cjacek cjacek changed the title [Object][COFF][NFC] Document writeImportLibrary. [Object][COFF][NFC] Make writeImportLibrary NativeExports argument optional. Feb 13, 2024
durin42 added a commit to durin42/rust that referenced this pull request Feb 13, 2024
@cjacek cjacek merged commit 4612208 into llvm:main Feb 13, 2024
7 checks passed
@cjacek cjacek deleted the implib-doc branch February 13, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants