-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[Support] Add clang tooling generated explicit visibility macros #113097
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
Open
fsfod
wants to merge
5
commits into
llvm:main
Choose a base branch
from
fsfod:exported-api/gen-support
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
305f60a
[Support] Add clang tooling generated explicit visibility macros
fsfod 9422085
Add explicit visibility macros to Support source files
fsfod 26aa1fb
Merge branch 'main' into exported-api/gen-support
vgvassilev 39ade94
Reformat changes
fsfod cc46329
format again
fsfod File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Is it intentional that the classes in this file are not marked LLVM_ABI?
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.
Any class with no out of line members is skipped to help reduce the number of exports created for MSVC.
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.
Hm, do you have any idea what fraction of classes are like that? This seems like a bit of a foot-gun, as someone adding an out of line member is likely not going to check whether this is the first out of line member in the class and they have to add an ABI annotation.
Or is it expected that automation is going to check that all necessary annotations are present and we instead have to explicitly mark things that we don't want part of the ABI (with which macro? It is still the LIBRARY_VISIBILITY one?)
Generally I'd appreciate it if the PR included some useful context on what the wider maintenance plan here is supposed to be.
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 the idea is automate adding new annotations by running something like post commit action like gn syncbot does. There some discussion about this in the Clang PR #109702.
I just added a new macro LLVM_ABI_NOT_EXPORTED that the tool looks for, I never really though about reusing LIBRARY_VISIBILITY maybe i could. It would have to be not defined when the tool runs so the tool can define it as a clang annotate attribute it can find.
One of the extra goals is default symbol visibility is eventually set to hidden for non windows platforms to also reduce the number of exported symbols for them #19707.
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.
Some of this is also discussed in @compnerds old thread https://discourse.llvm.org/t/supporting-llvm-build-llvm-dylib-on-windows/58891
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.
It would generally be nice if the old LLVM_EXTERNAL_VISIBILTY, LLVM_LIBRARY_VISIBILITY and LLVM_ALWAYS_EXPORT and the new macros could be consolidated. Having 3 old macros on top of 5 new ones really doesn't make it easy to understand when you're supposed to use which one.
And on that note, it would be great to have a section in https://llvm.org/docs/ProgrammersManual.html that explains the different ABI macros and when you're supposed to use them. As these are going to be become widespread now, having a reference that we can point people to will be helpful. (There is some limited documentation in Compiler.h, but it does lose nuances like the "only use if there is an out of line member" guideline mentioned above.)
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.
This has been my concern as well. However, this is currently an intermediatory step. The desired end goal is to have ~3-4 of these macros for the libraries that we want to support.
In my mind the ones that we want:
LLVM_SUPPORT_ABI
- ABI surface for LLVMSupportLLVM_ABI
- ABI surface for LLVM (DSO)LLVM_C_ABI
- ABI surface for llvm-cCLANG_ABI
- ABI surface for clang (DSO)LLDB_ABI
- ABI surface for LLDB (DSO)This would allow most of the common uses of the library builds be possible on all the platforms. There is a need for splitting out LLVMSupport as it is so commonly used, and LLVM and LLVM C are separate as there is somewhat of an ABI guarantee on the LLVM C interface.
I agree that documenting them is important. However, my concern is that it is going to spread fear and confusion due to the current state. We need to get to the state where the macros are properly setup so that we are able to build the DLLs with the reduced macro set to demonstrate that we are in the right state. At that point, as this becomes part of the development workflow, it would be something that must be documented and have clear guidelines on how to use the tooling to verify (as well as builds).
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.
As the discussion on this PR and the clang one show, you already have fear and confusion now :)
I'd be fine with going ahead with this as long as there is some place we can point people to explaining what is going on here and what the plan going forward is. And by "some place" I don't mean "breadcrumbs spread across five different issues, PRs and discourse threads".
There should be a discourse post that says something along the lines of: We are going to be mass adding ABI annotations (because...), you should just ignore them for now (we'll add them to any new code ourselves), we're going to add automation (which does...?) to make sure they stay up to date once the bulk has landed. The annotations will only get used once we have automation and documentation in place.