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

Rebase, all tests passes locally

Resolve merge conflict, revert clang-format for ASTBitCodes.h to avoid further conflicts, use C++ cast

Use 64-bit offsets for TypesOffsests and DeclOffsests

In D76594#1937289, @sammccall wrote:

This looks reasonable to me, though I'm not familiar enough with the code to do a good review.

Just wanted to confirm my understanding: the problematic offsets that are now section-relative instead of file-relative are only used in two sections of the file (macro directives and sloc entries). These sections seem likely to be small relative to others that hold e.g. the AST. So this should raise the effective size limit by a lot, more likely 100x than 5?

clang-format for ASTBitCodes.h

Relative 32-bit offsets in https://reviews.llvm.org/D76594

In D75184#1932764, @alexfh wrote:

There's one more thing to consider: just by looking at the name of a local option we don't know whether it will be read using get() or getLocalOrGlobal(). By removing local options we may introduce an even more surprising behavior than without this special treatment. WDYT?

In D76295#1927041, @aganea wrote:

Hello Dmitry! Could we use varints instead of uint64_t if size on disk is a concern?

@alexfh could you please take another look to the diff?
All comments resolved.

Applied suggested changes

In D75184#1912650, @njames93 wrote:

How are local and global options handled.
From what I can gather it will read the value as 1 as the local option is checked first no matter which file it was defined in.

Handle global options

Also updated rst file

@alexfh could you please take another look, I added more tests and updated the doc.

Comments resolved, please take another look.

Friendly ping, please take a look.

Fix issue with config inheritance and caching configs

Rebase on top of master

@thakis I'm sorry sorry if it was not clear. Please let me know if you still prefer to have separate directory for clangTidyMain to have only one target per CMakeLists.txt.

Added #include "ClangTidyMain.h" in ClangTidyMain.cpp

Updated comment per review suggestion.