[Clang] Optimize -Wunsafe-buffer-usage.

[ivanaivanovska]

The Clang disgnostic -Wunsafe-buffer-usage was adding up to +15% compilation time when used. Profiling showed that most of the overhead comes from the use of ASTMatchers.

This change replaces the ASTMatcher infrastructure with simple matching functions and keeps the functionality unchanged. It reduces the overhead added by -Wunsafe-buffer-usage by 87.8%, leaving a negligible additional compilation time of 1.7% when the diagnostic is used.

Old version without -Wunsafe-buffer-usage:

$ hyperfine -i -w 1 --runs 5 '/tmp/old_clang -c -std=c++20 /tmp/preprocessed.cc -ferror-limit=20'
Benchmark 1: /tmp/old_clang -c -std=c++20 /tmp/preprocessed.cc -ferror-limit=20
  Time (mean ± σ):     231.035 s ±  3.210 s    [User: 229.134 s, System: 1.704 s]
  Range (min … max):   228.751 s … 236.682 s    5 runs

Old version with -Wunsafe-buffer-usage:

$ hyperfine -i -w 1 --runs 10 '/tmp/old_clang -c -std=c++20 /tmp/preprocessed.cc -ferror-limit=20 -Wunsafe-buffer-usage'
Benchmark 1: /tmp/old_clang -c -std=c++20 /tmp/preprocessed.cc -ferror-limit=20 -Wunsafe-buffer-usage
  Time (mean ± σ):     263.840 s ±  0.854 s    [User: 262.043 s, System: 1.575 s]
  Range (min … max):   262.442 s … 265.142 s    10 runs

New version with -Wunsafe-buffer-usage:

$ hyperfine -i -w 1 --runs 10 '/tmp/new_clang -c -std=c++20 /tmp/preprocessed.cc -ferror-limit=20 -Wunsafe-buffer-usage'
Benchmark 1: /tmp/new_clang -c -std=c++20 /tmp/preprocessed.cc -ferror-limit=20 -Wunsafe-buffer-usage
  Time (mean ± σ):     235.169 s ±  1.408 s    [User: 233.406 s, System: 1.561 s]
  Range (min … max):   232.221 s … 236.792 s    10 runs

[ilya-biryukov]

Thanks a lot for this!

I have left quite a few comments, mostly NITs.
A few things that really stood out and I think we should fix:

• increasing the number of macros and signatures we use seems like a bad trade-off. It's much better to have more parameters and ignore them in most cases, but only have a single macro to deal with as a return. The lower complexity of the code using the macros is really something we should optimize for.
• the code is using the ASTMatchers terminology despite switching away from it. I find it to be causing more confusion than good. We are really replacing matchers with AST traversal, so I left a few suggestions that go in the direction of switching from
• some code has been moved around, making it a little hard to diff between the original and updated version. I suspect it would not compile otherwise, but I am happy to help find workarounds to keep the code compiling and keep every single function and class that reimplemented the original code at the same place. I think this should really help to ensure the code is equivalent and will make it easier to review.

[ilya-biryukov]

I only have a few suggestions left, overall this LG.

Could you also add a description for the PR that explains the motivation and approach taken for the rewrite?
Benchmark numbers would also be needed there.

Thanks!

[ilya-biryukov]

This generally looks good, just one more minor comment from me.
Could we put this out of the draft state and send to upstream owners of the check to start a discussion about landing it?

[ilya-biryukov]

Is it really necessary to make these members public?
They only seem to be used for reporting results internally, or am I missing something?

[ilya-biryukov]

FYI: this PR was taking too long to pick changes after rebase, so the discussions continued in #125492.