In D123177#3443809, @brooks wrote:

This fails to configure in cmake when enabled as a project.

$ cmake ../llvm -DLLVM_ENABLE_PROJECTS='llvm;clang;lld;openmp'
...
-- Configuring done
CMake Error at /home/bed22/git/llvm-project/openmp/libomptarget/plugins/amdgpu/CMakeLists.txt:74 (add_dependencies):
  The dependency target "omptarget.devicertl.amdgpu" of target
  "omptarget.rtl.amdgpu" does not exist.


-- Generating done
CMake Generate step failed.  Build files cannot be regenerated correctly.

Reverting this commit allows cmake to succeed. The linkage fix in rG2e0cb61570ec does not fix this issue.

FYI https://github.com/llvm/llvm-project/issues/56103

To answer my own question partially, the original commit is Howard's: rG088e37c77aafaec5ead8fbe7ebf918265e6b86f2:

Despite my pathological distrust of spin locks, the number just don't lie. I've put a small spin in __sp_mut::lock() on std::mutex::try_lock(), which is testing quite well. In my experience, putting in a yield for every failed iteration is also a major performance booster. This change makes one of the performance tests I was using (a highly contended one) run about 20 times faster.

In D126882#3553338, @ldionne wrote:

This patch was put together in collaboration with a performance engineer on our side. It was tested with a use case that was previously problematic and this patch did solve the issue. We believe this should be an improvement for all OSes if their pthread_mutex_lock is reasonable (which is the case on Linux and Apple at least)

In any case it would be nice if it was possible to decide whether ABIv2 will be pinned down (and ABIv3 is made into the 'new' unstable) before 15.0 is branched. :-)

In D126462#3540480, @philnik wrote:

...

But for 14.0, which is our trunk/main/head, there is no such ABI promise, therefore we can simply bump libc++.so to .2 now and fix the issue that way. If the upstream libc++ ABI version gets bumped to 2 though, we'll have to bump our .so to .3 I'm afraid, since the ELF soversion doesn't really allow "half-sized" version numbers.

Do you have to bump the version now or would it be possible for you to wait maybe one or two months so we can get things sorted out upstream?

In D126462#3540371, @philnik wrote:

In D126462#3539933, @emaste wrote:

Sounds good to me. We can bump the .so version in the base system, but how do we treat it upstream (i.e., here in llvm) and in the ports tree?

Would it make sense from your side to to declare ABIv2 stable?

Thanks a lot. I'll make sure to put up a libc++ review now to get rid of the FreeBSD std::pair problem in the future! :)

In D125899#3531250, @kpn wrote:

Will this fix FreeBSD 12 (and maybe 11?). FreeBSD 12 is still very much supported.

In D125899#3529158, @dim wrote:

...

There's a subtle difference between the two std::vector constructors, mentioned at https://en.cppreference.com/w/cpp/container/vector/vector, where variant 3 is "Constructs the container with count copies of elements with value value" (so it needs a copy constructor), and variant 4 is "Constructs the container with count default-inserted instances of T. No copies are made". E.g. the latter should not require a copy constructor, but just a default constructor.

In D125899#3522946, @dblaikie wrote:

In D125899#3522879, @kparzysz wrote:

Changed the copy to a deep one.

Ah, if the type wasn't deep copying before - ie: its default copy operation wasn't actually valid/probably would've resulted in duplicate free/delete/etc and generally been broken. Then my use case was just getting lucky because it was copying empty maps.

If that's the case, then maybe that does argue in favor of the unique_ptr solution/marking the broken copy ctor as deleted. (or figuring out some way to do the construction in-place in my case without using copying)

I'll take a closer look.

In D125611#3520595, @dblaikie wrote:

In D125611#3519571, @dim wrote:

...

Ultimately, this is caused by _LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR in /usr/include/c++/v1/__config (this was last modified by @EricWF, but a long time ago). If this setting is enabled (to preserve the old ABI), std::pair will always have a non-trivial copy constructor, and since IntervalMap contains a std::pair (via RootLeaf -> IntervalMapImpl::LeafNode -> NodeBase<std::pair<KeyT, KeyT>, ValT, N>), IntervalMap itself is also non-trivially copyable. std::vector in turn requires its value type to be copy-constructible (at least certainly when you use the constructor with a size).

I was hoping something I could reproduce without having FreeBSD myself - either on godbolt (though godbolt might not have different OS platforms as a primary feature) or something like that. Or a brief example that emulates the quirk of FreeBSD to better understand what we're working around here.

I can confirm that this fixes both my original full test case, and the minimized variant. Since this change resembles D78668, I think it is relatively safe to apply now, and merge to the release/14.x branch?

In D125611#3517574, @dblaikie wrote:

Some more details on the specifics in the patch description would be good - I'm still not 100% clear on the reproduction case to understand what we're working around, exactly. Do you have a godbolt reproduction/small example, by chance?

Cross-reference: https://github.com/llvm/llvm-project/issues/55414

In rG0d8cb8b399adcd17e8bf17be7814d030308c8b82#1084447, @dblaikie wrote:

In rG0d8cb8b399adcd17e8bf17be7814d030308c8b82#1084182, @kparzysz wrote:

This fails to compile:

$ /usr/bin/c++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/w/bld/org/lib/DebugInfo/DWARF -I/w/src/llvm.org/llvm/lib/DebugInfo/DWARF -I/w/bld/org/include -I/w/src/llvm.org/llvm/include -I/usr/local/include -stdlib=libc++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -ffunction-sections -fdata-sections -O3 -DNDEBUG  -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -c /w/src/llvm.org/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
In file included from /w/src/llvm.org/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:8:
In file included from /w/src/llvm.org/llvm/include/llvm/DebugInfo/DWARF/DWARFVerifier.h:12:
In file included from /w/src/llvm.org/llvm/include/llvm/ADT/Optional.h:19:
In file included from /w/src/llvm.org/llvm/include/llvm/ADT/Hashing.h:51:
In file included from /usr/include/c++/v1/algorithm:653:
In file included from /usr/include/c++/v1/functional:500:
In file included from /usr/include/c++/v1/__functional/function.h:20:
In file included from /usr/include/c++/v1/__memory/shared_ptr.h:22:
/usr/include/c++/v1/__memory/allocator.h:154:28: error: call to implicitly-deleted copy constructor of 'llvm::IntervalMap<unsigned int, unsigned long, 12>'
        ::new ((void*)__p) _Up(_VSTD::forward<_Args>(__args)...);
                           ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/v1/__memory/allocator_traits.h:290:13: note: in instantiation of function template specialization 'std::allocator<llvm::IntervalMap<unsigned int, unsigned long, 12>>::construct<llvm::IntervalMap<unsigned int, unsigned long, 12>, const llvm::IntervalMap<unsigned int, unsigned long, 12> &>' requested here
        __a.construct(__p, _VSTD::forward<_Args>(__args)...);
            ^
/usr/include/c++/v1/vector:1085:25: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<llvm::IntervalMap<unsigned int, unsigned long, 12>>>::construct<llvm::IntervalMap<unsigned int, unsigned long, 12>, const llvm::IntervalMap<unsigned int, unsigned long, 12> &, void>' requested here
        __alloc_traits::construct(this->__alloc(), _VSTD::__to_address(__pos), __x);
                        ^
/usr/include/c++/v1/vector:1178:9: note: in instantiation of member function 'std::vector<llvm::IntervalMap<unsigned int, unsigned long, 12>>::__construct_at_end' requested here
        __construct_at_end(__n, __x);
        ^
/w/src/llvm.org/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:410:48: note: in instantiation of member function 'std::vector<llvm::IntervalMap<unsigned int, unsigned long, 12>>::vector' requested here
  std::vector<IntervalMap<uint32_t, uint64_t>> Sections(
                                               ^
/w/src/llvm.org/llvm/include/llvm/ADT/IntervalMap.h:970:14: note: copy constructor of 'IntervalMap<unsigned int, unsigned long, 12>' is implicitly deleted because variant field 'leaf' has a non-trivial copy constructor
    RootLeaf leaf;
             ^
1 error generated.

What standard library is in use here? I believe it's non-conformant (& since lots of other buildbots haven't failed on this, that seems likely), possibly outdated.

LLVM requires C++14 and C++14 provides a std::vector constructor that takes a count (& not a default value) and default constructs the elements in-place without copying a singular default value.

Add llvm/utils/gn/secondary/lldb/include/lldb/Version/BUILD.gn while we're here.

Sure, LGTM.

It's a pity this one didn't make it into 14.0.0, as you now can't even include <ranges> without immediately getting an error:

In D119969#3351389, @ldionne wrote:

In D119969#3349883, @manojgupta wrote:

Running readelf shows following in our shipping shared libs.

 231: 00000000000c7db0     8 OBJECT  GLOBAL DEFAULT   23 _ZNSt3__123__libcpp_debug_functionE
 668: 0000000000075160    13 FUNC    GLOBAL DEFAULT   14 _ZNSt3__127__libcpp_set_debug_functionEPFvRKNS_19__libcpp_debug_infoEE
1438: 0000000000075110    79 FUNC    GLOBAL DEFAULT   14 _ZNSt3__129__libcpp_abort_debug_functionERKNS_19__libcpp_debug_infoE
1979: 0000000000074ef0   544 FUNC    GLOBAL DEFAULT   14 _ZNKSt3__119__libcpp_debug_info4whatEv

Okay, right -- so as expected, you've been shipping support for the debug mode as part of the dylib, and if any user has been enabling assertions with _LIBCPP_DEBUG=0, they are relying on those symbols. So I think my proposed direction stands (it's this patch as-is). It's not the most elegant design since we're reusing old __libcpp_debug_foo for assertions, but I think it's the best decision engineering-wise for the time being.

I can't vouch for any of the PowerPC specific stuff, but I would like to get it into FreeBSD if at all possible. :)

FWIW, this breaks compilation of Firefox:

LGTM (@emaste I've never used the standalone build, always as part of the 'full monty' all llvm components)

Hm @pkubaj do you remember if this is needed for 32-bit PowerPC on FreeBSD? I can't remember...

In any case, LGTM for this review itself of course.

Having said that, if this document is supposed to be published on GitHub, and read by people primarily via GitHub, I suppose it would make sense to change all the mentioned usernames to GitHub usernames instead of Phabricator usernames?

Note: andykaylor is Andy's GitHub username: https://github.com/andykaylor
and similarly, smaslov-intel is Sergey's: https://github.com/smaslov-intel

As far as I know:
Andy Kaylor = @andrew.w.kaylor
Sergey Maslov = @smaslov

Address review comments.

In D102707#3269962, @mehdi_amini wrote:

Your profile shows a great amount of maps lookup though. It can be that the map size exploded for some reason or that we query it much more?

The creduce run is almost finished, I'll go create a ticket now.

The top 10 list from perf report output shows that it's doing quite a lot of StringMapImpl work:

In D102707#3267708, @mehdi_amini wrote:

In D102707#3267278, @dim wrote:

For some reason, this commit causes a huge memory usage, and extreme slowdown, when compiling the FreeBSD port math/openturns (https://github.com/openturns/openturns) port; see https://bugs.freebsd.org/261341.

With llvmorg-13-init-11364-g0ce58c52d50 (i.e. just before this commit):?
...
I had to terminate the clang process after ~9 minutes, as it was using >115GiB virtual memory, and this was a bit of a risk to crash the (shared) machine I was working on.

Any ideas, before I attempt reducing the ~196,000 line test case? :)

This is pretty bad... Can you clarify if your clang is built with assertions or in release mode?

For some reason, this commit causes a huge memory usage, and extreme slowdown, when compiling the FreeBSD port math/openturns (https://github.com/openturns/openturns) port; see https://bugs.freebsd.org/261341.

In D117811#3262289, @philnik wrote:

The build failure looks to me like a gcc bug. Can anybody confirm this? Or is it implementation divergence?

How do you mean "no practical difference"? _VSTD is defined as std::_LIBCPP_ABI_NAMESPACE, and in turn, _LIBCPP_ABI_NAMESPACE is configurable by the user, or defaults to mangling the ABI version in it. So this would be a huge change, for unknown benefit?

LGTM, thanks!

LGTM too. I will commit this now.

In D68480#3218277, @ldionne wrote:

Also, on a different note: I'm pretty serious about FreeBSD needing to implement pre-commit CI if it wants to be supported officially. It's a small investment, but it needs to happen for libc++ to keep FreeBSD as part of its list of supported platforms. @dim can you help with this?

Okay, in my case the test case is quite a bit shorter:

David, in this particular change, does "valid and handled successfully" mean "not crash or assert"? I just ran into the following assertion when building part of llvm (13.0.0) itself under FreeBSD 12:

In D109789#3192593, @reames wrote:

Can you file a bug now that the transition is complete?

It's a pity this never landed. Now libc++ can't work on older arm without non-standard patches... :)

FWIW, this commit turned out to break the FreeBSD dns/bind916 port, see https://bugs.freebsd.org/259921.

In D112753#3181695, @noajshu wrote:

@dim, I just committed D115189 which should solve your problem. Thanks for your help and please let me know if your build still fails.

In D112753#3180838, @noajshu wrote:

Thanks @dim, I have a patch that should fix this D115189
Would that solve the breakage on freebsd?

This breaks building on FreeBSD, where I see:

From a fellow FreeBSD user I received a test case which *appears* to be fixed by this particular review/commit, and which otherwise leads to an assertion:

In D115074#3170635, @JDevlieghere wrote:

We should move this from the wrapper to the header block, similar to what Pavel did for Python: https://reviews.llvm.org/rG9a14adeae00015798843ff5cad987e5fdbdddb34

In D114186#3167979, @jrtc27 wrote:

I've tried to stay out of this for my own sanity, but my objection has not been about the end state which, in a world where SHF_GNU_RETAIN can be relied upon to work, is a more sensible state than historically (I would've preferred that uses that wanted the new GC behaviour use a new way of doing things rather than repurposing something that didn't do the right thing and then forcing their needs on it, breaking existing uses, but I can see and appreciate the technical reasons for doing it this way, and ignoring compatibility issues it does make more sense). My objection has been that SHF_GNU_RETAIN is not ubiquitous enough, both in terms of support and use, for the default start-stop-gc behaviour to have been changed.

Address review comments.

FWIW, this fixes both the minimized test case from PR52594, and the full original test case from https://bugs.freebsd.org/260000.

Squash commits because arc tricked me.

Drop trailing dots from report messages.

LGTM

Note that this is the version as we have it in the FreeBSD base system version of llvm 13.0.0. Without the isPPC64() check, we would get unexpected R_X86_64_REX_GOTP relocations in our kernel modules, which the kernel module loader cannot handle; see e.g. https://reviews.llvm.org/D109090#3113552 and further comments.