Yes, abandoning seems reasonable. I think the main idea of step 4 when I filed PR22780 was to reduce memory usage, but this didn't turn out to be a bottleneck.

LGTM.

Nice!

LGTM! Thanks for reworking this.

Who changed the behaviour? Can we get them to review the behaviour of the upgrade?

LGTM.

I have a few more nitpicks; LGTM after you fix those.

In D34331#831686, @arphaman wrote:

Don't you need // UNSUPPORTED: c++98, c++03 since std::function is supported in C++11 only?

Oh, also found a couple of things you should likely split into prep commits to simplify this patch.

This looks like a great improvement. I've littered the patch with nit-picks, but my main concern is that there aren't any unit tests for the new data structures. I wonder if there's a hacky way to set that up...

In D35951#823626, @aprantl wrote:

In D35951#823564, @dexonsmith wrote:

The code changes and test/Bitcode/upgrade-dbg-value.ll LGTM.

I spot-checked the other testcases, and couldn't find any that had anything other than i64 0. Is that expected? Or can you point me at a non-trivial one?

We have been phasing out any remaining use of the offset field and replaced it with DIExpressions. This is also why the upgrade merely strips any dbg.value intrinsics with nonzero offsets. It's effectively dead code.

The code changes and test/Bitcode/upgrade-dbg-value.ll LGTM.

In D35951#823082, @aprantl wrote:

In D35951#823038, @dexonsmith wrote:

Should there also be an update to LangRef?

In D35951#823015, @aprantl wrote:

For those living downstream I should also note that the IR testcase updates are not strictly necessary, since the auto upgrade code also works for assembler input, but I think it is less confusing to have the testcases use the up-to-date version of the intrinsics.

I feel like it would be better to error, and require the upgrade. Is that not practical for intrinsics?

LLParser::ValidateEndOfModule() always calls UpgradeCallsToIntrinsic(), therefore special-casing just the debug info intrinsics would be inconsistent.

In D35159#823211, @erik.pilkington wrote:

Use LLVM naming conventions for all the new stuff in this patch.

Should there also be an update to LangRef?

LGTM.

In D35159#809724, @erik.pilkington wrote:

Rebase. I don't think the issue of purging underscores from this file/libcxx should block this, if we want to discuss that cfe-dev would probably be the place. I agree that it would be nice to clang-format this file, would anyone have any problems with me committing that after this? I found the git-blame of this file to be pretty useless, almost every line just points to r184097.
Are there any other thoughts on this change?

In D34331#803452, @EricWF wrote:

In D34331#803105, @dexonsmith wrote:

In D34331#802747, @EricWF wrote:

@dexonsmith Mind if I hijack this and check in your changes to <functional> with my tests?

Not at all.

In D34331#802821, @EricWF wrote:

@dexonsmith I'm not sure it's sane to allow reentrant behavior. Could you explain why you think it is?

It didn't seem sane to me at first either, despite this being supported by std::unique_ptr and std::shared_ptr. I found our user fairly convincing, though:

• They had an underlying reference counted object, so only the destruction of the last copy of A nulled out the pointer (mimicked that with the bool cancel).
• They had a callback function intended to be called once, and dropping that function on cancellation (mimicked that with a global variable). The cancel operation nulled out a std::function, but destroying the objects captured in the lambda in that std::function also separately decided to perform a cancel, which should have been OK. The cancel function just contained callback = nullptr.

According to [reentrancy] it is implementation defined what STL functions are allowed to be recursively reentered. I'm not opposed to providing reentrancy where it's useful, but we would have to provide it well.
This is where I was running into problems while I was writing tests. There are so many different ways you can reenter std::function's special members that it makes it impossible for an implementation
to truly support reentrancy as the standard would require.

If you consider that every possible copy construction or destruction of a user type is a potential reentrancy point, the complexity of having well-defined reentrant behavior starts to become clear.
Any time a copy constructor or destructor is invoked you have a potential reentrancy point which, in order to provide well defined behavior, must also act as a sort of _sequence point_ where the side effects of the current call are visible to the reentrant callers (For example your patches use of operator=(nullptr_t)).

The methods fixed in this patch are seemingly improvements; It's clear that operator=(nullptr) can safely be made reentrant. On the other hand consider operator=(function const& rhs), which is specified as equivalent to function(rhs).swap(*this).
I posit swap is not the kind of thing that can easily be made reentrant, but that making std::function assignment reentrant in general clearly requires it.

If fixing this bug is sufficiently important I'm willing to accept that, but I don't think it improves the status quo; We may have fixed a specific users bug, but we haven't made these functions safely reentrant (in fact most of the special members are likely still full of reentrancy bugs).

+mclow.lists

This LGTM. Mehdi, do you have any other concerns?

In D34331#802747, @EricWF wrote:

@dexonsmith Mind if I hijack this and check in your changes to <functional> with my tests?

Assuming nothing in arena<>...Db actually changed (just moved), this LGTM if you split the move into a separate prep commit.

In D34578#801519, @hfinkel wrote:

In D34578#801357, @dexonsmith wrote:

Ping! Hal, you committed r283051. Do you have a problem with this?

It looks like you're just renaming __libcpp_isnan and friends to __libcpp_isnan_or_builtin` and similar. LGTM

Ping! Hal, you committed r283051. Do you have a problem with this?

LGTM.

Ping!

In D34574#791158, @rsmith wrote:

Should we also remove the recently-added availability attributes in libc++ too? If I'm using a recent libc++ and providing my own aligned allocation functions, we shouldn't reject attempts to call those aligned allocation functions.

In D34574#791124, @ahatanak wrote:

Duncan and I had a discussion on this.

We are thinking about adding a warning that tells users that aligned allocation /deallocation operators are being called but they are not defined in the library.

LGTM.

I followed the other availability macros in using "strict", but I'm not sure this was the right decision. The documentation seems to recommend not using "strict" (it says that weakly-linking is almost always a better API choice).

libc++ is one of the exceptions. Use strict.

In D34446#787934, @arphaman wrote:

In D34446#787263, @dexonsmith wrote:

Right now, sys::getProcessTriple returns the LLVM_HOST_TRIPLE, whose system version might not be the actual version of the system on which the compiler running.

Are we sure no one depends on that behaviour?

The primary users of this function are the JIT libraries, but so far I haven't found any checks for the OS version in the JIT code. And it looks like most of the other users of this function don't check the OS version.

Right now, sys::getProcessTriple returns the LLVM_HOST_TRIPLE, whose system version might not be the actual version of the system on which the compiler running.

We use the -Wc++NN-compat- prefix on all the other subwarnings of -Wc++NN-compat warnings

I think -Wc++1z-mangling is best. IMO, with C++1z in the name, it's already clear that this is about compatibility. I also don't think we need to get as specific as -Wc++1z-compat-exception-spec.

Fixed in r305661.

Committed in r305647.

LGTM.

This is the right idea, although it only covers macOS.

You mention that the lock-free deque gives a speedup. I agree that should be left for a follow-up, but what kind of speedup did it give?

LGTM.

The current patch seems to be missing context on Phab (-U999999). Please remember to add it on the next iteration.

In D33513#765223, @dexonsmith wrote:

In D33513#765222, @mehdi_amini wrote:

If there is no other path to exercise LazyLoading of Metadata, let's just create one! (add an option to llvm-dis for example).

I agree.

In D33513#765222, @mehdi_amini wrote:

If there is no other path to exercise LazyLoading of Metadata, let's just create one! (add an option to llvm-dis for example).

I'm probably a good person to review this, but it's been a while since I've thought this so I might need help to refresh my context. This use-tracking code is unfortunately rather subtle.

Actually, looking through the comments, it appears that everyone (eventually) agreed with the approach in the patch. I agree too. LGTM.

Tests LGTM.

In D30633#756347, @chandlerc wrote:

In D30633#756320, @dexonsmith wrote:

As an aside, it seems the logic around unreachable (with and without the patch) might pessimize code paths that lead to noreturn functions, such as posix_spawn(). Is that really what we want? Why?

Heh, I thought about bringing this up, but it really seems orthogonal and something I wanted to look at.

Yeah, generally, the unreachable logic I wrote eons ago was dead wrong and this is still wrong in the case of nice "run forever" functions. I'll either send a patch or file a bug about this.

The testcase updates look good to me, and demonstrate the patch is doing what's intended. It might be worth adding a couple of tests for multiple branch targets (switch statements), since I'm not convinced everything adds up to 1 right now.

In D32724#750074, @vsk wrote:

In D32724#749868, @dexonsmith wrote:

In D32724#747728, @aprantl wrote:

Is it the right solution to use the module hash for correctness, or should the mismatch of the serialized langopts trigger a module rebuild and the module hash is only there to tune the performance/disk size tradeoff?

I'm not sure if there is (or should be) a hard-and-fast rule, but certainly in this case changing the hash SGTM. Otherwise, users toggling back and forth between build configurations would have to rebuild the modules each time.

Great, it looks like changing the module hash is acceptable. However, I'm not sure whether it's OK to make sanitizer feature mismatches errors for explicit modules. @aprantl suggests checking for language option mismatches early on instead of just relying on the module hash, but @rsmith mentioned:

I would expect this [sanitizer features] to be permitted to differ between an explicit module build and its use. (Ideally we would apply the sanitization settings from the module build to the code generated for its inline functions in that case, but that can wait.)

In D32724#747728, @aprantl wrote:

Is it the right solution to use the module hash for correctness, or should the mismatch of the serialized langopts trigger a module rebuild and the module hash is only there to tune the performance/disk size tradeoff?

Breaking the ABI of that configuration SGTM.

Thanks for being flexible; committed in r302095. Maybe longer term we could add a CMake configuration for whether to include the tests, but naming it might be hard ;).

LGTM.

(I must have misclicked somewhere.)

This LGTM too.

This LGTM, and it's liable to bitrot if it hangs out here any longer. We can always iterate in tree if we find a better way to organize the markup and/or tests.

Upgrade LGTM. (I'm not planning to review the rest, but let me know if you need me to take a closer look.)

rsmith: I just noticed you still have a red "X" here (since Phab won't let me "close"). I think I addressed your comments, but let me know if you want me to revert until you have time to look.

Thanks for the reviews! Committed in r301593.

Fixed in r301592.

(Both changes SGTM, but I doubt you're looking for my advice on the CMake logic.)

Oh, and it would be nice to split out Preprocessor::diagnoseMissingHeaderInUmbrellaDir in a separate NFC commit ahead of time.

LGTM after a couple of changes inline.

Thanks Eli and Richard.