Hi @jfb @rnk This patch results in a compiler crash when building a simple C program on a Windows X86 Debug build. I have filed this issue to track it.

In D78853#2003866, @efriedma wrote:

Please don't add null checks for pointers that can't be null. It makes the code slower and harder to understand. And least one of the checks you added is actively breaking the code.

In some cases, the analysis is pointing to cases where the code could be made more clear for both humans and machines with some refactoring or assertions. Patches welcome, but please make sure any assertions properly explain the invariant. And please split the patches up a bit more; adding assertions for complex invariants in ten different unrelated places is more than I really want to review at once.

(Also, a reminder, please post patches with full context.)

Thanks for fixing this. LGTM.

LGTM.

Have you tried measuring compile-time on some benchmark with this patch. Changing unordered containers to ordered ones can increase compile-time. What if you simply sort the containers before iteration?

In D60349#1488411, @efriedma wrote:

LGTM, assuming it passes testing on electron

Thanks @richard.townsend.arm . I have updated the patch.

@rnk @efriedma Is this patch good to commit?

In D60348#1474684, @efriedma wrote:

Do you need to modify AArch64TargetLowering::isEligibleForTailCallOptimization to prevent a tail call in cases where the tail call would return the wrong value?

Thanks @kristof.beyls for the review. We are still working on the combining the clang patches which should fix PR41135 and PR41136. I will then add unit tests to this patch.

In D60349#1462638, @ostannard wrote:

The document you linked in the LLVM change (https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=vs-2019#return-values) says that small POD types are returned directly in X0 or X0 and X1, but this looks like it will always return them indirectly. I think we also need to check the size of the type, and fall back the the plain C ABI for small types.

The new patch now also handles struct returns for instance methods.

Got rid of the confusing SuppressSRet logic. The "inreg" attribute is now used to indicate sret returns in X0.

Refer https://bugs.llvm.org/show_bug.cgi?id=41135 for a detailed discussion on the issue. Please review this and the related clang patch and let me know if this implementation is something we can proceed with.

In D59279#1439014, @Szelethus wrote:

In D59279#1438514, @mgrang wrote:

Although, as you rightly pointed out that ordered sets of pointers are as non-deterministic as unordered ones.

What if you store pointers to the elements of an array? In that case, it should be deterministic.

-----------
|1|2|3|5|6|
-----------
 ^     ^ 
 p     q

In the above example, pointer p's memory address will always be less then q's, so in an ordered set, iterating over them would be deterministic. When hashing algorithms come into the picture, than it wouldn't be :^)

In D59279#1438500, @rnkovacs wrote:

In D59279#1427017, @mgrang wrote:

Following are the assumptions/limitations of this patch:

1. The assumption is that iteration of ordered containers of pointers is not non-deterministic.

Could you please explain which type of non-determinism we are addressing here? If our issue is that iteration order is not consistent across runs, then an unordered set of integers seems just as non-deterministic as an unordered set of pointers. On the other hand, if our issue is that pointer values vary between runs, then an ordered set of pointers seems just as non-deterministic as an unordered set of pointers. Are unordered sets of pointers distinguished because they lie in the intersection of these categories and thus avoid the most false positive cases? If so, for someone debugging non-deterministic behavior in their code, would it be useful to add a strict option that shows other cases too? If not, maybe we could document our reasons somewhere.

Ping for reviews please.

I was trying to write another checker for hashing of pointers. Basically, I wanted to match all instances where the keys of std::map are pointers. Writing an AST matcher for std::map is straightforward. However, I am not sure how to check for pointer keys after matching std::map.

Could you please add a commit that changes the top of the file on your previous commit? Feel free to do that without review (I can't commit it myself atm).

Also, what is the difference between these two: https://clang.llvm.org/docs/analyzer/checkers.html, https://clang-analyzer.llvm.org/available_checks.html. It seems they document similar stuff. Should we add the doc for each new checker in both of these?

The obvious question, why not implement this in clang-tidy?

Following are the assumptions/limitations of this patch: