LGTM

LGTM

Uploaded wrong commit.

Simplify.

In D146251#4201293, @brad wrote:

Looks like all three OS's have tgmath.h as well. Why not just make this

#if defined(__MINGW32__)
#include <complex.h>
#elif defined(__APPLE__)
#include <math.h>
#else
#include <tgmath.h>
#endif

For now, unless you can also build test on macOS?

Is the -srcdir option no longer needed?

This was already fixed by @aaronpuchert in rGc1a31ee65b3a2bf2b452febb99703b3fdfa43328, in a slightly different way, but functionally equivalent. Test cases were also updated.

In D145884#4189341, @hans wrote:

With this, could we get rid of project_list and runtime_list and just use projects and runtimes directly throughout?

Yes, this looks good to me. Tried with very recent main:

I think this is fine. @emaste can you think of any objections? 8 and 9 are long gone.

Note that the original problem was not only about _Float16 (COMPILER_RT_HAS_FLOAT16 in CMake), but also __bf16 (COMPILER_RT_HAS_BFLOAT16 in CMake). Both failed on i386-freebsd.

LGTM, are there no test cases which are affected by this?

LGTM

In D144444#4142146, @MaskRay wrote:

isPPC32SecurePlt is probably more accurate.

Yes, this is quite a bit nicer. Maybe run the isPPCSecurePlt() function through clang-format, just to be sure of the style?

In D144341#4140652, @arichardson wrote:

LGTM, but I'll let @dim and @emaste decide if FreeBSD 8 code is still needed.

Yeah, this looks quite a bit nicer, and should be more maintainable. Thanks.

LGTM, some minor clang-format nits, but these aren't critical (to me at least :)

In D144232#4136787, @brad wrote:

I noticed this review. I have provided a more complete diff for review at D144321.

Yes, this should save some time while compressing.

Ugh, this is really a messy situation. COMPILER_RT_HAS_FLOAT16 (as a CMake feature) is also used in compiler-rt/test/builtins/CMakeLists.txt, where it is unconditionally used for each arch. I received some failures in buildbots, e.g.:

In D110471#4073813, @MaskRay wrote:

hexdump looks like to come from libmba, not a critical component (libc is usually difficult to instrument, but other libraries are not). I think the user should compile libmba with instrumentation instead of adding interceptors to sanitizers (which adds complexity). @dim @emaste

Abandoning in favor of D136044.

Rebase onto main.

Btw these changes are independent of whether -m32 is used or not, they only check ${arch}. So if compiler-rt is ever changed to go over all support arches with e.g. -target x-y-z, this change would still work as-is. I'm mostly interested in getting the out-of-the-box build going on FreeBSD again, as it has been broken since 15.0.0.

FWIW the BSDs never had lseek64; off_t was 64 bit from the original BSD 4.4 Lite sources... :)

In D130718#3916780, @pengfei wrote:

In D130718#3916720, @vitalybuka wrote:

What do we need to move forward with the patch?

HEAD as-is is broken on Ubuntu 22.10
https://lab.llvm.org/buildbot/#/builders/169/builds/13450/steps/7/logs/stdio

I noticed the bot is starting to fail 3 days ago, but the patch merged for months. Is there any recent change exposed the problem?

As disscussed in https://discourse.llvm.org/t/how-to-build-compiler-rt-for-new-x86-half-float-abi/63366/25 the build of 32-bits using 64-bit config is the root cause. I think always turning on SSE2 explicitly in the 64-bit config should be a feasible workaround.

In D136044#3861187, @arichardson wrote:

What is being done for things like 64bit atomics and int128 support? Surely that will have the same problem?

Another note to clarify: the problem is that the builtins are built *twice*, once for x86_64 and once for i386, the latter using the -m32 compiler flag. In the CMake configuration phase however, the feature detection is always done with the default compiler flags, i.e. without -m32, and therefore it will find features that aren't supported for 32-bit targets.

@brooks btw didn't you encounter this problem during llvm-devel port builds ? E.g. it dies with:

LGTM

Yes, this should probably be fine for 16.

I'm fine with it, adding @brooks so he's aware for the FreeBSD llvm port.

So I've been trying again with the following diff applied on top of this one:

Address review comments.

Unfortunately it doesn't really change anything on the FreeBSD build side. It still shows similar output when I put some messages in there:

GCC 12 -Wformat -pedantic emits a warning:

warning: ISO C17 does not support the ‘%b’ gnu_printf format [-Wformat=]

The behavior is not ported (and it's unclear to me how to implement it).

LGTM.

Okay, so do we continue with this approach at all, then? The problem that x86_64 builds fail by default on non-gcc compilers still stands. I'm just not good enough in handling the CMake logic to figure that out, if we have to handle it via CMake.

In D130718#3686439, @pengfei wrote:

@kparzysz reminded me. This is not the right direction to solve the problem. The ABI of runtime libraries should be constant rather than changing with different features. Imagining a program may be built with or without SSE2 and linked with the same runtime. Another problem is compatibility with libgcc. Using uint16_t is not compatible with libgcc anymore.

Fortunately, LLVM codegen (unfortunately Darwin isn't) was using __gnu_h2f_ieee/__gnu_f2h_ieee before the change https://godbolt.org/z/GnrT5vGr1. So an easy way to solve the problem is always assign -msse2 when build extendhftf2/trunctfhf2.

I have a patch D128872 for Darwin targets, I hope we can put it into the 15.x release.

In D130718#3685679, @tstellar wrote:

Is this something we need in the release branch?

Fix typo.

LGTM, are there any tests that need to be enabled explicitly?

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.