Yes, this looks good to me. Thanks for this fix!

https://bugs.llvm.org/show_bug.cgi?id=50522

Hm so this is definitely something that we'd want in 12.0.1? @tstellar it doesn't change any ABI, so should be perfectly safe. I'll create a PR.

Reverted due to the influence this had on some other test cases. I'll do a full check-all and go over them one by one, but it will take some time...

Update comment and commit message, and add mips64 test.

Add test case, and update comment to explicitly mention ld.lld.

Welcome, Kate.

Squash commits to convince Phabricator.

Address review comments.

In D98602#2663871, @fhahn wrote:

In D98602#2651521, @dim wrote:

My previous update accidentally dropped the changes to SCCP.cpp.
Squashed both local revisions to get both the functionality change and
the test case.

Thanks for adding the test! As long as it fails with ASan, that's fine, there are multiple bots building with that enabled. If it would be possible to reduce it even further, that would obviously be great, but it probably needs a large enough input to trigger.

My previous update accidentally dropped the changes to SCCP.cpp.
Squashed both local revisions to get both the functionality change and
the test case.

LGTM, but this would probably need some sort of test, at least that it correctly accepts the flag(s) now?

LGTM

Looks like a good idea to me.

Added a bugpoint-reduced test case. (I could only get clang and opt to
trigger errors on a Linux machine by building with
LLVM_USE_SANITIZER=Address.)

Ok, I have not been able to make either llc or opt show any addressanitizer errors with the .ll output from the .cpp test case. Is it OK to check in a .cpp test case instead? That would require clang at regression test time though, and I'm unsure if that is available?

Hm, I've reduced the test case to the following C++ source:

In D98602#2624871, @lebedev.ri wrote:

Test missing.

Oh please s/FreeVBD/FreeBSD in the description before committing :)

Yes please, this warning has annoyed me for ages. I think we can't get rid of this include entirely, since we need to get at the declarations even if they are deprecated.

In D96720#2563989, @ldionne wrote:

Is the 12.x release fixed, or does this patch need to be cherry-picked to the branch?

Note, this is mainly aimed at getting the 12.0.0 release built, as I had to apply a custom hack to turn the operators on. But I would like to get rid of the hack and have this applied in a way acceptable to the libc++ maintainers.

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.