Page MenuHomePhabricator

[libc++abi] Refactoring works around __dynamic_cast
Needs ReviewPublic

Authored by Lancern on Nov 14 2022, 11:37 PM.

Details

Reviewers
philnik
ldionne
EricWF
avogelsgesang
Group Reviewers
Restricted Project
Restricted Project
Summary

This commit contains refactorings around __dynamic_cast without changing its
behaviors. Some important changes include:

  • Refactor __dynamic_cast into various small helper functions;
  • Move dynamic_cast_stress.pass.cpp to libcxx/benchmarks and refactor it into a benchmark. The benchmark performance numbers are updated as well.

Diff Detail

Event Timeline

Lancern created this revision.Nov 14 2022, 11:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2022, 11:37 PM
Lancern requested review of this revision.Nov 14 2022, 11:37 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 14 2022, 11:37 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
tschuett added inline comments.
libcxxabi/src/private_typeinfo.cpp
155

If you make the if explicit, you get too much indentation?

Lancern updated this revision to Diff 477689.Nov 23 2022, 11:47 PM

Rebase onto the main branch so that CI triggers correctly

Hi @Lancern,

Thanks for looking into the performance of dynamic_cast and for splitting your initial patch from https://reviews.llvm.org/D137315!
Unfortunately, it looks like your patch is still not rebased onto current main. E.g., your patch claims to delete a couple of lines which are not currently on https://github.com/llvm/llvm-project/blob/main/libcxxabi/src/private_typeinfo.cpp. I marked one of those examples below. That's also why the CI currently fails (see "patch application failed" link further up on this page). Let's get that sorted out first, and as soon as the diff is correct, I will start reviewing your changes (I see little value in reviewing a non-applicable patch)

libcxxabi/src/private_typeinfo.cpp
667

E.g., here: this line of code is not currently in https://github.com/llvm/llvm-project/blob/main/libcxxabi/src/private_typeinfo.cpp but your patch still tries to delete it.

Ah, now I understand how you are planning to apply your changes. I assumed you first wanted to apply this patch, and then https://reviews.llvm.org/D138005 on top of it (i.e. first the refactoring, then the perf improvement). But you want to apply them exactly the other way around.

In that case, you need to set the "parent revision" here on phabricator, such that the CI knows how to apply your patch...

Lancern added a comment.EditedDec 9 2022, 6:26 PM

Seems like the failed unit tests are not related to this patch nor the parent revision.

Seems like the failed unit tests are not related to this patch nor the parent revision.

The failures on apple look related. The ones on AIX are probably just timeouts.

Here is one of the error messages produced on Apple

/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/w4-4macminivaultcom-local/llvm-project/libcxx-ci/libcxxabi/src/private_typeinfo.cpp:189:53: error: use of undeclared identifier 'dynamic_type'
                    ", %s.\n", static_type->name(), dynamic_type->name());
                                                    ^
/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/w4-4macminivaultcom-local/llvm-project/libcxx-ci/libcxxabi/src/private_typeinfo.cpp:191:69: warning: missing field 'dst_ptr_not_leading_to_static_ptr' initializer [-Wmissing-field-initializers]
        info = {dst_type, static_ptr, static_type, src2dst_offset, 0};
                                                                    ^
/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/w4-4macminivaultcom-local/llvm-project/libcxx-ci/libcxxabi/src/private_typeinfo.cpp:193:9: error: use of undeclared identifier 'dynamic_type'
        dynamic_type->search_above_dst(&info, dynamic_ptr, dynamic_ptr, public_path, true);
        ^
/Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/w4-4macminivaultcom-local/llvm-project/libcxx-ci/libcxxabi/src/private_typeinfo.cpp:286:69: warning: missing field 'dst_ptr_not_leading_to_static_ptr' initializer [-Wmissing-field-initializers]
        info = {dst_type, static_ptr, static_type, src2dst_offset, 0};
                                                                    ^
2 warnings and 2 errors generated.

It seems your code inside the _LIBCXXABI_FORGIVING_DYNAMIC_CAST ifdef is incorrect. This flag is apparently only enabled on Apple

Seems like the failed unit tests are not related to this patch nor the parent revision.

I restarted the test run for the parent revision, and the parent revision now indeed has a green CI run

Lancern updated this revision to Diff 485191.Dec 24 2022, 2:05 AM

Fix build issues on Apple system

Lancern updated this revision to Diff 496685.Feb 11 2023, 6:45 AM

Rebase onto b945bb246ad9.

Lancern updated this revision to Diff 498839.Feb 20 2023, 6:46 AM

Rebase work onto the latest main.

Lancern updated this revision to Diff 499449.Feb 22 2023, 4:03 AM

Sync with the parent commit.

philnik accepted this revision.Mar 31 2023, 2:11 PM

Thanks! LGTM.