LGTM.

@sylvestre.ledru removed the minor version from the binary (on purpose, I think?) in rGa8b717fda42294d1c8e1f05d71280503e5839f14:

LGTM (this mirrors what is in libcxx/include/__config btw)

Address comments from review:

• Move FreeBSD specific include under common block
• Use #if #elif #endif for OS specific implementations

Yes, this looks pretty fine to me, but indeed needs a test.

LGTM.

In D68269#2322295, @ldionne wrote:

In D68269#2322291, @dim wrote:

In D68269#2322151, @ldionne wrote:

Ping. Please take a look. I'll ship this in a week if nobody cares.

On FreeBSD we replaced (quite a while ago now) libsupc++ with libcxxrt. Currently this also provides new and delete operators, but not all that are provided by libc++ (as of ~11.0.0 rc5):

[...]

That said, both libcxxrt and libc++ mark their implementations as weak, so it's good guess which one wins... I'm actually not completely sure, except for the variants that libcxxrt doesn't implement. :)

In any case, we'd at least have to add the variants with align_val_t and with the const nothrow_t to libcxxrt, to make it backwards compatible. Another option would be to switch to libc++abi, but that is quite a lot more work.

The other option, which I would argue is the least amount of work, is for you to configure CMake with -DLIBCXX_ENABLE_NEW_DELETE_DEFINITIONS=ON. The libc++ produced with that option should match what you have today.

In D68269#2322151, @ldionne wrote:

Ping. Please take a look. I'll ship this in a week if nobody cares.

In D73425#2266446, @Bdragon28 wrote:

Any chance of a backport to 11?

In D86510#2240266, @efriedma wrote:

We are triggering -Watomic-alignment because __c11_atomic_is_lock_free(8) expands to a __atomic_is_lock_free call for mips32 since the property is not known statically, so the branch inside the switch emitted and contains a call to __atomic_load_8 which triggers the warning.

Which -Watomic-alignment warning are we talking about? The "max lock-free size" one, or the "actual alignment" one?

Please note, I did a follow-up commit in rG47b0262d3f82, to address compile errors I received from the x86_64-linux-debian build bot:

This has been obsoleted by D86397 (committed as rGcde8f4c164a2 with follow-up rG47b0262d3f82).

After this change, it turns out that some of the errors that this results in can be very confusing. For example, when building ocaml, it uses a .S file (https://github.com/ocaml/ocaml/blob/trunk/runtime/amd64.S#L729) containing:

As @JDevlieghere suggests, only instantiate Error objects when necessary.

Hm, the static FrameHeaderCache ProcessFrameHeaderCache; is biting us here, at least in a multithreaded process like lld, which is the process that is crashing for me all the time now. I added some instrumentation, which shows the problem by adding a thread ID to each log message, and by adding an assert in FrameHeaderCache::add() that MostRecentlyUsed != nullptr:

I turned on _LIBUNWIND_DEBUG_FRAMEHEADER_CACHE, and indeed such segfaults appear immediately after the message libunwind: FrameHeaderCache reset. That means FrameHeaderCache::MostRecentlyUsed has just been set to nullptr ...

It seems that after this change, I'm getting sporadic lld segfaults in libunwind, which look like:

For me, this fixes the assertion for both the original test case (a fully preprocessed ASTReader.cpp) and the minimized test case from https://bugs.llvm.org/show_bug.cgi?id=47041#c1

In D85082#2190303, @arichardson wrote:

In D85082#2190291, @arichardson wrote:

Can we instead add a dependency on libpthread.so/libthr.so to libasan.so?

Never mind, it seems like it's already there:

File: ./lib/clang/12.0.0/lib/freebsd/libclang_rt.asan-x86_64.so
Format: elf64-x86-64
Arch: x86_64
AddressSize: 64bit
LoadName: libclang_rt.asan-x86_64.so
NeededLibraries [
  libc++.so.1
  libc.so.7
  libcxxrt.so.1
  libdl.so.1
  libgcc_s.so.1
  libm.so.5
  librt.so.1
  libthr.so.3
]

Hm, this review's still open after two years, and even as of 2020-08-02 clang still crashes on the sample. :)

Note that as of rG1db4318766256f25a03ef80af8dbb3f99743ebe9, applying this change results in the Passed testcases increasing from 49259 to 49666, so more than 400 Failed test cases are fixed by it.

In D77776#2187334, @myfreeweb wrote:

In D77776#2187273, @dim wrote:

+#ifdef __FreeBSD__
+	  return __FreeBSD_version / 100000;
+#else
+	  return 10;
+#endif

No, that would hardcode something at build time. We need to respect whatever triple the user is passing in.

Nobody has proposed not respecting that, this is all in the context of the if (Major == 0) fallback

In D77776#1993211, @arichardson wrote:

I don't like the fact that this only changes one of the users of getTriple().getOSMajorVersion().

In D83645#2160761, @MaskRay wrote:

@dim

Hi, your git commit contains extra Phabricator tags. You can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (--date=now is my personal preference (author dates are usually not useful. Using committer dates can make log almost monotonic in time))

llvm/utils/git/pre-push.py can validate the message does not include unneeded tags.

LGTM.

Okay, after a long while of attempting to make a reproduction scenario, I finally managed one that consistently worked. As BSD make puts in different redirections for stderr when running in -j mode, it turned out that I could simply run the compilation twice on a terminal, first normally, then with stderr redirected to /dev/null, e.g. my test script is:

For completeness sake, here are the valgrind outputs of clang rGf7f1abdb889 (llvmorg-11-init-16778) built by gcc 9.3.0-10ubuntu2 (in RelWithDebInfo mode):

In D69825#2071687, @dim wrote:

Trying to reduce this now.

In D69825#2070926, @hans wrote:

In D69825#2063979, @dim wrote:

FWIW, this change causes https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246630, see in particular https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246630#c18.

For some reason, not spawning a new process for the cc1 stage can lead to non-reproducible output. The exact cause is not known yet.

Can you share the preprocessed source and compiler command-line for printf.c, either on the bug or here?

FWIW, this change causes https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246630, see in particular https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=246630#c18.

In D56398#2038621, @ldionne wrote:

Since we should pick up FreeBSD's EINTEGRITY via include_next, I don't see why we need this patch. Can we abandon?

In D79774#2031805, @MaskRay wrote:

In D79774#2031755, @dim wrote:

Note I already merged this fix (minus the test case) into FreeBSD here: https://svnweb.freebsd.org/changeset/base/360697, since we needed it ASAP for being able to build big-endian ARM kernel code. So in that sense it's already been exercised for a bit. :)

MFC after: immediately, since this fix is meant for stable/11

I still think the offending FreeBSD assembly can be fixed to be more portable.

Note I already merged this fix (minus the test case) into FreeBSD here: https://svnweb.freebsd.org/changeset/base/360697, since we needed it ASAP for being able to build big-endian ARM kernel code. So in that sense it's already been exercised for a bit. :)

Verified that this fixes both the original test case (which is rather large so I didn't attach it to the PR), and the minimized test case.

Trying this out now, thanks!

In D77776#1971543, @arichardson wrote:

Alternatively, the checks could be changed to also handle OSMajorVersion == 0 and translate that to 10.
This seems to be what NetBSD.cpp does. Darwin.cpp also infers the version from the host when running on macos.

FYI FreeBSD is one of the few OSes that enables -Wsystem-headers by default, *and* uses libc++ as its default C++ library. So we've already submitted a bunch of warning fixes for libc++.

In D6379#1934620, @nickdesaulniers wrote:

Hello, we're hitting this issue when assembling the Linux kernel with Clang (https://github.com/ClangBuiltLinux/linux/issues/716). I'd be happy to dust this off/rebase and add tests. @dim would you prefer to do it or shall I?

As far as I'm concerned, most suggestions make sense, and it is good to have this information clearly stated.

In D76192#1923936, @hans wrote:

This seems reasonable, but can we wait until after the release? I would prefer to not change these things right now.

Note: one thing that I am unsure about is the _NOEXCEPT after some of the copy constructors. I only applied those to classes that have other _NOEXCEPT constructors, but copying may involve other things that could throw.

In D73990#1868108, @plotfi wrote:

In D73990#1868085, @dim wrote:

In D73990#1868071, @plotfi wrote:

This breaks in our downstream runtimes builds for armv7-none-linux-androideabi16. FYI

Can you point me to some build logs? (I didn't receive any failure mails from bots yet.)

Posted a fix at D74358

Yeah sorry, I used a whitelist instead, here: rG52f2df1ecdd79cc550b694ab280f3b0396d7cf9a

In D73990#1868071, @plotfi wrote:

This breaks in our downstream runtimes builds for armv7-none-linux-androideabi16. FYI

Ping, can we get this committed then? :)

Remove stray newline.

Add better NetBSD implementation of internal_dlinfo().

• Rebase onto reformatted files.
• Add internal_dlinfo() for FreeBSD and NetBSD, with stubs for other platforms.
• Add GET_LINK_MAP_BY_DLOPEN_HANDLE() macro for NetBSD.