[PATCH] D40660: Enable auto-linking on Windows

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai accepted this revision.
smeenai added a comment.
This revision is now accepted and ready to land.

I think it would make sense for this change to also have the conditional for the static C++ library selection. Basically something like

  #if defined(_LIBCPP_ABI_MICROSOFT) && !defined(_LIBCPP_BUILDING_LIBRARY)
  # if defined(_DLL)
  #  pragma comment(lib, "c++.lib")
  # else
  #  pragma comment(lib, "libc++.lib")
  # endif
  #endif

If you wanna be super consistent with `use_ansi.h`, change the second condition to

  # if defined(_DLL) && !defined(_STATIC_CPPLIB)

LGTM with that.

Also, the diff needs more context :)



================
Comment at: include/__config:1267
+# if defined(_DLL) && !defined(_LIBCPP_BUILDING_LIBRARY)
+#   if defined(_LIBCPP_DEBUG)
+#     pragma comment(lib, "c++d.lib")
----------------
compnerd wrote:
> smeenai wrote:
> > compnerd wrote:
> > > smeenai wrote:
> > > > I guess `_DLL` is appropriate here. Ideally though I think adding the pragma should be keyed on exactly the same conditional that determines whether we annotate with dllexport/dllimport, and right now that's only conditional on `_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS`.
> > > Its more complicated.  This is for the *user* not the library itself.  When building the shared library we need to ensure that it is not added (`!defined(_LIBCPP_BUILDING_LIBRARY)`).  When using the headers as a user, the `_DLL` tells you about the dynamic/static behavior.
> > I know, but the dllimport annotations are also for the *user*. If you're getting the dllimport annotations, you should also be getting the pragma, and vice-versa.
> Right, but `_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS` does not imply use of a static libc++, but `!defined(_DLL)` does, unless Im missing something.  So doesn't this do exactly what you were thinking of?
What I meant was, this is our current logic for determining whether dllimport annotations are used: https://reviews.llvm.org/diffusion/L/browse/libcxx/trunk/include/__config;319549$616-632. In other words, you get the dllimport annotations if `_LIBCPP_OBJECT_FORMAT_COFF && !_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS && !_LIBCPP_BUILDING_LIBRARY`.

I agree that semantically, `_DLL` is the right thing to key the pragma on. However, the condition for the pragma and the condition for dllimport annotations is now inconsistent, so we can end up in situations where we get the pragma but not the dllimport annotations, or vice versa, which seems wrong.

Making the conditionals consistent is hairy, however, because `_DLL` won't be defined for non-MSVC drivers, and the dllimport codepath is required for all of Windows. I'm fine with this as is, and we can think more about that consistency later.

Do you think you also wanna add a pragma for the non-DLL codepath (`libc++.lib`)? Could do that in a follow-up as well, but it seems natural to have that as part of this change, and it would be more consistent with how `use_ansi.h` does it.

Looks like msvcprt also has `_STATIC_CPPLIB` for forcing the use of the static C++ library even when `_DLL`. Idk if we care about supporting that.


Repository:
  rCXX libc++

https://reviews.llvm.org/D40660