git-clang-format

git-clang-format

Moved call annotateNonNullNoUndefBasedOnAccess()

Thanks for the information. I found the review now in https://reviews.llvm.org/D97512

This patch seems to introduce warnings for the case

I have now updated the testcase according to comments by @MaskRay in commit 073cdb239044
Thanks for post-commit review comments.

Updated patch according to review comments.

In D70527#1786718, @rnk wrote:

In D70527#1785552, @ikudrin wrote:

Personally, I would prefer to see the file name and path to be changed as little as possible because that would help to recognize the files better. We cannot use remove_dots() on POSIX OSes to simplify paths, because it may return an invalid path; thus we have to use getRealPath(). If I understand it right, there is no similar problem with the file name itself.

So, which issues this patch is going to solve?

It seems clear to me, the filename could be an absolute symlink to a real file somewhere far removed from the realpath of the parent directory. It seems reasonable that -fdiagnostics-absolute-paths would look through symlinks in this case.

Ping

Update testcases according to review comment.

Thanks!

What about a testcase? It shouldn't be hard to add a small testcase that demonstrate that the changed builtins now work when compiling OpenCL C code for x86. I don't think you have to add all changed builtins to the testcase (but a few to demonstrate the change).

Updated according to review comments

Added a new C++ testcase.
Removed the REQUIRES: avr-registered-target in the avr-builtins.c testcase.

Ping?

Do you think this small lit testcase below work as a testcase for the -ffile-macro-prefix-to-remove option?

I found another reproducer for the problem. This one is slightly different where a different assert blow, but I guess its only a different symptom for the same fault. As this one is slightly smaller you might want to use this one as a testcase instead (or use both)? Run with "opt -bounds-checking -S".

In D48650#1165966, @NoQ wrote:

This commit causes an assertion failure on void foo(int a, int *b) { (int)b < a; }

Run-line: clang -cc1 -analyze -analyzer-checker=core -analyzer-eagerly-assume test.c

So it kinda frustrates RangeConstraintManager with a SymSymExpr that expresses a comparison between a Loc and a NonLoc symbol.

I'll think a bit more on how do i want to address that.

Fixed another case when PHI nodes are not correctly updated.
This time it is when switch is replaced with a single branch.

The fix solves the problem I saw originally in a out of tree target (described in PR37745) but I don't know JumpThreading enough to say if the fix is good or not. Thanks for working on this.

In D47203#1120655, @arsenm wrote:

Shouldn't there be a removePredecessor call somewhere that takes care of this?

Ping?

Ping?

Updated testcases according to comments

In D46379#1118995, @spatel wrote:

In D46379#1118803, @Ka-Ka wrote:

Do you think I should still include the negative testcase that you specified above even if the patch now only affect the GEP case?

Yes - because the earlier revision would have caused a bug with no regression test coverage for it. Let's add that to be safe. Also, I'd prefer to use the utils/update_test_checks.py script on all tests because it makes maintenance easier. And as before, if you check in the baseline version of the test using trunk output, that's even better - we want to keep the tests in place even if the patch gets reverted for some reason.

In D46379#1118168, @spatel wrote:

This is a constant folding change, so the test should be visible with just "opt -constprop ..."

Ie, the test should be minimized and moved under test/Analysis/ConstantFolding.

Updated testcase according to comment.

Adding reviewers.

In D45686#1113426, @lebedev.ri wrote:

In D45686#1113425, @Ka-Ka wrote:

In D45686#1113414, @aaron.ballman wrote:

In D45686#1109295, @dstenb wrote:

Ping.

We have added a lit reproducer for this now in clang-tools-extra: https://reviews.llvm.org/D47251.

The above should be rolled into this patch as the test case verifying the behavioral changes.

I guess the intention from @dstenb was to submit both patches together. The reason for a separate patch with the testcase alone is that the reproducer use clang-tidy and as far as I know all clang-tidy tests exist in clang-tools-extra.

.. and both of them exist in the same svn repo, or git monorepo. (granted, not many use that)

In D45686#1113414, @aaron.ballman wrote:

In D45686#1109295, @dstenb wrote:

Ping.

We have added a lit reproducer for this now in clang-tools-extra: https://reviews.llvm.org/D47251.

The above should be rolled into this patch as the test case verifying the behavioral changes.

Added Jonas Devlieghere as reviewer as he was involved in the review of rL327851 (which seems to be the reason for this patch).

Reduced the size of the testcase.

Adding Matthew Simpson and Sanjay Patel as reviewers also as they have done work in ConstantFold recently.

The reason for me to use WeakTrackingVH instead of WeakVH from the beginning was only that it was used elsewhere in the file. However as you point out WeakVH should be fine as RAUW is not used in RecursivelyDeleteTriviallyDeadInstructions(). I adjust the code to use WeakVH.

Ping?

Ping!
Anyone up for reviewing this?

This definitely solve the problem I saw in the issue pr37369.

Changed testcase according what was suggested by lebedev.ri

Ping!
Anyone up for reviewing this?

Adding Matthias Braun and Krzysztof Parzyszek as reviewer as it seems you both have done work in BranchFolding.cpp before. Do you have time to review this change?

In D46048#1079135, @jonpa wrote:

Adding Uli as a reviewer for the test case, since it's SystemZ (he always reviews SystemZ changes). Note: as Kalle explained to me in a mail, this test case has been somewhat modified to expose the bug on SystemZ, but since this is a .mir test case which passes the machine verifier, I think this is not a problem.