This is an archive of the discontinued LLVM Phabricator instance.

[libcxxabi] Resync llvm & libcxxabi's demangler
ClosedPublic

Authored by urnathan on Oct 7 2022, 7:09 AM.

Details

Reviewers
ldionne
bader
Group Reviewers
Restricted Project
Commits
rG702d937f1e1d: [libcxxabi]: Resync llvm & libcxxabi's demangler
Summary

I noticed this when addressing pr58117

Sadly the demangler copies have diverged. This resyncs them by
a) pulling the meaningful llvm changes into libcxxabi's gold copy.
b) rerunning the sync script

Please be aware of the syncing (having a single version and teaching cmake about that was beyond my abilities)

It turns out the new include is needed, but highlights another problem -- the placement new uses presume the allocator succeeds. That;s not true in general. Currently the allocator calls std::terminat on failre, but that means it isn't an ABI-compliant demangler. That should return an error code on memory exhaustion. https://github.com/llvm/llvm-project/issues/58289 captures that problem

Diff Detail

Event Timeline

urnathan created this revision.Oct 7 2022, 7:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2022, 7:09 AM
urnathan requested review of this revision.Oct 7 2022, 7:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2022, 7:09 AM
urnathan updated this revision to Diff 466786.Oct 11 2022, 5:53 AM
urnathan edited the summary of this revision. (Show Details)
urnathan edited the summary of this revision. (Show Details)Oct 11 2022, 6:16 AM
bader accepted this revision.Oct 12 2022, 2:52 AM
bader added subscribers: steffenlarsen, jcranmer-intel.

@urnathan, sorry, we didn't notice that https://reviews.llvm.org/D130909 introduces divergence. @steffenlarsen, @jcranmer-intel, FYI.
Regarding dropping LLVM_ prefix from header guards, it looks like these were removed with some automatic tool (https://github.com/intel/llvm/commit/c13a09a462807936f9eb17cc64f53ad7c9e8ddec), so I would expect someone else will run the tool again and add them back. Any ideas how to prevent this type of changes in the future? Maybe we can keep LLVM_ prefix for sources in llvm/ directory and just ignore this difference. What do you think?

This revision is now accepted and ready to land.Oct 12 2022, 2:52 AM

The libcxxabi copy is the canonical version, right? If so, we could add a test inside LLVM that ensures that re-syncing the files does not produce any difference in output.

@urnathan, sorry, we didn't notice that https://reviews.llvm.org/D130909 introduces divergence. @steffenlarsen, @jcranmer-intel, FYI.
Regarding dropping LLVM_ prefix from header guards, it looks like these were removed with some automatic tool (https://github.com/intel/llvm/commit/c13a09a462807936f9eb17cc64f53ad7c9e8ddec), so I would expect someone else will run the tool again and add them back. Any ideas how to prevent this type of changes in the future? Maybe we can keep LLVM_ prefix for sources in llvm/ directory and just ignore this difference. What do you think?

yeah, that;s annoying, the src and dst are not identical anyway, because we insert DO NOT EDIT comments via sed.

I'm sure I can tweak that to modify the idempotency macros. then one can apply the script, and if there's no change in the src version there will also be no change in the dst version.

This revision was landed with ongoing or failed builds.Oct 12 2022, 10:13 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2022, 10:13 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript

The libcxxabi copy is the canonical version, right? If so, we could add a test inside LLVM that ensures that re-syncing the files does not produce any difference in output.

Given that we now have a monorepo, it should be possible to have one single source of truth and use relative source paths from either the llvm/ or the libcxxabi/ implementation to include the right files, right? Or do we intend for it to be possible to build LLVM projects with only a subset of the top-level directories checked out?