rprichard added a comment.
In https://reviews.llvm.org/D38599#899198, @danalbert wrote:
> In https://reviews.llvm.org/D38599#899196, @howard.hinnant wrote:
>
> > Fwiw, I wrote this code. All of that "fallback" stuff was written to make
> > customer code that was incorrect, but working on OS X
howard.hinnant added a comment.
Ok. Well that's why it is under a #define: to make it easier to include or
not, depending on the needs of the platform.
https://reviews.llvm.org/D38599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http:/
danalbert added a comment.
In https://reviews.llvm.org/D38599#899196, @howard.hinnant wrote:
> Fwiw, I wrote this code. All of that "fallback" stuff was written to make
> customer code that was incorrect, but working on OS X
> -version-that-used-libsupc++ continue to work. I.e. to be a crutch
howard.hinnant added a comment.
Fwiw, I wrote this code. All of that "fallback" stuff was written to make
customer code that was incorrect, but working on OS X
-version-that-used-libsupc++ continue to work. I.e. to be a crutch for
incorrect code. It should all be removed if you no longer wan
danalbert abandoned this revision.
danalbert added a comment.
nbjoerg and zygoloid got me pointed in the right direction. Both `Base` and
`BaseImpl` are missing their key functions, and that's the problem here. Patch
should be unnecessary.
https://reviews.llvm.org/D38599
___
jroelofs added a comment.
Needs a docs entry for the new flag (in libcxx's BuildingLibcxx.rst). Other
than that, all the stuff I've asked you to add LGTM. I'd still appreciate
@EricWF/@mclow's opinion on the meat of the functional change part of this
though... I don't know all the implications
danalbert planned changes to this revision.
danalbert added a comment.
In https://reviews.llvm.org/D38599#894041, @jroelofs wrote:
> (possibly renamed to _LIBCXXABI_DYNAMIC_FALLBACK)
I opted for adding this switch to libc++ instead. Like @rprichard points out,
we'll need to do this in `std::ty
danalbert updated this revision to Diff 118711.
danalbert added a comment.
Herald added a subscriber: mgorny.
Update the test with an XFAIL when _LIBCXX_DYNAMIC_FALLBACK is not set.
https://reviews.llvm.org/D38827 adds this cmake option to libc++.
https://reviews.llvm.org/D38599
Files:
CMake
jroelofs added a comment.
In https://reviews.llvm.org/D38599#893990, @jroelofs wrote:
> In https://reviews.llvm.org/D38599#893985, @danalbert wrote:
>
> > In https://reviews.llvm.org/D38599#893903, @jroelofs wrote:
> >
> > > That reminds me... this does need a testcase or two.
> >
> >
> > Oh, als
jroelofs added a comment.
In https://reviews.llvm.org/D38599#893985, @danalbert wrote:
> In https://reviews.llvm.org/D38599#893903, @jroelofs wrote:
>
> > That reminds me... this does need a testcase or two.
>
>
> Oh, also, any test I add is going to fail, since the case I'm trying to
> account
danalbert updated this revision to Diff 118502.
danalbert edited the summary of this revision.
danalbert added a comment.
Added a (failing) test case. The test case will fail unless the default value
of `_LIBCXX_DYNAMIC_FALLBACK` is changed.
https://reviews.llvm.org/D38599
Files:
src/private
danalbert added a comment.
In https://reviews.llvm.org/D38599#893903, @jroelofs wrote:
> That reminds me... this does need a testcase or two.
Oh, also, any test I add is going to fail, since the case I'm trying to account
for here is not the default behavior.
I could make the more invasive ch
danalbert added a comment.
In https://reviews.llvm.org/D38599#893903, @jroelofs wrote:
> That reminds me... this does need a testcase or two.
Didn't realize I could do multi binary test cases with this test runner. It'll
be a little messy, but I'll try adding one.
Repository:
rL LLVM
http
jroelofs added a comment.
That reminds me... this does need a testcase or two.
Repository:
rL LLVM
https://reviews.llvm.org/D38599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rprichard added a comment.
Some relevant code links:
-
https://github.com/llvm-mirror/clang/blob/8c9bf999aa40ab6077b958b5edcf587b9d76ce24/lib/CodeGen/ItaniumCXXABI.cpp#L359
==> iOS64CXXABI overrides shouldRTTIBeUnique to return false.
-
https://github.com/llvm-mirror/libcxx/blob/ca79c159d8bfbe
rprichard added a comment.
Here's the Clang bug I filed: https://bugs.llvm.org/show_bug.cgi?id=34907
Repository:
rL LLVM
https://reviews.llvm.org/D38599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman
rprichard added a comment.
I assume std::type_info::operator== also needs to be adjusted to compare string
names? It looks like libc++'s version of the function does string comparisons
for ARM64 iOS, but only on some classes (e.g. public(?) ones). Look for the
_LIBCPP_HAS_NONUNIQUE_TYPEINFO and
danalbert added a comment.
> Are you 100% sure that you're not just a person with broken code?
Absolutely, since it isn't my code ;) I maintain the toolchain and this is a
behavioral change when switching from libstdc++ to libc++.
> In other words, what did this guy from 2013 get wrong? -- or,
Quuxplusone added a comment.
I'm also much out of my depth here, but I'm skeptical. You're changing the
comments in the code from essentially saying "This workaround helps people with
broken code" to essentially saying "This indispensable functionality helps
people like me who use dlopen()." A
jroelofs resigned from this revision.
jroelofs added a comment.
I'm not sure I'm the right person to review this.
Repository:
rL LLVM
https://reviews.llvm.org/D38599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cg
danalbert added a comment.
In https://reviews.llvm.org/D38599#889842, @smeenai wrote:
> Does dlopen cause issues even with `RTLD_GLOBAL`?
From my testing, yes. Regardless, `RTLD_LOCAL` is how JNI libraries get loaded
when `System.loadLibrary` is used, so the `strcmp` fallback is a requirement
smeenai added a comment.
Does dlopen cause issues even with `RTLD_GLOBAL`?
Repository:
rL LLVM
https://reviews.llvm.org/D38599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
danalbert created this revision.
This doesn't only happen for incorrectly built apps, it also happens
for libraries loaded with dlopen.
Repository:
rL LLVM
https://reviews.llvm.org/D38599
Files:
src/private_typeinfo.cpp
Index: src/private_typeinfo.cpp
23 matches
Mail list logo
