Overall looks good to me but please re-upload patch on top of some stable revision where failed test passes (it seems unrelated to your changes).

Fix clang-tidy style warning

Replace tab with spaces

In D90835#2808153, @lebedev.ri wrote:

This needs to explain why the existing functionality isn't sufficient - if the header is really not from this project,
then it should be included via -isystem, and that will naturally suppress all diagnostics from it.

Fix clang-tidy warning

@bruno and @dexonsmith thank you for the review!

Updated HeaderMapImpl::reverseLookupFilename according to the review comment

Rebase + try build on Windows again

Resolved comments

Fix forgotten comment

In D103142#2783665, @dexonsmith wrote:

Naively, this sounds like it could be a non-trivial tax on build times. But it looks like it's only called in Clang from Sema::diagnoseMissingImport, which only happens on error anyway.

Split changes into NFC https://reviews.llvm.org/D103229 and the rest

@alexfh, @njames93 and @thakis please take a look! I added all tests cases and put new logic behind a flag to make it as safe as possible.
Issue with diagnostics from macro expansion from third-party headers is the one of the biggest problem with deployment that we have and it cannot be properly fixed with wrappers around clang-tidy.

Added test for system like object macro

Fix linter

Fix clang-format issue

Fix wrong character

Put feature behind flag

I think there is no sense to invent another HeaderFilterRegex like option to control which headers should be excluded. HeaderFilterRegex should be good enough because if you would like to see warning from the header, most probably you can fix code in the header or at least add a suppression.

Check exact list of enabled checks

Rebase again

In D97563#2624680, @njames93 wrote:

Is DAG required because the header file warnings are printed in a different order depending on things like platform?

Rebase

@aaron.ballman @alexfh @njames93 - friendly ping, please take a look!

Use -DAG checks to make test stable

@njames93 thank you for quick response and good suggestion!

Comments resolved

@shixiao do you have plans to keep working on this diff? If not, do you have any objections if I send for review a similar diff that will eliminate the check Sources.isInMainFile and will rely on generic header filtration mechanism in clang-tidy?

I investigated this issue a bit under debugger. It seems that there are multiple places where resolver is used as "base" object for ifunc and resolver attributes like comdat are applied to ifunc itself. It is reasonable for aliases but not for ifunc. Unfortunately it is not something that can be changed easily. The proposal with deriving GlobalIFunc from GlobalObject makes more sense to me now.

Oh, it seems that LTO is already relay on getBaseObject from ifunc :(

I think it should fix immediate problem that we have and makes things more consistent in general.

@njames93 thank you for the feedback!

• addressed comments in the diff
• stop ignoring system macro

Fix clang-tidy warning

Sorry, I don't think it's worth reverting and resubmitting the same changes in such a case. Commit attribution is correct and there is link to this code review.
After all, most clang-tidy users will never read commit messages and will only use --help for discovering this feature.

Hello @@DmitryPolukhin ,

When I submitted latest via 'arc diff' my commit-message was...
Can you please help to have following commit message? Below commit message is more clear and helpful.
Sorry for inconvenience caused!

In D89936#2370163, @Hiralo wrote:

When this patch will be merged and available in master?

Looks good to me.

In D89936#2361580, @njames93 wrote:

It's also be nice if --config-file would also support being passed a directory. If a directory was passed it would append ".clang-tidy" to the path and load that file, WDYT?

I think this diff looks very close to what we need. I hope it will be the last iteration.

Yes, it is what I meant as a simpler solution. But please add Lit test even for such trivial option.

@Hiralo, it looks like you uploaded wrong diff because your previous revision is shown as "base" version. Base revision should be clang-tidy sources without your changes.

I'm not sure that we need additional option to read configuration from file but, if we do need, I think this diff needs some improvements + test for new option.

In D89886#2346851, @alexfh wrote:

Ah, btw, any chance of adding a test for this?

Added all module map file names

And one more time linting

Linting diff

Thank you for identifying and fixing!

@aaron.ballman - thank you for the review!

I don't understand what do you mean by "not idempotent" behavior in this case. As far as I can see GlobalIFunc doesn't implement own getBaseObject (and it is not virtual) so calling getBaseObject on the IFunc should return null same as calling it on Alias-to-IFunc. Calling getbaseObject on Alias-to-IFunc will recursively call it on IFunc that will return null that will be propagated, isn't it? So in my opinion computeAliasSummary should handle null without crash because other places have checks for null returned from getBaseObject.

I did a bit of archeology and it turns out that getBaseObejct was part of moved from GlobalAlias to GlobalIndirectSymbol in https://github.com/llvm/llvm-project/commit/95549497ec8b5269f0439f12859537b7371b7c90
It looks like the simplest solution is to handle nullptr from getBaseObejct in computeAliasSummary...

getBaseObject is only small part of code sharing, the majority of the code is outside the class in common code that handles both GlobalIFunc and GlobalAliases in the same way. Here they should be treated differently so, IMHO, there is no need in changing the inheritance but GlobalIFunc should be handles as a special case here.

@njames93 and @aaron.ballman - please take a look to this diff. Multiline replacements in YAML are broken and cannot be applied correctly.

In D81911#2111204, @nextsilicon-itay-bookstein wrote:

Doesn't that mean that GlobalIFunc shouldn't really inherit from GlobalIndirectSymbol (and then GlobalIndirectSymbol loses the justification for its existence)?
I get the feeling like there's an expectation for getBaseObject to be idempotent (since it pierces through ConstantExprs and GlobalAliases), but GlobalIndirectSymbol::getBaseObject takes the resolver (getOperand(0)) and performs the lookup on it.

Fix test