Another high-level note (besides adding tests): please resubmit the patch with full context (e.g., git diff -U9999999 HEAD^..).

In D30882#1075589, @dexonsmith wrote:

In D30882#1075576, @pete wrote:

Would it be ok to turn this on by default, without a flag, only in the case of the path actually existing, and only the found path being the one we add to the .d?

I think that pessimizes some incremental builds:

• You have a __has_include("missing.h"), but don't include missing.h.
• Change "missing.h" (but don't delete it).
• An incremental build now has to rebuild the object file, even though nothing will have changed.
  
  However, it's fixing an actual bug, so it makes sense to me to be more conservative.

In D30882#1075576, @pete wrote:

Oh, that actually wasn't my intention when I wrote it.

Honestly I didn't expect it to log anything for missing paths at all, as we don't currently log all the missing (but attempted) paths for regular #include's.

In D30882#1075407, @ddunbar wrote:

In D30882#1074822, @dexonsmith wrote:

I don't think this is quite right. I know at least make-based incremental builds wouldn't deal well with this.

This is actually not a novel problem w.r.t. this patch. The exact same situation comes up with Makefile-included .d files and when one of the referenced headers is removed.

This is typically solved somewhere in the build system, for example Make has -include, and Ninja and llbuild have explicit support for this situation.

That doesn’t seem like the right place for the newline; instead, it should be wherever the asterisks were printed.

I don't think this is quite right. I know at least make-based incremental builds wouldn't deal well with this.

Thanks for working on this! I've just done a pass through for style, and I have a few nits to pick.

Hal requested splitting the patch in two, since the two features are separable, but they both still seem to be here. Perhaps start with the prefix patch?

I'd prefer to have this be as close as possible to the libcxxabi demangler (and use the same data structures) so that it's easy to detect when they diverge. If we ever move to monorepo hopefully there will be a way to factor them out into a single implementation.

Have you confirmed this doesn't need to depend on the deployment target? I.e., do we need the old logic sometimes when back-deploying?

LGTM.

LGTM.

LGTM.

I agree with Saleem and Bob: __is_target_* is not confusing here and seems to be a straightforward spelling. It has also already shipped in LLVM 6.0.0: it would be awkward to stop supporting this syntax.

LGTM.

LGTM!

If it's possible to separate the bugfix into a separate commit (either before or after), I think you should.

LGTM.

LGTM once you clean up the #if 0.

Thanks for working on this!

In D41695#966554, @sdesmalen wrote:

Not sure if this requires an RFC on llvm-dev, but please let me know if it does.

In D41050#954863, @rjmccall wrote:

Heh, alright, that works. It's unfortunate that -disable-llvm-passes doesn't suppress running the pass under -O0; that seems like an oversight.

Anyway, LGTM.

Akira and/or John should take a look instead of me.

This seems correct to me. I wouldn't mind having someone else back me up though.

Committed in r320208.

LGTM, after you try to shrink the testcase. Thanks for tracking this down!

In D40891#947361, @AndrewScheidecker wrote:

I ran the patch through clang-format. But looking at the code again, I see that calling the getEdgeProbability overload with a successor index or iterator instead of the destination basic block actually does something different from the original code: passing a successor index/iterator gets the probability for a single edge, while passing a basic block sums the probability for all edges between the two blocks.

However, and I have to admit that I'm in over my head here, what the original code was doing doesn't seem correct. For each edge from A to B, it was summing the probability of all edges from A to B, then adding the sum to the mass for B. If there are N edges from A to B, and they have probabilities that sum to M, then this will distribute N*M mass to B instead of M. In my contrived case, N=196928.

I still think this change is good, but I didn't mean for it to change the results. What are your thoughts?

In D39502#914667, @steven_wu wrote:

Also split out the testcase for UNIX conformance. That test is not related
to output file cleanup. Split it out to make it clear that is for UNIX
conformance.

I have a few nitpicks, but otherwise this LGTM. I'd like to wait for someone on the CUDA side to confirm though.

In D39245#908059, @davide wrote:

That said, my point that we shouldn't have yet another macro stands, I guess, independently from the bucket we decide to throw these checks in.

In D39245#907527, @davide wrote:

In D39245#907426, @mgrang wrote:

In D39245#907221, @chandlerc wrote:

I don't really like the ever growing set of configuration macros here.

I would much prefer we have one ABI-breaking check macro and just use that for everything here...

Thanks @chandlerc. I am fine with enabling this based on LLVM_ABI_BREAKING_CHECKS. However, this macro is ON by default for +asserts builds. This means all existing +asserts bots would start failing as soon as this patch gets merged since there are about 15 unit test failures. Do we want to gate patches if they show up sort order failures uncovered by shuffling?
My idea was to control this via a separate macro so that we can isolate shuffle+sort failures into a separate bot which may make it easier to fix without gating the regular flow. Thoughts?

I agree with Chandler that we should avoid the proliferation of macros and use ABI_BREAKING_CHECKS instead.

In D39245#905862, @mgrang wrote:

In D39245#905733, @dexonsmith wrote:

std::sort and array_pod_sort both use quicksort

FWIW, most standards-compliant std::sort implementations use introsort... although it's also unstable.

NOTE: To randomly shuffle before std::sort we may have to change all calls to std::sort with llvm::sort.

It looks to me like the current overloads of llvm::sort take a parallel execution policy, which would be unfortunate boilerplate to add. Perhaps you should create an overload in ADT/STLExtras.h with the same signature as std::sort (that wraps std::sort with the initial call to std::random_shuffle).

Sure, I can add overload for llvm::sort which takes ExecutionPolicy and invoke std::sort.

std::sort and array_pod_sort both use quicksort

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