-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
dllimport conflicts with exclude_from_explicit_instantiation. #40363
Comments
assigned to @zmodem |
Basically, we have two things that say the function will be externally available, one from the language (extern template) and one extension (dllimport), and then we have our own, new extension (exclude_from_expicit_instantiation), which says the opposite. I guess the "negative" available_externally attribute should win, since it only exists to say that an inline function is not available_externally. I wonder if it makes sense to extend this attribute to work in non-template cases, consider: class EXPORT Foo { // One inline method that I don't want to export, for whatever reason: Obviously, the spelling "exclude_from_explicit_instantiation" doesn't make any sense, but what's really been created here is an available_externally-blocker attribute. The same effect can also be achieved with clang-cl /Zc:dllExportInlines-, by the way: http://blog.llvm.org/2018/11/30-faster-windows-builds-with-clang-cl_14.html Speculatively assigning to Hans since he supervised /Zc:dllExportInlines-. |
+Louis, since he added the attribute. |
(For anyone looking, that was r343790.)
That sounds reasonable to me. |
ldionne, as author of exclude_from_explicit_instantiation could you take a look? libc++ currently needs to carry a workaround for this. |
libc++ will need another workaround for this for https://bugs.chromium.org/p/chromium/issues/detail?id=923166#c44 |
Separately from what clang should do, does Chromium use the libc++ ABI unstable macros? Can it? If so, I would think that the "unstable ABI" build shouldn't use the exclude_from_explicit_instantiation attributes, since I believe those are only needed to stabilize the ABI. |
Yes, Chromium sets _LIBCPP_ABI_UNSTABLE (except, for now, in branded CrOS builds, but that's hopefully temporary). That sounds like a good idea to me. (Having said that, updating libc++ in Chromium is a bit of a mess atm, so a compiler-side fix might still help Chromium earlier even if we implemented that suggestion for libc++ very soon and the compiler fix a bit later.) |
Tentative fix: https://reviews.llvm.org/D155713 |
#40363 caused the C++20 `str() const &` and `str() &&` to be dllimport'ed despite _LIBCPP_HIDE_FROM_ABI. This is a temporary solution until #40363 is fixed. Reviewed By: #libc, hans, ldionne, Mordante Differential Revision: https://reviews.llvm.org/D155185
@llvm/issue-subscribers-clang-codegen |
Also `istringstream::str()` and `ostringstrem::str()`. #40363 causes `str()` to be dllimport'ed despite _LIBCPP_HIDE_FROM_ABI. This is a temporary solution until #40363 is fixed. Reviewed By: #libc, thakis, philnik Differential Revision: https://reviews.llvm.org/D157602
Also `istringstream::str()` and `ostringstrem::str()`. llvm/llvm-project#40363 causes `str()` to be dllimport'ed despite _LIBCPP_HIDE_FROM_ABI. This is a temporary solution until #40363 is fixed. Reviewed By: #libc, thakis, philnik Differential Revision: https://reviews.llvm.org/D157602 (cherry picked from commit 090996f)
Also `istringstream::str()` and `ostringstrem::str()`. llvm/llvm-project#40363 causes `str()` to be dllimport'ed despite _LIBCPP_HIDE_FROM_ABI. This is a temporary solution until #40363 is fixed. Reviewed By: #libc, thakis, philnik Differential Revision: https://reviews.llvm.org/D157602 (cherry picked from commit 090996f)
…stantiation members during explicit instantiation. Fixes llvm#40363
…stantiation members during explicit instantiation (llvm#65961) This is a continuation of https://reviews.llvm.org/D155713 Fixes llvm#40363
…stantiation members during explicit instantiation (llvm#65961) This is a continuation of https://reviews.llvm.org/D155713 Fixes llvm#40363
I had to revert my patch due to #66909 |
Not sure why this was marked completed again. The fix was reverted in 700f683 |
Apologies for this, there appears to be an issue around merging / pushing into private forks that causes behaviour such as the above. It has happened before and it is rather obnoxious:( |
Sorry, didn't mean to close this. Not sure how that happened. |
No problem, I guess it's a github thing :) |
I reported this unwanted closing at https://support.github.com/ticket/personal/0/2342354. Not sure if that's visible to others. |
Unfortunately, it's not visible to me. |
GitHub support replied regarding the unwanted closing:
|
Thank you for looking into this. Now I'm kind of scared to update the default branch of my not-quite-a-fork of llvm-project (it's actually a fork of an independent mirror that pulls from llvm-project in set intervals). Do we know if reverting a commit that automatically closed an issue also re-opens the issue automatically? If that's the case maybe what happened is that my local "fork" push happened to include the fix but not the revert, but I happened to push it after the revert had been done upstream, so it closed back the issue. If that wasn't it I fear that I will close every open issue that was automatically closed at one point via a "Fixes ..." commit message every time I update my fork, so I'll just avoid it from now on. And sorry for taking the conversation here off the rails, since it's not at all related to the issue. |
@pfusik Thanks a lot for looking into this with Github support. Can you bring that up in a Discourse thread and ping @tstellar and @tru ? I think this is something we need to carefully consider because LLVM is in the situation where a lot of people have write access to the repository, so this could happen quite often. I'm not so worried about people having bad intentions, but I am pretty worried about people doing it by mistake or even via automergers since those are quite common. |
Not sure we can do much about this? An action that would check every closed issue and see where it comes from and reopen in that case is the only thing I can think about. |
The only other thing I can think about is to change the way LLVM does business and restrict who has direct push access, and to go exclusively through PRs. That wouldn't solve this issue entirely, but it would make it less likely to happen. |
That seems like a very disruptive change and I am not sure it would be a very positive one for the community. But yeah. |
llvm/llvm-project#40363 caused the C++20 `str() const &` and `str() &&` to be dllimport'ed despite _LIBCPP_HIDE_FROM_ABI. This is a temporary solution until #40363 is fixed. Reviewed By: #libc, hans, ldionne, Mordante Differential Revision: https://reviews.llvm.org/D155185 NOKEYCHECK=True GitOrigin-RevId: 8ecb9591646d4b0326ea7547b99d300336c6c423
Also `istringstream::str()` and `ostringstrem::str()`. llvm/llvm-project#40363 causes `str()` to be dllimport'ed despite _LIBCPP_HIDE_FROM_ABI. This is a temporary solution until #40363 is fixed. Reviewed By: #libc, thakis, philnik Differential Revision: https://reviews.llvm.org/D157602 NOKEYCHECK=True GitOrigin-RevId: 090996f4a1186b1522e1225b6779d74ce9da250c
llvm/llvm-project#40363 caused the C++20 `str() const &` and `str() &&` to be dllimport'ed despite _LIBCPP_HIDE_FROM_ABI. This is a temporary solution until #40363 is fixed. Reviewed By: #libc, hans, ldionne, Mordante Differential Revision: https://reviews.llvm.org/D155185
Extended Description
When both dllimport and exclude_from_explicit_instantiation are passed, Clang still expects the function to have a externally available definition. But this is not the case.
For example: https://godbolt.org/z/gKOHCk
This leads to linker errors because of undefined symbols.
I think one approach would be to make Clang not inherit
dllimport
attributes on functions declared with exclude_from_explicit_instantiation. When dllimport is applied directly, we could emit an error for incompatible attributes.The text was updated successfully, but these errors were encountered: