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
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
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
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
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:
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,
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:
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
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
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.
-
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
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
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()."
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
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
