Most of these clang-tidy errors are from a mismatch between gRPC naming conventions and ours, I'm going to keep it as the gRPC ones because that's what we do elsewhere unless there are any objections.

Updates.

In D95314#2521637, @JonChesterfield wrote:

I don't think any of this is thread safe, which is bad as the plugin can definitely be called from multiple threads on the host.

Yep, it is not designed for that and I added a warning in the documentation.

Updates.

Removed redundant third function register_requires, it may become necessary later but it isn't now.

Update CommandLineReference.rst to also include -fno-finite-loops

while(1) case was mishandled for C++11 onwards, fixed now.

LGTM.

I'm happy to add a patch amending this, the reason it wasn't done that way was because at the time and even now, out of icc/clang/msvc/gcc, gcc seems to be the only one that happily removed such loops in C++ (https://godbolt.org/z/W9vj99), and I didn't have a particularly strong opinion on which way we should lean at the time.

In D86844#2484568, @fhahn wrote:

In D86844#2481922, @xbolva00 wrote:

int a, b;

int f(void) {
    while (1) {
        if (a != b) return 1;
    }
    return 0;
}

int g(int a, int b) {
    while (1) {
        if (a != b) return 1;
    }
    return 0;
}

LLVM does not catch these cases; gcc does.

https://godbolt.org/z/jW7son

Looks like must progress does not get added? If it gets added to the IR the loops get removed: https://godbolt.org/z/77v17P

Better?

In D86844#2475552, @xbolva00 wrote:

Do you plan to implement gcc’s option in Clang as followup?

Ah I see, I was never able to find clear documentation on what exactly the -cc1 flag does other than the conceptual description. This should fix it right?

I can't say that I know with any certainty, I'm too inexperienced. This failure was missed by me locally, the pre-merge bot, and by most of the buildbots other than the ones I mentioned above. I have no idea under what configuration of CMake options will clang not emit this particular case of a label for a BB.

Yep, sorry, I'm just waiting for a final stamp of approval from one of the reviewers.

As expected, @fhahn was right, adding willreturn to all those tests was an artifact from previous revisions of this patch and it passed the tests so I didn't pay them any mind, but I removed them now.

Addressed comments

More fixes.

Fixes.

Updated commit message, sanity ping for pre-merge buildbot.

Added debug messages for when the new calls fail.

Addresses inline comments (partially, fixing).

I believe this happened becase when I removed the loop, I did not update MemorySSA. The exact error was from GVN, but this update seems to fix the stage 2 build compile time error locally (I checked by running the build bot script).

This introduced a compile-time error that showed up during a stage 2 build.

I'm so sorry, I saw that you fixed it, thank you.

I naively assumed that reverting it would be fine, I'm working on a fixing right now.

final fixes.

• Fixed failing test

Rebase

removed non-parent revision.

using buildkite recommended fix

• Addressed nits
• Rebase
• Final Pre-commit testing

Hopefully the unrelated hwasan test failure is now fixed on master, trying again.

try 2.

ignore, testing pre-build bots again.

Rebase.

• Added triple to fix mangling errors in test
• Modified (unrelated) recently added test to have the mustprogress attribute

• Updated title, revision summary, and commit message
• Added documentation for struct and members

Thank you for the comments. I think I follow, I'll post another less intrusive patch soon.

Update comments.

• Adds test.
• Uses getExitBlocks() instead of getUniqueExitBlocks() and moved definition.
• Changed definition of hasNoExitBlocks() to use empty() instead of conditional
• Style changes in deleteDeadLoop for readibility.

Added word back in.

Add parent revision for buildkite.

ping. @jyknight @aaron.ballman @rjmccall any more thoughts?

rebase.

Sorry, I understand now. Is this more correct?

fixed splice.

Reverted to prior diff.

I think that phrase ("w/o maxobjsize deduction") could be a bit misleading, IINM, when @jdoerfert says w/o maxobjsize deduction he means without having the functionality built into the attributor to update the maximum object size estimate. So in the patch D87975, there's a crude over-approximation for the maxobjsize (getPointerMaxObjSize) and the numbers are for just using that over-approximation without including the changes in D87978 that refine it, so we would still need the maxobjsize attribute first in order to do this.

Fixed nit.