Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

dllimport conflicts with exclude_from_explicit_instantiation. #39988

Open Quuxplusone opened 5 years ago

Quuxplusone commented 5 years ago
Bugzilla Link PR41018
Status CONFIRMED
Importance P normal
Reported by Eric Fiselier (eric@efcs.ca)
Reported on 2019-03-08 17:06:21 -0800
Last modified on 2019-11-26 11:21:48 -0800
Version trunk
Hardware PC All
CC blitzrakete@gmail.com, dgregor@apple.com, erik.pilkington@gmail.com, hans@chromium.org, ldionne@apple.com, llvm-bugs@lists.llvm.org, nicolasweber@gmx.de, richard-llvm@metafoo.co.uk, rnk@google.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

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.

Quuxplusone commented 5 years ago
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-.
Quuxplusone commented 5 years ago

+Louis, since he added the attribute.

Quuxplusone commented 5 years ago
(In reply to Reid Kleckner from comment #2)
> +Louis, since he added the attribute.

(For anyone looking, that was r343790.)

(In reply to Eric Fiselier from comment #0)
> 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.
Quuxplusone commented 5 years ago

ldionne, as author of exclude_from_explicit_instantiation could you take a look? libc++ currently needs to carry a workaround for this.

Quuxplusone commented 5 years ago

libc++ will need another workaround for this for https://bugs.chromium.org/p/chromium/issues/detail?id=923166#c44

Quuxplusone commented 5 years ago

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.

Quuxplusone commented 4 years ago

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.)