[PATCH] D37182: [libcxx] Special visibility macros for the experimental library
mclow.lists added a comment. This is an old patch; is this still needed/desired? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D37182/new/ https://reviews.llvm.org/D37182 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37182: [libcxx] Special visibility macros for the experimental library
smeenai added a comment. Sorry, this has been on my queue for a long time, but I haven't gotten the chance to get to it yet. I'll try to take a look (at this and your other patches) in the next few days. https://reviews.llvm.org/D37182 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37182: [libcxx] Special visibility macros for the experimental library
hamzasood added a comment. I have access now, so I'm able to commit this myself. However it's been a while since it was approved, so I'd be grateful if someone could take another look to make sure nothing has changed in the meantime (besides potentially needing to re-tag some new APIs). https://reviews.llvm.org/D37182 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37182: [libcxx] Special visibility macros for the experimental library
hamzasood added a comment. Thanks. Could someone commit this for me please? Comment at: include/experimental/__config:57 +#if defined(_LIBCPP_OBJECT_FORMAT_COFF) +# define _LIBCPPX_TYPE_VIS compnerd wrote: > hamzasood wrote: > > smeenai wrote: > > > I think it would make more sense to make these macros empty on all > > > platforms, not just Windows. It's true that they'll only cause link > > > errors on Windows (since you'll attempt to dllimport functions from a > > > static library), but on ELF and Mach-O, the visibility annotations would > > > cause these functions to be exported from whatever library > > > c++experimental gets linked into, which is probably not desirable either. > > > > > > The exception (if you'll pardon the pun) is `_LIBCPPX_EXCEPTION_ABI`, > > > which will still need to expand to at least > > > `__type_visibility__((default))` for non-COFF in order for throwing and > > > catching those types to work correctly. > > I might be mistaken, but I think the regular libc++ library still uses > > visibility annotations when it's being built as a static library on > > non-Windows platforms. So I figured it'd be best to match that behaviour. > It's not about matching that behaviour, libc++ is built shared, this is built > static. The static link marked default visibility would make the functions > be re-exported. However, if I'm not mistaken, this already happens today, so > fixing that in a follow-up is fine as the status quo is maintained. But isn't it possible to build libc++ as static instead of shared? (LIBCXX_ENABLE_STATIC=YES and LIBCXX_ENABLE_SHARED=NO). I'm pretty sure that in that case, the visibility annotations are left active. https://reviews.llvm.org/D37182 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37182: [libcxx] Special visibility macros for the experimental library
compnerd accepted this revision. compnerd added inline comments. This revision is now accepted and ready to land. Comment at: include/experimental/__config:57 +#if defined(_LIBCPP_OBJECT_FORMAT_COFF) +# define _LIBCPPX_TYPE_VIS hamzasood wrote: > smeenai wrote: > > I think it would make more sense to make these macros empty on all > > platforms, not just Windows. It's true that they'll only cause link errors > > on Windows (since you'll attempt to dllimport functions from a static > > library), but on ELF and Mach-O, the visibility annotations would cause > > these functions to be exported from whatever library c++experimental gets > > linked into, which is probably not desirable either. > > > > The exception (if you'll pardon the pun) is `_LIBCPPX_EXCEPTION_ABI`, which > > will still need to expand to at least `__type_visibility__((default))` for > > non-COFF in order for throwing and catching those types to work correctly. > I might be mistaken, but I think the regular libc++ library still uses > visibility annotations when it's being built as a static library on > non-Windows platforms. So I figured it'd be best to match that behaviour. It's not about matching that behaviour, libc++ is built shared, this is built static. The static link marked default visibility would make the functions be re-exported. However, if I'm not mistaken, this already happens today, so fixing that in a follow-up is fine as the status quo is maintained. https://reviews.llvm.org/D37182 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37182: [libcxx] Special visibility macros for the experimental library
compnerd added a comment. I think that splitting this up in a series of patches would be much better. The first patch should be to do the entirely mechanical change of the visibility attribute. It is a separate library and needs its own visibility attribute. That would significantly slim down this subsequent change. A follow-up change could fix the build, and a third change adds the test harness/support the tests. https://reviews.llvm.org/D37182 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37182: [libcxx] Special visibility macros for the experimental library
mclow.lists added a comment. The reason for building the filesystem library as a statically linked lib (instead of dynamic) is that for quite a while it was changing significantly. Having people link statically means that we can make changes w/o worrying (as much) about people using the library - they can pick up new features /changes when they re-link their programs. Once we change to a dylib (and we will, but not yet), then we have to worry about ABI changes affecting programs in the field. https://reviews.llvm.org/D37182 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37182: [libcxx] Special visibility macros for the experimental library
hamzasood added a comment. In https://reviews.llvm.org/D37182#855262, @smeenai wrote: > The other question, of course, is why the experimental library needs to be > static. If it were built shared, the annotations would just work on Windows > in theory (though I'm sure there are other issues there). Even if libc++experimental is no longer forced to be static, it'd probably be too restrictive to force libc++ and libc++experimental to be the same library type. E.g. someone might want a dynamic libc++ but a static libc++experimental. If that's possible (even if not by default), different visibility macros will be needed. Comment at: include/experimental/__config:57 +#if defined(_LIBCPP_OBJECT_FORMAT_COFF) +# define _LIBCPPX_TYPE_VIS smeenai wrote: > I think it would make more sense to make these macros empty on all platforms, > not just Windows. It's true that they'll only cause link errors on Windows > (since you'll attempt to dllimport functions from a static library), but on > ELF and Mach-O, the visibility annotations would cause these functions to be > exported from whatever library c++experimental gets linked into, which is > probably not desirable either. > > The exception (if you'll pardon the pun) is `_LIBCPPX_EXCEPTION_ABI`, which > will still need to expand to at least `__type_visibility__((default))` for > non-COFF in order for throwing and catching those types to work correctly. I might be mistaken, but I think the regular libc++ library still uses visibility annotations when it's being built as a static library on non-Windows platforms. So I figured it'd be best to match that behaviour. https://reviews.llvm.org/D37182 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37182: [libcxx] Special visibility macros for the experimental library
smeenai added reviewers: mclow.lists, compnerd. smeenai resigned from this revision. smeenai added a comment. It's really cool that you're getting the filesystem library to work on Windows :) This looks reasonable to me; it's the only way I can think of to get the experimental library working on Windows if we're keeping it static. It's a large change, but most of it is mechanical. Given its size, however, I also want to let at least one of Eric and Marshall take a look. The other question, of course, is why the experimental library needs to be static. If it were built shared, the annotations would just work on Windows in theory (though I'm sure there are other issues there). Comment at: include/experimental/__config:57 +#if defined(_LIBCPP_OBJECT_FORMAT_COFF) +# define _LIBCPPX_TYPE_VIS I think it would make more sense to make these macros empty on all platforms, not just Windows. It's true that they'll only cause link errors on Windows (since you'll attempt to dllimport functions from a static library), but on ELF and Mach-O, the visibility annotations would cause these functions to be exported from whatever library c++experimental gets linked into, which is probably not desirable either. The exception (if you'll pardon the pun) is `_LIBCPPX_EXCEPTION_ABI`, which will still need to expand to at least `__type_visibility__((default))` for non-COFF in order for throwing and catching those types to work correctly. https://reviews.llvm.org/D37182 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
