[llvm] DLLExport public methods from SmallVector #127850
Conversation
|
@llvm/pr-subscribers-llvm-adt Author: Andrew Rogers (andrurogerz) ChangesOverviewAnnotate the BackgroundThis change is required as part of the overall project to build LLVM as a Windows DLL described in #109483. Without this change, LLVM tools fail to link. ValidationBuilt LLVM with MSVC on Windows 11: Verified the annotated interface no longer appears in the list of unresolved external symbols. Full diff: https://github.com/llvm/llvm-project/pull/127850.diff 1 Files Affected:
diff --git a/llvm/include/llvm/ADT/SmallVector.h b/llvm/include/llvm/ADT/SmallVector.h
index bd3e887e36bce..cda7c1de3db2a 100644
--- a/llvm/include/llvm/ADT/SmallVector.h
+++ b/llvm/include/llvm/ADT/SmallVector.h
@@ -66,13 +66,13 @@ template <class Size_T> class SmallVectorBase {
/// This is a helper for \a grow() that's out of line to reduce code
/// duplication. This function will report a fatal error if it can't grow at
/// least to \p MinSize.
- void *mallocForGrow(void *FirstEl, size_t MinSize, size_t TSize,
- size_t &NewCapacity);
+ LLVM_ABI void *mallocForGrow(void *FirstEl, size_t MinSize, size_t TSize,
+ size_t &NewCapacity);
/// This is an implementation of the grow() method which only works
/// on POD-like data types and is out of line to reduce code duplication.
/// This function will report a fatal error if it cannot increase capacity.
- void grow_pod(void *FirstEl, size_t MinSize, size_t TSize);
+ LLVM_ABI void grow_pod(void *FirstEl, size_t MinSize, size_t TSize);
public:
size_t size() const { return Size; }
|
andrurogerz
changed the title
|
I don't think that this is tractable this way. We need to scope the changes to a library at a time rather than an API at a time. There is going to be a large number of decorations that are needed as we start to get further. Can you collapse all the Support changes into a single PR and the set of changes for ADT into a single PR? |
I think a large change like this, no matter if split into a sequence of small PRs or not, requires an RFC on discourse. |
|
@kuhar that RFC/discussion has happened multiple times over now. The last time I brought this up was at https://discourse.llvm.org/t/supporting-llvm-build-llvm-dylib-on-windows. There was a follow up GSoC project https://discourse.llvm.org/t/support-clang-plugins-on-windows. There are plenty of related discussion on the forums. This work enables building LLVM as a DLL on Windows and to enable building plugins for clang on Windows. |
|
Having these discussion linked in the PR description would provide very valuable context. I realized they are linked in the referenced issue, but that's an extra layer of indirection that made me miss those RFCs. I followed some of the links and I think the most complete attempt was this draft PR #67502 by @tstellar . The scope of the changes seems very broad to me, perhaps it would be worth reviving the discourse thread and making sure this is the direction we want to pursue? Similarly, do we have a policy in place that will make us apply these macros in the future code? |
Agreed, that the change is going to be very large in total, but scoping it to per-library is a reasonable approach IMO.
Yes, @vgvassilev has a colleague who is looking into setting up ids to help catch missing annotations. I don't have the link for that conversation off hand unfortunately. |
cc-ing @mcbarton here as he reported some progress in private... |
@compnerd I will batch these PRs per library. I apologize for the noise. Are you OK with merging batches of annotation changes before a library has been entirely annotated? Or would you prefer I hold-off on PRs until each library is complete? The latter will be more challenging since "complete" is a bit of a moving target, and I'll need to maintain a set of outstanding patches over multiple weeks until everything builds properly.
@kuhar Thank you for the feedback. I will directly reference RFCs and prior art in my future PRs for this work. |
|
I think that it becomes difficult to see the full picture if we cannot get most of the library in a single pass. I know that complicates things, but it needs to be approximately complete where if there are small changes they can be batched up into a single change at the end. |
Please also keep track of the progress in the meta issue we have open. |
|
Before you do any further work on this project, please create a new discourse thread, which explains what the further plan for adding and maintaining these annotations is. There is a bunch of scattered discussion in various random pull requests, like #109702 or #113097. As this will be a sweeping, highly visible change across the entire codebase, it is important to have a central, up-to-date description of what is going on that further PRs can reference, and make sure that people are on board with your methodology before it starts getting implemented. |
I will probably leave @compnerd to comment in details. I don’t oppose your suggestion. In fact, multiple RFCs have already covered this topic extensively. However, the pull request discussions reveal that code owners need clarification on certain aspects of the work, which may be too specific or technical to have been fully addressed in the RFCs. I think that also confirms my personal observation that pull requests somehow are lower barrier for the community to engage(?). For instance, we encountered workarounds that initially seemed functional but turned out to be ODR violations. There isn't a clear place to discuss these finer details, as they often pertain to specific files within particular components. For years, we had the ideas and implementation plan but lacked the humanpower to execute them. Now, with several contributors available to take on the heavy lifting, we have a rare opportunity to move forward. Restarting discussions risks arriving at the same conclusions while potentially losing momentum and delaying this work for yet another N years. PS: I think we should update the existing rfc proposing ids as part of the pre-merge checks... |
FWIW, this has already had an RFC on Discourse (https://discourse.llvm.org/t/supporting-llvm-build-llvm-dylib-on-windows/58891) as well as has a meta issue for tracking progress (#109483). I agree that it's a sweeping change, but I believe the buy-in already exists and this work has been ongoing for quite a while (at least in Clang, I'm less up-to-date on the LLVM side). This is a very important project for getting plugins to work on Windows, which is a significant use case for Clang (especially our tooling, like clang-tidy), so hopefully LLVM is willing to take on this functionality as well. |
To avoid sounding like a broken record, I'll just point you to this to explain what my expectation is here: #113097 (comment) Basically, all I want is that when a maintainer receives a PR adding ABI annotations (or just sees them in code), there is some coherent, up-to-date documentation on what they are supposed to do (which is largely just "nothing"), so we don't have to repeat this discussion for the (N+1)th time. |
|
+1 to what @nikic. The current organization of the issues and PRs doesn't make it easy to understand the larger context without significant effort, like I explained in #127850 (comment).
You definitely have local buy-in, but I don't believe you reached global visibility to convince unrelated parties whose code this will end up modifying. A fresh PSA discourse post would go a long way IMO. |
This is reasonable, it probably should be somewhere near the coding style docs.
If it's just a PSA, that's fine. What I want to avoid is reopening the discussion of "do we want this" after we already said yes and people have put significant time and money into doing the work. A "here's a reminder of what we were doing and why, an update of where we're at, and an update of what's left to do" kind of post seems reasonable to me though. |
|
Thank you for the input. I put together a write-up summarizing the proposal and past efforts at https://discourse.llvm.org/t/building-llvm-as-a-windows-dll/85307 |
Would you consider putting a PSA or some other tag in the name to attract more attention? If I wasn't involved in this discussion I'd never open this thread because of the name that's windows dll specific. I may be oversimplifying, but as soon as you put 'Windows DLL' in the subject line I'd expect a very large portion of the community not invested in windows at all to scroll past. And the suggestion was to get attention of the larger community even if they are not interested in Windows in general. |
|
Even in the thread body, it doesn't read like a PSA and doesn't mention the larger impact until the middle with the section 'Proposal | Annotating all Public LLVM Symbols'. I don't think this thread uses a format that accomplishes what we discussed above:
|
|
@kuhar thank you for taking a look. I will restructure the thread to focus on the impact to developers instead organizing it as a project proposal. I will also revise the title. |
Overview
Annotate the
llvm::SmallVectorBase::mallocForGrowandllvm::SmallVectorBase::grow_podmethods so they are explicitly included in LLVM's public interface. When building LLVM as a Windows DLL, this annotation exports them from the DLL.Background
This change is required as part of the overall project to build LLVM as a Windows DLL described in #109483. Without this change, LLVM tools fail to link.
Validation
Built LLVM with MSVC on Windows 11:
Verified the annotated interface no longer appears in the list of unresolved external symbols.