[libc++abi] Handle catch null pointer-to-object #68076
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@llvm/pr-subscribers-libcxxabi ChangesThis addresses cases (currently failing) where we throw a null pointer-to- object and is a proposed fix for We are trying to satisfy the following bullet from the C++ ABI 15.3:
The existing implementation assesses the ambiguity of bases by computing the offsets to them; ambiguous cases are then when the same base appears at different offsets. The computation of offset includes indirecting through the vtables to find the offsets to virtual bases. When the thrown pointer points to a real object, this is quite efficient since, if the base is found, and it is not ambiguous and on a public path, the offset is needed to return the adjusted pointer (and the indirections are not particularly expensive to compute). However, when we throw a null pointer-to-object, this scheme is no longer applicable (and the code currently bypasses the relevant computations, leading to the incorrect catches reported in the issue). The solution proposed here takes a composite approach:
A base is then ambiguous iff:
When a handler for a pointer succeeds in catching a base pointer for a thrown null pointer-to-object, we still return a nullptr (so the adjustment to the pointer is not required and need not be computed). Since we noted that there was no object when starting the search for ambiguous bases, we know that we can skip the pointer adjustment. This was : Differential Revision: https://reviews.llvm.org/D158769 Full diff: https://github.com/llvm/llvm-project/pull/68076.diff 3 Files Affected:
diff --git a/libcxxabi/src/private_typeinfo.cpp b/libcxxabi/src/private_typeinfo.cpp
index 82db4bbec1ada2e..197ea2ab8bccc25 100644
--- a/libcxxabi/src/private_typeinfo.cpp
+++ b/libcxxabi/src/private_typeinfo.cpp
@@ -42,6 +42,7 @@
// is_equal() with use_strcmp=false so the string names are not compared.
#include <cstdint>
+#include <cassert>
#include <string.h>
#ifdef _LIBCXXABI_FORGIVING_DYNAMIC_CAST
@@ -167,7 +168,7 @@ const void* dyn_cast_to_derived(const void* static_ptr,
src2dst_offset,
0, 0, 0, 0, 0, 0, 0, 0,
1, // number_of_dst_type
- false, false, false
+ false, false, false, true
};
// Do the search
dst_type->search_above_dst(&info, dynamic_ptr, dynamic_ptr, public_path, false);
@@ -192,7 +193,7 @@ const void* dyn_cast_to_derived(const void* static_ptr,
static_ptr,
static_type,
src2dst_offset,
- 0, 0, 0, 0, 0, 0, 0, 0, 0, false, false, false
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, false, false, false, true
};
info.number_of_dst_type = 1;
dst_type->search_above_dst(&info, dynamic_ptr, dynamic_ptr, public_path, true);
@@ -239,7 +240,7 @@ const void* dyn_cast_try_downcast(const void* static_ptr,
src2dst_offset,
0, 0, 0, 0, 0, 0, 0, 0,
1, // number_of_dst_type
- false, false, false
+ false, false, false, true
};
dynamic_type->search_above_dst(&dynamic_to_dst_info, dynamic_ptr, dynamic_ptr, public_path, false);
if (dynamic_to_dst_info.path_dst_ptr_to_static_ptr != unknown) {
@@ -266,7 +267,7 @@ const void* dyn_cast_slow(const void* static_ptr,
static_ptr,
static_type,
src2dst_offset,
- 0, 0, 0, 0, 0, 0, 0, 0, 0, false, false, false
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, false, false, false, true
};
dynamic_type->search_below_dst(&info, dynamic_ptr, public_path, false);
@@ -292,7 +293,7 @@ const void* dyn_cast_slow(const void* static_ptr,
static_ptr,
static_type,
src2dst_offset,
- 0, 0, 0, 0, 0, 0, 0, 0, 0, false, false, false
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, false, false, false, true
};
dynamic_type->search_below_dst(&info, dynamic_ptr, public_path, true);
}
@@ -481,9 +482,11 @@ __class_type_info::can_catch(const __shim_type_info* thrown_type,
if (thrown_class_type == 0)
return false;
// bullet 2
- __dynamic_cast_info info = {thrown_class_type, 0, this, -1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,};
+ assert (adjustedPtr && "catching a class without an object?");
+ __dynamic_cast_info info = {thrown_class_type, 0, this,
+ -1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, true};
info.number_of_dst_type = 1;
- thrown_class_type->has_unambiguous_public_base(&info, adjustedPtr, public_path);
+ thrown_class_type->has_unambiguous_public_base(&info, adjustedPtr, nullptr, public_path);
if (info.path_dst_ptr_to_static_ptr == public_path)
{
adjustedPtr = const_cast<void*>(info.dst_ptr_leading_to_static_ptr);
@@ -496,19 +499,39 @@ __class_type_info::can_catch(const __shim_type_info* thrown_type,
#pragma clang diagnostic pop
#endif
+// When we have an object to inspect - we just pass the pointer to the sub-
+// object that matched the static_type we just checked. If that is different
+// from any previously recorded pointer to that object type, then we have
+// an ambiguous case.
+
+// When we have no object to inspect, we need to account for virtual bases
+// explicitly.
+// virtualBase is the pointer to the name of the innermost virtual base
+// type, or nullptr if there is no virtual base on the path so far.
+// adjustedPtr points to the subobject we just found.
+// If virtualBase != any previously recorded (including the case of nullptr
+// representing an already-found static sub-object) then we have an ambiguous
+// case. Assuming that the virtualBase values agree; if then we have a
+// different offset (adjustedPtr) from any previously recorded, this indicates
+// an ambiguous case within the virtual base.
+
void
__class_type_info::process_found_base_class(__dynamic_cast_info* info,
void* adjustedPtr,
+ const void* virtualBase,
int path_below) const
{
- if (info->dst_ptr_leading_to_static_ptr == 0)
+ if (info->number_to_static_ptr == 0)
{
// First time here
info->dst_ptr_leading_to_static_ptr = adjustedPtr;
info->path_dst_ptr_to_static_ptr = path_below;
+ // re-purpose this pointer.
+ info->dst_ptr_not_leading_to_static_ptr = virtualBase;
info->number_to_static_ptr = 1;
}
- else if (info->dst_ptr_leading_to_static_ptr == adjustedPtr)
+ else if (info->dst_ptr_not_leading_to_static_ptr == virtualBase &&
+ info->dst_ptr_leading_to_static_ptr == adjustedPtr)
{
// We've been here before. Update path to "most public"
if (info->path_dst_ptr_to_static_ptr == not_public_path)
@@ -527,62 +550,83 @@ __class_type_info::process_found_base_class(__dynamic_cast_info* info,
void
__class_type_info::has_unambiguous_public_base(__dynamic_cast_info* info,
void* adjustedPtr,
+ const void* virtualBase,
int path_below) const
{
if (is_equal(this, info->static_type, false))
- process_found_base_class(info, adjustedPtr, path_below);
+ process_found_base_class(info, adjustedPtr, virtualBase, path_below);
}
void
__si_class_type_info::has_unambiguous_public_base(__dynamic_cast_info* info,
void* adjustedPtr,
+ const void* virtualBase,
int path_below) const
{
if (is_equal(this, info->static_type, false))
- process_found_base_class(info, adjustedPtr, path_below);
+ process_found_base_class(info, adjustedPtr, virtualBase, path_below);
else
- __base_type->has_unambiguous_public_base(info, adjustedPtr, path_below);
+ __base_type->has_unambiguous_public_base(info, adjustedPtr, virtualBase, path_below);
}
void
__base_class_type_info::has_unambiguous_public_base(__dynamic_cast_info* info,
void* adjustedPtr,
+ const void* virtualBase,
int path_below) const
{
+ bool is_virtual = __offset_flags & __virtual_mask;
ptrdiff_t offset_to_base = 0;
- if (adjustedPtr != nullptr)
+ if (info->have_object)
{
+ /* We have an object to inspect, we can look through its vtables to
+ find the layout. */
offset_to_base = __offset_flags >> __offset_shift;
- if (__offset_flags & __virtual_mask)
+ if (is_virtual)
{
const char* vtable = *static_cast<const char*const*>(adjustedPtr);
offset_to_base = update_offset_to_base(vtable, offset_to_base);
}
+ } else if (! is_virtual) {
+ /* We have no object - so we cannot use it for determining layout when
+ we have a virtual base (since we cannot indirect through the vtable
+ to find the actual object offset). However, for non-virtual bases,
+ we can pretend to have an object based at '0' */
+ offset_to_base = __offset_flags >> __offset_shift;
+ } else {
+ // no object to inspect, and the next base is virtual.
+ // we want to update virtualBase to the new innermost virtual base.
+ // using the pointer to the typeinfo name as a key.
+ virtualBase = static_cast<const void*>(__base_type->name ());
+ // .. and reset the pointer.
+ adjustedPtr = nullptr;
}
__base_type->has_unambiguous_public_base(
info,
static_cast<char*>(adjustedPtr) + offset_to_base,
+ virtualBase,
(__offset_flags & __public_mask) ? path_below : not_public_path);
}
void
__vmi_class_type_info::has_unambiguous_public_base(__dynamic_cast_info* info,
void* adjustedPtr,
+ const void* virtualBase,
int path_below) const
{
if (is_equal(this, info->static_type, false))
- process_found_base_class(info, adjustedPtr, path_below);
+ process_found_base_class(info, adjustedPtr, virtualBase, path_below);
else
{
typedef const __base_class_type_info* Iter;
const Iter e = __base_info + __base_count;
Iter p = __base_info;
- p->has_unambiguous_public_base(info, adjustedPtr, path_below);
+ p->has_unambiguous_public_base(info, adjustedPtr, virtualBase, path_below);
if (++p < e)
{
do
{
- p->has_unambiguous_public_base(info, adjustedPtr, path_below);
+ p->has_unambiguous_public_base(info, adjustedPtr, virtualBase, path_below);
if (info->search_done)
break;
} while (++p < e);
@@ -679,13 +723,22 @@ __pointer_type_info::can_catch(const __shim_type_info* thrown_type,
dynamic_cast<const __class_type_info*>(thrown_pointer_type->__pointee);
if (thrown_class_type == 0)
return false;
- __dynamic_cast_info info = {thrown_class_type, 0, catch_class_type, -1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,};
+ bool have_object = adjustedPtr != nullptr;
+ __dynamic_cast_info info = {thrown_class_type, 0, catch_class_type, -1,
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+ have_object };
info.number_of_dst_type = 1;
- thrown_class_type->has_unambiguous_public_base(&info, adjustedPtr, public_path);
+ thrown_class_type->has_unambiguous_public_base(&info, adjustedPtr, nullptr, public_path);
if (info.path_dst_ptr_to_static_ptr == public_path)
{
- if (adjustedPtr != NULL)
+ // In the case of a thrown null pointer, we have no object but we might
+ // well have computed the offset to where a public sub-object would be.
+ // However, we do not want to return that offset to the user; we still
+ // want them to catch a null ptr.
+ if (have_object)
adjustedPtr = const_cast<void*>(info.dst_ptr_leading_to_static_ptr);
+ else
+ adjustedPtr = nullptr;
return true;
}
return false;
diff --git a/libcxxabi/src/private_typeinfo.h b/libcxxabi/src/private_typeinfo.h
index 622e09cc24217f3..68b02cbc885b100 100644
--- a/libcxxabi/src/private_typeinfo.h
+++ b/libcxxabi/src/private_typeinfo.h
@@ -110,6 +110,11 @@ struct _LIBCXXABI_HIDDEN __dynamic_cast_info
bool found_any_static_type;
// Set whenever a search can be stopped
bool search_done;
+
+ // Data that modifies the search mechanism.
+
+ // There is no object (seen when we throw a null pointer to object).
+ bool have_object;
};
// Has no base class
@@ -122,8 +127,7 @@ class _LIBCXXABI_TYPE_VIS __class_type_info : public __shim_type_info {
const void *, int) const;
_LIBCXXABI_HIDDEN void process_static_type_below_dst(__dynamic_cast_info *,
const void *, int) const;
- _LIBCXXABI_HIDDEN void process_found_base_class(__dynamic_cast_info *, void *,
- int) const;
+ _LIBCXXABI_HIDDEN void process_found_base_class(__dynamic_cast_info*, void*, const void*, int) const;
_LIBCXXABI_HIDDEN virtual void search_above_dst(__dynamic_cast_info *,
const void *, const void *,
int, bool) const;
@@ -131,8 +135,7 @@ class _LIBCXXABI_TYPE_VIS __class_type_info : public __shim_type_info {
search_below_dst(__dynamic_cast_info *, const void *, int, bool) const;
_LIBCXXABI_HIDDEN virtual bool can_catch(const __shim_type_info *,
void *&) const;
- _LIBCXXABI_HIDDEN virtual void
- has_unambiguous_public_base(__dynamic_cast_info *, void *, int) const;
+ _LIBCXXABI_HIDDEN virtual void has_unambiguous_public_base(__dynamic_cast_info*, void*, const void*, int) const;
};
// Has one non-virtual public base class at offset zero
@@ -147,8 +150,7 @@ class _LIBCXXABI_TYPE_VIS __si_class_type_info : public __class_type_info {
int, bool) const;
_LIBCXXABI_HIDDEN virtual void
search_below_dst(__dynamic_cast_info *, const void *, int, bool) const;
- _LIBCXXABI_HIDDEN virtual void
- has_unambiguous_public_base(__dynamic_cast_info *, void *, int) const;
+ _LIBCXXABI_HIDDEN virtual void has_unambiguous_public_base(__dynamic_cast_info*, void*, const void*, int) const;
};
struct _LIBCXXABI_HIDDEN __base_class_type_info
@@ -166,7 +168,7 @@ struct _LIBCXXABI_HIDDEN __base_class_type_info
void search_above_dst(__dynamic_cast_info*, const void*, const void*, int, bool) const;
void search_below_dst(__dynamic_cast_info*, const void*, int, bool) const;
- void has_unambiguous_public_base(__dynamic_cast_info*, void*, int) const;
+ void has_unambiguous_public_base(__dynamic_cast_info*, void*, const void*, int) const;
};
// Has one or more base classes
@@ -190,8 +192,7 @@ class _LIBCXXABI_TYPE_VIS __vmi_class_type_info : public __class_type_info {
int, bool) const;
_LIBCXXABI_HIDDEN virtual void
search_below_dst(__dynamic_cast_info *, const void *, int, bool) const;
- _LIBCXXABI_HIDDEN virtual void
- has_unambiguous_public_base(__dynamic_cast_info *, void *, int) const;
+ _LIBCXXABI_HIDDEN virtual void has_unambiguous_public_base(__dynamic_cast_info*, void*, const void*, int) const;
};
class _LIBCXXABI_TYPE_VIS __pbase_type_info : public __shim_type_info {
diff --git a/libcxxabi/test/catch_null_pointer_to_object_pr64953.pass.cpp b/libcxxabi/test/catch_null_pointer_to_object_pr64953.pass.cpp
new file mode 100644
index 000000000000000..b06f15b697e3163
--- /dev/null
+++ b/libcxxabi/test/catch_null_pointer_to_object_pr64953.pass.cpp
@@ -0,0 +1,182 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This test case checks specifically the cases under bullet 3.3:
+//
+// C++ ABI 15.3:
+// A handler is a match for an exception object of type E if
+// * The handler is of type cv T or cv T& and E and T are the same type
+// (ignoring the top-level cv-qualifiers), or
+// * the handler is of type cv T or cv T& and T is an unambiguous base
+// class of E, or
+// > * the handler is of type cv1 T* cv2 and E is a pointer type that can <
+// > be converted to the type of the handler by either or both of <
+// > o a standard pointer conversion (4.10 [conv.ptr]) not involving <
+// > conversions to private or protected or ambiguous classes <
+// > o a qualification conversion <
+// * the handler is a pointer or pointer to member type and E is
+// std::nullptr_t
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: no-exceptions
+
+#include <exception>
+#include <stdlib.h>
+#include <assert.h>
+#include <stdio.h>
+
+struct Base {
+ int b;
+};
+struct Base2 {
+ int b;
+};
+struct Derived1 : Base {
+ int b;
+};
+struct Derived2 : Base {
+ int b;
+};
+struct Derived3 : Base2 {
+ int b;
+};
+struct Private : private Base {
+ int b;
+};
+struct Protected : protected Base {
+ int b;
+};
+struct Virtual1 : virtual Base {
+ int b;
+};
+struct Virtual2 : virtual Base {
+ int b;
+};
+
+struct Ambiguous1 : Derived1, Derived2 {
+ int b;
+};
+struct Ambiguous2 : Derived1, Private {
+ int b;
+};
+struct Ambiguous3 : Derived1, Protected {
+ int b;
+};
+
+struct NoPublic1 : Private, Base2 {
+ int b;
+};
+struct NoPublic2 : Protected, Base2 {
+ int b;
+};
+
+struct Catchable1 : Derived3, Derived1 {
+ int b;
+};
+struct Catchable2 : Virtual1, Virtual2 {
+ int b;
+};
+struct Catchable3 : virtual Base, Virtual2 {
+ int b;
+};
+
+// Check that, when we have a null pointer-to-object that we catch a nullptr.
+template <typename T // Handler type
+ ,
+ typename E // Thrown exception type
+ >
+void assert_catches() {
+ try {
+ throw static_cast<E>(0);
+ printf("%s\n", __PRETTY_FUNCTION__);
+ assert(false && "Statements after throw must be unreachable");
+ } catch (T t) {
+ assert(t == nullptr);
+ return;
+ } catch (...) {
+ printf("%s\n", __PRETTY_FUNCTION__);
+ assert(false && "Should not have entered catch-all");
+ }
+
+ printf("%s\n", __PRETTY_FUNCTION__);
+ assert(false && "The catch should have returned");
+}
+
+template <typename T // Handler type
+ ,
+ typename E // Thrown exception type
+ >
+void assert_cannot_catch() {
+ try {
+ throw static_cast<E>(0);
+ printf("%s\n", __PRETTY_FUNCTION__);
+ assert(false && "Statements after throw must be unreachable");
+ } catch (T t) {
+ printf("%s\n", __PRETTY_FUNCTION__);
+ assert(false && "Should not have entered the catch");
+ } catch (...) {
+ assert(true);
+ return;
+ }
+
+ printf("%s\n", __PRETTY_FUNCTION__);
+ assert(false && "The catch-all should have returned");
+}
+
+// Check that when we have a pointer-to-actual-object we, in fact, get the
+// adjusted pointer to the base class.
+template <typename T // Handler type
+ ,
+ typename O // Object type
+ >
+void assert_catches_bp() {
+ O* o = new (O);
+ try {
+ throw o;
+ printf("%s\n", __PRETTY_FUNCTION__);
+ assert(false && "Statements after throw must be unreachable");
+ } catch (T t) {
+ assert(t == static_cast<T>(o));
+ //__builtin_printf("o = %p t = %p\n", o, t);
+ return;
+ } catch (...) {
+ printf("%s\n", __PRETTY_FUNCTION__);
+ assert(false && "Should not have entered catch-all");
+ }
+
+ printf("%s\n", __PRETTY_FUNCTION__);
+ assert(false && "The catch should have returned");
+}
+
+void f1() {
+ assert_catches<Base*, Catchable1*>();
+ assert_catches<Base*, Catchable2*>();
+ assert_catches<Base*, Catchable3*>();
+}
+
+void f2() {
+ assert_cannot_catch<Base*, Ambiguous1*>();
+ assert_cannot_catch<Base*, Ambiguous2*>();
+ assert_cannot_catch<Base*, Ambiguous3*>();
+ assert_cannot_catch<Base*, NoPublic1*>();
+ assert_cannot_catch<Base*, NoPublic2*>();
+}
+
+void f3() {
+ assert_catches_bp<Base*, Catchable1>();
+ assert_catches_bp<Base*, Catchable2>();
+ assert_catches_bp<Base*, Catchable3>();
+}
+
+int main(int, char**) {
+ f1();
+ f2();
+ f3();
+ return 0;
+}
|
iains
force-pushed
the
llvm-issue-64953
branch
from
459ea8d
to
4c6af9a
Compare
|
ran git clang-format with clang-format from llvm-17. |
There was a problem hiding this comment.
I'm... not sure it's okay to change v-tables in the std::type_info hierarchy like this. In theory it's non-ABI and purely an internal implementation detail of the C++ runtime, but in practice there are some other subclasses out there, like in the ObjC runtime.
I know that the bot is telling you to reformat, but that is making it a lot more difficult to review this patch; please just ignore the bot or do the reformat in a separate PR.
Let me try to understand the basic problem here. We have an exception that's dynamically a null pointer statically typed as Base*, and we're testing whether it's caught by Derived*. This should succeed only if there is a unique public Base subobject in Derived. Catching is built on top of the machinery for dynamic_cast, but the exception path can't be handled the same way it is for dynamic_cast for two reasons:
- First, the
dynamic_castoperator on a null pointer just produces a null pointer unconditionally; it is not meaningful to ask whether the cast "succeeded". That is not true with EH, because we have to decide whether we enter thecatchblock (or pass the EH filter). - Second, in
dynamic_castwe know both types statically and could therefore evaluate the unique-public-subclass condition statically if we needed to. This is not possible with EH because the type relationship is dynamic.
So EH needs a way to check the has-unique-public-subclass condition without looking at actual objects. Makes sense.
My, perhaps flawed, understanding is that Objective-C is supposed to inherit from the parent language and thus, absent a specification to the contrary, a bug in the exception handling here would also be a bug in the Objective-C exception handling, thus it should also import the fix)?
(agreed that the layout change obfuscates some of the change purpose - most of it is just initialising the new bool in __dynamic_cast_info). However, if you like I can find my original patch (without the layout changes) and force-push that here?
Right, exactly.
|
It's not that Objective-C doesn't want the fix, it's that Apple's Objective-C runtime (at least) has its own subclass of
I think that would be better, yeah.
Let me tag in @mikeash for this. |
iains
force-pushed
the
llvm-issue-64953
branch
from
21515eb
to
2ac5359
Compare
Right, that was what I expected; It would be good for me to understand the constraints better (in case it opens up other opportunities for fixes). e.g. It seems to me that pushing all state into the
Done, and rebased/retested (so we expect the formatting check to fail for now). |
|
libobjc has this wacky hand-written vtable: https://github.com/apple-oss-distributions/objc4/blob/objc4-906/runtime/objc-exception.mm#L183 It's mostly no-ops and relies on the fact that the calling conventions allow passing more parameters than the function actually consumes. There's one entry that actually does anything, which is ARM64e ptrauth adds a wrinkle. Changing the signature of these methods will change the ptrauth discriminator, which will no longer match libobjc's hardcoded discriminators. That's not a big deal for That's the only virtual method that changed, so I think we're all good here. libobjc might want to update its discriminator at some point, but that's just a nice-to-have, and it doesn't have to be coordinated with this change. @rjmccall Anything I might have missed here? |
|
apropos the build kite fails:
|
Ah, I was going to ask about that. Adding the missing
The back-deployment jobs are testing the case where you build an application (here your test) against a recent SDK (i.e. a recent version of libc++), but you specify that you will be deploying to an older version of macOS. And then you run your freshly-built application against an older dylib. So it's building against a recent libc++, but running against an old one. This is meant to capture the use case of someone building an application on a recent system, but the application is meant to work on various older devices. In practice, we sometimes find subtle ABI breaks with these jobs. In this case, since you are fixing an issue in Then we will evolve the test, and when we eventually drop support for these older macOS versions, we will remove the annotation (it will take a while). If you want to reproduce this issue, you need to be on macOS (which seems to be your case). Then you should be able to just run |
iains
force-pushed
the
llvm-issue-64953
branch
from
2ac5359
to
95c21fc
Compare
|
OK:
Thanks for the pointers. I was unable to reproduce the asan fail on either x86_64 macOS 14 or arm64 macOS 12.7 - so I went ahead and made a conservative change - we will have to see if the problem clears on the bot. The other cases are fixed (only testable on x86_64 AFAICT), I see some ld64 warnings about mismatched build versions on arm64 macOS 12,7 - and when the library is missing - we then get caught be the changes that mean there's no lib in (we still expect the formatting complaints) |
iains
force-pushed
the
llvm-issue-64953
branch
from
95c21fc
to
5db84da
Compare
|
update:
|
|
@iains Can you rebase onto |
iains
force-pushed
the
llvm-issue-64953
branch
from
5db84da
to
ff086f5
Compare
|
sure; done |
|
@ldionne well, it seems we still have one issue with the CI. The Perhaps that test should really be declaring |
|
Interesting. This actually highlights a blind spot we have in the expressiveness of our Lit features. We don't have a way to express "libc++ configured as Apple system library, targeting , but using the tip-of-trunk version of the library". In other words, we generally assume that To avoid blocking you on this (which really has nothing to do with this patch), you could just use |
in this case it was just the plain
I suppose libc++abi moves slowly, not sure why it does not bite more with libc++ tho - if the assumption of the lit config is that it's the installed libs being tested, but actually we're testing the new ones.
OK - I guess that means that the test will not run in any CI - but should still run when people test the library locally? |
We're compiling and linking against the new library, but we are running against the installed system one.
The test will only be skipped when we build for the Apple-system configuration, which is not most configurations. If you just take libc++ and build it out of the box on Apple platforms, you get the LLVM configuration, not the Apple System configuration (so the test would run). |
For the but for the and the test execution line is (long paths trimmed): So, in this case, the test is executed with new libraries. edit: and, in fact, I do not think it will work with the installed libs because there are no I'll do the |
iains
force-pushed
the
llvm-issue-64953
branch
from
ff086f5
to
f7f3e51
Compare
|
rebase, added the UNSUPPORTED, slight amendment to the title (removed the Issue # reference). Cannot figure out why the CI now thinks there are formatting errors: edit: let's try this again.... |
This addresses cases (currently failing) where we throw a null pointer-to- object and is a proposed fix for llvm#64953 We are trying to satisfy the following bullet from the C++ ABI 15.3: * the handler is of type cv1 T* cv2 and E is a pointer type that can be converted to the type of the handler by either or both of: o a standard pointer conversion (4.10 [conv.ptr]) not involving conversions to private or protected or ambiguous classes. o a qualification conversion. The existing implementation assesses the ambiguity of bases by computing the offsets to them; ambiguous cases are then when the same base appears at different offsets. The computation of offset includes indirecting through the vtables to find the offsets to virtual bases. When the thrown pointer points to a real object, this is quite efficient since, if the base is found, and it is not ambiguous and on a public path, the offset is needed to return the adjusted pointer (and the indirections are not particularly expensive to compute). However, when we throw a null pointer-to-object, this scheme is no longer applicable (and the code currently bypasses the relevant computations, leading to the incorrect catches reported in the issue). ----- The solution proposed here takes a composite approach: 1. When the pointer-to-object points to a real instance (well, at least, it is determined to be non-null), we use the existing scheme. 2. When the pointer-to-object is null: * We note that there is no real object. * When we are processing non-virtual bases, we continue to compute the offsets, but for a notional dummy object based at 0. This is OK, since we never need to access the object content for non-virtual bases. * When we are processing a path with one or more virtual bases, we remember a cookie corresponding to the inner-most virtual base found so far (and set the notional offset to 0). Offsets to inner non-virtual bases are then computed as normal. A base is then ambiguous iff: * There is a recorded virtual base cookie and that is different from the current one or, * The non-virtual base offsets differ. When a handler for a pointer succeeds in catching a base pointer for a thrown null pointer-to-object, we still return a nullptr (so the adjustment to the pointer is not required and need not be computed). Since we noted that there was no object when starting the search for ambiguous bases, we know that we can skip the pointer adjustment. This was : Differential Revision: https://reviews.llvm.org/D158769
iains
force-pushed
the
llvm-issue-64953
branch
from
f7f3e51
to
a730892
Compare
iains
changed the title
|
It seems like the CI passed on this a while back. Are we waiting on anything to merge this? |
an ack from the ABI owners, I think :) |
|
If the function signatures aren't changing, this definitely ought to be okay. |
|
I'll rebase / squash and retest - is the process then to use the "squash and merge" or is it better to commit the "old way"? |
|
Actually, we can just squash and merge right away here. If there are no merge conflicts, I don't think this needs to go through CI again. Pressing "squash and merge" and then editing the resulting message appropriately (if it needs to be edited) is the right way to proceed nowadays. |
|
It looks like this applies cleanly onto |
This addresses cases (currently failing) where we throw a null pointer-to-object and fixes llvm#64953. We are trying to satisfy the following bullet from the C++ ABI 15.3: * the handler is of type cv1 T* cv2 and E is a pointer type that can be converted to the type of the handler by either or both of: - a standard pointer conversion (4.10 [conv.ptr]) not involving conversions to private or protected or ambiguous classes. - a qualification conversion. The existing implementation assesses the ambiguity of bases by computing the offsets to them; ambiguous cases are then when the same base appears at different offsets. The computation of offset includes indirecting through the vtables to find the offsets to virtual bases. When the thrown pointer points to a real object, this is quite efficient since, if the base is found, and it is not ambiguous and on a public path, the offset is needed to return the adjusted pointer (and the indirections are not particularly expensive to compute). However, when we throw a null pointer-to-object, this scheme is no longer applicable (and the code currently bypasses the relevant computations, leading to the incorrect catches reported in the issue). ----- The solution proposed here takes a composite approach: 1. When the pointer-to-object points to a real instance (well, at least, it is determined to be non-null), we use the existing scheme. 2. When the pointer-to-object is null: * We note that there is no real object. * When we are processing non-virtual bases, we continue to compute the offsets, but for a notional dummy object based at 0. This is OK, since we never need to access the object content for non-virtual bases. * When we are processing a path with one or more virtual bases, we remember a cookie corresponding to the inner-most virtual base found so far (and set the notional offset to 0). Offsets to inner non-virtual bases are then computed as normal. A base is then ambiguous iff: * There is a recorded virtual base cookie and that is different from the current one or, * The non-virtual base offsets differ. When a handler for a pointer succeeds in catching a base pointer for a thrown null pointer-to-object, we still return a nullptr (so the adjustment to the pointer is not required and need not be computed). Since we noted that there was no object when starting the search for ambiguous bases, we know that we can skip the pointer adjustment. This was originally uploaded as https://reviews.llvm.org/D158769. Fixes llvm#64953
This addresses cases (currently failing) where we throw a null pointer-to-object and fixes llvm#64953. We are trying to satisfy the following bullet from the C++ ABI 15.3: * the handler is of type cv1 T* cv2 and E is a pointer type that can be converted to the type of the handler by either or both of: - a standard pointer conversion (4.10 [conv.ptr]) not involving conversions to private or protected or ambiguous classes. - a qualification conversion. The existing implementation assesses the ambiguity of bases by computing the offsets to them; ambiguous cases are then when the same base appears at different offsets. The computation of offset includes indirecting through the vtables to find the offsets to virtual bases. When the thrown pointer points to a real object, this is quite efficient since, if the base is found, and it is not ambiguous and on a public path, the offset is needed to return the adjusted pointer (and the indirections are not particularly expensive to compute). However, when we throw a null pointer-to-object, this scheme is no longer applicable (and the code currently bypasses the relevant computations, leading to the incorrect catches reported in the issue). ----- The solution proposed here takes a composite approach: 1. When the pointer-to-object points to a real instance (well, at least, it is determined to be non-null), we use the existing scheme. 2. When the pointer-to-object is null: * We note that there is no real object. * When we are processing non-virtual bases, we continue to compute the offsets, but for a notional dummy object based at 0. This is OK, since we never need to access the object content for non-virtual bases. * When we are processing a path with one or more virtual bases, we remember a cookie corresponding to the inner-most virtual base found so far (and set the notional offset to 0). Offsets to inner non-virtual bases are then computed as normal. A base is then ambiguous iff: * There is a recorded virtual base cookie and that is different from the current one or, * The non-virtual base offsets differ. When a handler for a pointer succeeds in catching a base pointer for a thrown null pointer-to-object, we still return a nullptr (so the adjustment to the pointer is not required and need not be computed). Since we noted that there was no object when starting the search for ambiguous bases, we know that we can skip the pointer adjustment. This was originally uploaded as https://reviews.llvm.org/D158769. Fixes llvm#64953
This addresses cases (currently failing) where we throw a null pointer-to- object and is a proposed fix for
#64953
We are trying to satisfy the following bullet from the C++ ABI 15.3:
the handler is of type cv1 T* cv2 and E is a pointer type that can be converted to the type of the handler by either or both of:
o a standard pointer conversion (4.10 [conv.ptr]) not involving conversions to private or protected or ambiguous classes.
o a qualification conversion.
The existing implementation assesses the ambiguity of bases by computing the offsets to them; ambiguous cases are then when the same base appears at different offsets. The computation of offset includes indirecting through the vtables to find the offsets to virtual bases.
When the thrown pointer points to a real object, this is quite efficient since, if the base is found, and it is not ambiguous and on a public path, the offset is needed to return the adjusted pointer (and the indirections are not particularly expensive to compute).
However, when we throw a null pointer-to-object, this scheme is no longer applicable (and the code currently bypasses the relevant computations, leading to the incorrect catches reported in the issue).
The solution proposed here takes a composite approach:
When the pointer-to-object points to a real instance (well, at least, it is determined to be non-null), we use the existing scheme.
When the pointer-to-object is null:
A base is then ambiguous iff:
When a handler for a pointer succeeds in catching a base pointer for a thrown null pointer-to-object, we still return a nullptr (so the adjustment to the pointer is not required and need not be computed).
Since we noted that there was no object when starting the search for ambiguous bases, we know that we can skip the pointer adjustment.
This was : Differential Revision: https://reviews.llvm.org/D158769