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

In D81911#2098301, @tejohnson wrote:

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

I added a test case that exercises the crash before the fix.
However, your comment made me realize that there are two separate issues + one question here:

1. getBaseObject() is currently unable to indirect through GlobalIFuncs (as the current fix tries to address).
2. computeAliasSummary() assumes that getBaseObject() can never return nullptr, while it has potential flows that return nullptr. In Verifier::visitGlobalAlias() I see that aliases-to-ConstantExpr-s are valid, but in Verifier::visitAliaseeSubExpr I see that alias cycles are invalid. If we assume that the module for which we generate a summary is valid, this question amounts to whether it is valid for the ConstantExpr case of an alias to ever return nullptr there. I can add an assert(Aliasee) / fatal(..) in computeAliasSummary.
3. Is it valid for a GlobalIFunc to have an alias as its resolver function operand? It sounds contrived, but valid nonetheless (e.g. if the resolver is in a different translation unit and they end up getting merged in LTO?). A GlobalIFunc with a resolver that is itself a GlobalIFunc sounds like an invalid construction to me, though. I don't see references to GlobalIFunc in Verifier.cpp to use as guiding principles for what would be considered valid. So, it sounds to me that with respect to module validity, in an alias-...-alias-ifunc-alias-...-resolverfunc chain there should only ever be a single ifunc at most.

Regardless, changing the findBaseObject implementation to take a set of GlobalIndirectSymbols and uniformizing that handling sounds like a fine alternative to the currently proposed fix, depending (?) on the points I raised above.

My knowledge of IFunc details is pretty thin. Adding @DmitryPolukhin who ported over ifunc support from gcc and might be able to provide some guidance. I agree it makes sense to have the use in computeAliasSummary check for the null with an assert. And if the usage you describe in 3 is illegal, it would be good to have the verifier check for this.

Use double quotation for multiline strings. It solves problems with internal newlines because now they are escaped. Also double quotation solves the problem with leading whitespace after newline. In case of single quotation YAML parsers should remove leading whitespace according to specification. In case of double quotation these leading are internal space and they are preserved. There is no way to instruct YAML parsers to preserve leading whitespaces after newline so double quotation is the only viable option that solves all problems at once.

It looks like there is no support for the proposed solution so I found alternative solution that might be even better. We can use double quotation " for multiline strings. It solves problem because in case of double quotation LLVM escapes new line like \n so there is no need to double newlines. Escaped newlines can be parsed correctly by other YAML parsers like pyyaml. BTW, LLVM YAML reading also has issue with not removing leading spaces for multiline strings so multiline strings serialised by pyyaml with single quotation cannot be parsed correctly by clang-apply-replacements. With double quotation it seems to work fine in both directions.

@njames93 - friendly ping, could you please take another look.

Rebase

LGTM

In D75184#2093089, @njames93 wrote:

As an afterthought, Would you think it a good idea to extend this logic to the ConfigOptionsProvider That way you can use something along the lines of:

clang-tidy --config='{InheritParentConfig: true, CheckOptions: [<Options overriding/complimenting .clang-tidy files>]}'

clang-tidy would then go to read the .clang-tidy configuration files in much the same was as is done here. This would let you set custom configuration options on the command line on top of whats in configuration files.
Obviously is InheritParentConfig is unset or false then we wouldn't go looking for .clang-tidy files.

@aaron.ballman thank you for review and quick response!

@alexfh, @gribozavr2, @aaron.ballman - friendly ping, please take a look to this diff. It is just trivial implementation of TODO in the code.

Rebase

Fix single new line handling, it should be replace with space

Fix spelling

Rewrite input string split too

Apply suggested changes with string split

+ @gribozavr, @Eugene.Zelenko, @thegameg who touched/reviewed this code, please take a look.

Fixed second string after new line + more test case

Fix clang-tidy warnings

Sent D80301 for review.

@mgehre @yvvan it seems that the issue still not fixed and '\n' gets duplicated in the replacements. Are you going to fix this issue or I should create a patch to fix it?
Before this change '\n' was actually processed correctly ReplacementText: '#include <utility>\n\n' is actually replacement text with two new lines in standard YAML deserialiser.

@alexfh and @gribozavr2 friendly ping could you please take a look

Comments resolved

Split TypeOffsets to low/high parts too

In D76594#1986846, @martong wrote:

I am not sure, but maybe this patch causes an undefined behavior?
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/40472/steps/check-clang%20ubsan/logs/stdio

/b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Serialization/ASTReader.cpp:6327:28: runtime error: load of misaligned address 0x7feac40a55bc for type 'const uint64_t' (aka 'const unsigned long'), which requires 8 byte alignment
0x7feac40a55bc: note: pointer points here
  00 00 00 00 a3 c7 01 00  00 00 00 00 0c c8 01 00  00 00 00 00 29 c8 01 00  00 00 00 00 7a cc 01 00
              ^ 
    #0 0x3be2fe4 in clang::ASTReader::TypeCursorForIndex(unsigned int) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Serialization/ASTReader.cpp:6327:28
    #1 0x3be30a0 in clang::ASTReader::readTypeRecord(unsigned int) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Serialization/ASTReader.cpp:6348:24
    #2 0x3bd3d4a in clang::ASTReader::GetType(unsigned int) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Serialization/ASTReader.cpp:6985:26
    ...

(I am in the blamelist too of that build, so that's why I am sniffing.)

@thakis, I don't see this bot on LLVM http://lab.llvm.org:8011/console
Windows bots there still fail due to cmake issues. The issue is very real and thank you for pointing out but how should I find the bot?

@thakis sorry, I didn't notice the issue because of massive breakage with diff landed just be bore mine and also cmake issues on Windows bots. Thank you for trying to fix it.

@sammccall thank you for review!
I'll wait for one more day for additional feedback if any, and land this diff.

Fix nit

Split BitOffset in DeclOffset in high/low parts; rebase

Split BitOffset in DeclOffset in high/low parts; rebase

And one more time :(

One more try to remove useless lint issues

Remove clang-format errors in diff

Comments resolved + rebase

@alexfh thank you for review!

@alexfh friend ping, please take a look.

@rsmith, @dexonsmith, @jdoerfert could you please take a look to this diff?
If you think that there are other reviewers who might have more context AST persistence, please add them.

@alexfh friendly ping
I implemented solution with priorities to resolve your concerns about get() vs getLocalOrGlobal()
Could you please take another look to this diff?

Use options priority instead of overriding local options by global