[libcxx-commits] [PATCH] D158769: [libc++abi] Handle null pointer-to-object [PR64593]

Sat Aug 26 10:38:26 PDT 2023

iains created this revision.
Herald added a project: All.
iains updated this revision to Diff 553740.
iains retitled this revision from "[libc++abi] WIP for PR64953." to "[libc++abi] Handle null pointer-to-object [PR64593].".
iains edited the summary of this revision.
iains added a comment.
iains edited the summary of this revision.
iains edited the summary of this revision.
iains edited the summary of this revision.
iains edited the summary of this revision.
iains added reviewers: ldionne, rjmccall, urnathan.
iains updated this revision to Diff 553746.
iains retitled this revision from "[libc++abi] Handle null pointer-to-object [PR64593]." to "[libc++abi] Handle null pointer-to-object [PR64593]".
iains edited the summary of this revision.
iains published this revision for review.
Herald added subscribers: libcxx-commits, wangpc.
Herald added a project: libc++abi.
Herald added a reviewer: libc++abi.

added handling for virtual bases and a testcase

iains added a comment.

update the testcase to remove a case that triggers a GCC warning.

iains added a comment.

Some background since it is relevant to the testing done so far.

@urnathan : I think the strategy outlined in the patch header is necessary - I would welcome your opinion on whether it is sufficient.

There is clearly scope for lots of fallout from changes in this library ... so some notes on testing so far:

Mostly, I work on macOS - where the installed system C++abi lib is libc++abi.
I am working on a patch to allow GCC's libstdc++ to sit on top of libc++abi (to try and resolve some of the issues we face when multiple C++ and C++abi libraries get linked into a single executable).

Testing this combination on the GCC testsuite reveals a number of cases where c++abi has different behaviour (or possibly bugs) w.r.t. to supc++.  The cases here are completely reproducible in stand-alone tests and reported in the referenced issue.

The patch does resolve around 85 failing cases where the the common factor is a thrown null pointer-to-object.
The test case passes natively on arm64-apple-darwin21 and on x86_64-apple-darwin21 (via rosetta 2) - I plan to test more widely if there is interest in pursuing this.

This addresses cases (currently failing) where we throw a null pointer-to-object and is a proposed fix for https://github.com/llvm/llvm-project/issues/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.  This 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, 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).

-----

The solution proposed here takes a composite approach:

1. When the pointer-to-object points to a real instance (well, at least, it is 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.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158769

Files:
  libcxxabi/src/private_typeinfo.cpp
  libcxxabi/src/private_typeinfo.h
  libcxxabi/test/catch_null_pointer_to_object_pr64953.pass.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D158769.553746.patch
Type: text/x-patch
Size: 16901 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20230826/744fbcaf/attachment-0001.bin>