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 {
// Too many methods to individually annotate with EXPORT:
void baz00();
void baz01();
...
void baz99();

// One inline method that I don't want to export, for whatever reason:
int attribute((exclude_from_explicit_instantiation)) bar() {
return 42;
}
};

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.

+Louis, since he added the attribute.

(For anyone looking, that was r343790.)

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.

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

@llvm/issue-subscribers-clang-codegen

I had to revert my patch due to #66909

Not sure why this was marked completed again. The fix was reverted in 700f683

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:

I took a further look at this and I understand where your concern is coming from. It might seem unusual that a commit in one person’s repository, in this case, skatrak/llvm-project-rocm, can close an issue in a different repository like llvm/llvm-project.
However, this is actually a feature of GitHub and is part of the expected behavior. When a user, like skatrak, who has write permissions in both repositories, links a pull request to an issue in a different repository and merges the commit into the default branch, the linked issue will close. This is because the commit message contains specific keywords that trigger this action. Here’s the GitHub documentation explaining this feature: Linking a pull request to an issue.
I get that this could be worrisome, considering a scenario where someone with less than good intentions pushes multiple commits to their own repo with “Fixes” keywords in the messages. It's important to note, though, that this action can only happen if the user has the necessary permissions in the repository where the issue exists. So, it is crucial to manage permissions carefully.

GitHub support replied regarding the unwanted closing:

I took a further look at this and I understand where your concern is coming from. It might seem unusual that a commit in one person’s repository, in this case, skatrak/llvm-project-rocm, can close an issue in a different repository like llvm/llvm-project.
However, this is actually a feature of GitHub and is part of the expected behavior. When a user, like skatrak, who has write permissions in both repositories, links a pull request to an issue in a different repository and merges the commit into the default branch, the linked issue will close. This is because the commit message contains specific keywords that trigger this action. Here’s the GitHub documentation explaining this feature: Linking a pull request to an issue.
I get that this could be worrisome, considering a scenario where someone with less than good intentions pushes multiple commits to their own repo with “Fixes” keywords in the messages. It's important to note, though, that this action can only happen if the user has the necessary permissions in the repository where the issue exists. So, it is crucial to manage permissions carefully.

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.

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.