Thanks for your work, I am glad that finally the Window bug is fixed with a simple solution.

Looks quite straight-forward other than that new prototype.

Nice and precise work! And special thanks for the unit tests. I've found some minor nits.

In D92797#2452497, @compnerd wrote:

Ping

In D92634#2453822, @OikawaKirie wrote:

I think it could be better to implement this check with a checker on PreStmt<BinaryOperator> and so on. And IMO, checkers have enough functionalities to report these problems.

Besides, the return value should be the exact value computed from the two integers, even unknown, rather than undefined. As the developers may overflow an integer on purpose.

In D92634#2451646, @danielmarjamaki wrote:

No reviews => I will not contribute.

Shafik, thanks for the review!

Thanks for the update. I checked it, still looks good to me.

• Check the attribute in the test as well

I wouldn't worry much about the ASTMerge tests, those are mostly legacy and not actively used nowadays. (At one point we were thinking even to remove them because we could got rid of the ASTMergeAction which is exclusively used for testing.)
The main test infrastructure for the ASTImporter is in the unittests. Those tests are exercised even with -fdelayed-template-parsing and with -fms-compatibility. So I am fine with marking the ASTMerge tests with XFAIL on windows.

LGTM!

• Remove not relevant param from test

However, every single TU in LLDB starts being pretty much empty (we do have some dummy declarations in them, but none of them have any name conflicts with declarations in any other AST we import from). So technically the 'empty' LookupTable is accurate.

In D92848#2442609, @martong wrote:

In D92848#2440205, @teemperor wrote:

So the only downside I can see is that the ASTImporter is no longer considering non-imported declarations in the To context (which are not known to the SharedState) for merging?

This is the case if you create the SharedState without a lookup table. If you pass a TUDecl then a lookup table will be created and in the constructor of that we traverse the AST to initialize the lookup. And from that point we will find every existing Decl even if they were not imported. As it is now we'd still be using the old noload_lookup.

@martong I assume I can make a follow-up for removing all the ASTImporter code for situations without a shared state?

Yes, that'd would be super.
Other than that, if you could make sure that the ASTImporterLookupTable is initialized then noload_lookup would not be needed and we could simplify so much in clang::ASTImporter.

In D92848#2440205, @teemperor wrote:

So the only downside I can see is that the ASTImporter is no longer considering non-imported declarations in the To context (which are not known to the SharedState) for merging?

This is the case if you create the SharedState without a lookup table. If you pass a TUDecl then a lookup table will be created and in the constructor of that we traverse the AST to initialize the lookup. And from that point we will find every existing Decl even if they were not imported. As it is now we'd still be using the old noload_lookup.

Thanks for the review guys!

• Remove if (!DC || !LexicalDC)

Thanks for the review!

Ping.
@teemperor please take a look if you have some time. This is a really important patch which may influence some future directions regarding the ASTImporter.

• Use -1 for mmap too

In D92600#2433263, @martong wrote:

It might be worth to extend StmtComparer as well with this Expr. I mean, probably two selections exprs should be considered equal if their assoc exprs are equal too...

It might be worth to extend StmtComparer as well with this Expr. I mean, probably two selections exprs should be considered equal if their assoc exprs are equal too...

I was thinking about to create a separate ASTImporter implementation specifically for CTU and LLDB could have it's own implementation. For that we just need to create an interface class with virtual functions.

• Remove comments
• Hoist Range(-1,0) to ReturnsZeroOrMinusOne

Looks good, thanks!

Looks good, thanks!

Hey Raphael, thanks for looking into the CTU crash!

In D92101#2423685, @shafik wrote:

Should we add a test for the regular function template case as well?

@rsmith I have moved the check into TransformTypedefType. We do not add the typedef to the MaterializedTypedefs list if it's context is dependent because later we set the deduction guide as the DC for all typedefs in the list:

for (auto *TD : MaterializedTypedefs)
  TD->setDeclContext(Guide);

• Add to MaterializedTypedefs only when copying the Decl

• Move dependent context check into TransformTypedefType
• Add new test case for param with pointer type

In D92101#2423685, @shafik wrote:

Should we add a test for the regular function template case as well?

• Check for dependent DeclContext
• Remove "dump()" calls from tests

Looks good, thanks!

In D92307#2422468, @vsavchenko wrote:

That's a good fix!
How did this happen though that max value of off_t was even used for fd. Seems pretty odd!

• Update test to match the copy deduction guide

Thanks for the review! Your comment made me think about whether we handle the deduced template class properly (getDeducedTemplate). Then I realized we do import that when we import the DeclarationName. Anyway I added a check for that in the test.

• Add test for user-defined ctad
• Handle IsCopyDeductionCandidate
• Fix typo

Looks good, thanks!

This looks good as well, thanks!

LG! Thanks!

LGTM! And thanks for the pro unittest!

Thanks, looks good to me!

We are trying to write checkers to catch possibly zero state.

IMO, that should be handled with taint analysis, i.e. when the value's provenance is untrusted we should warn. I don't see any other cases when we'd like to warn about a possible 0 denominator because that would cause false positives.

Looks good to me now! Thanks for addressing my concerns.

In D88859#2359399, @compnerd wrote:

I'd like to have maximum flexibility from the submitter by willing to change the details of the implementation

I don't think that is a fair expectation - there are plenty of times where this is technical disagreement on functionality, and it doesn't get immediately resolved. A high quality implementation doesn't require that everything be changed immediately, nor does it mean that everything must be completely flexible. There are trade-offs, and the goal is to strike a balance between simplicity and the needs of the project.

FYI, I am working on repeating and reevaluating Peter's measurements with the current master. And if possible then I'd like to extend the measures for new C and C++ projects. I am planning to share the statistics and run-time results.

In D88859#2354377, @gribozavr2 wrote:

I am having a hard time to accept "this is how it is implemented in our fork" as a technical argument. Besides, I am not sure how could the Clang community benefit about being backward compatible with a specialized fork and thus making superfluous complications.

Being compatible with Apple's SDKs is quite important for some part of the community.

This is similar to Clang implementing GCC and MSVC attributes, for example. Some might call them specialized attributes -- they are non-standard and are certainly not used by every C++ user -- but there exists a sizeable population of downstream consumers that rely on them. Apple's SDKs are in the same class.

Fair enough. But I don't think that Clang developers just copied the implementation from GCC or from MSVC.
I accept to keep the interface of the feature (i.e. the YAML format) as it is in the fork, okay. But, I'd like to have maximum flexibility from the submitter by willing to change the details of the implementation. I don't think that the implementation in the fork is necessarily the best realization, we can do better, after all that's the purpose of this review, isn't it?

Now I'm going to make unittests for SValBuilder::evalCast. Let me ask you to offer me some examples of casts to check. As many cases we can collect as robust the test would be. E.g.

In D89638#2341923, @vsavchenko wrote:

In D89638#2341855, @martong wrote:

I was thinking about an alternative approach (5th approach (?), yeah, I think we need an RFC).

What if we had just one attribute with a StringArgument ?
Actually, we already have that with the annotate attribute.
How do you like this?

struct [[clang::annotate("CSA:supress:RefCntblBaseVirtualDtor")]] Derived2
    : RefCntblBase{};

Disadvantages: we must process strings whenever a node has the annotate attr attached, we have to come up with a "DSL".
Advantages: total flexibility.
WDYT?

Honestly, I think that "CSA:supress:RefCntblBaseVirtualDtor" is way too verbose. It is incredibly easy to make mistakes, it seems hard to use and it is hell a lot to put in the actual code. It also doesn't resolve the issue with what unique identifier we should use. Imagine we decide that we should use Bug type as that unique identifier. It means that "CSA:supress:RefCntblBaseVirtualDtor" should get changed, but it is already in the user's code! It is quite important to settle on a solution that will still be available when we land a more general solution.

I was thinking about an alternative approach (5th approach (?), yeah, I think we need an RFC).

Ok, thanks for the context. LG!

Unfortunately, I could not reproduce the mentioned crash on our macOS machine. The mentioned test just passed with the output below. I gave up.

myuser@msmarple ~/llvm3/build/release_assert $ /usr/local/Frameworks/Python.framework/Versions/3.8/bin/python3.8 /Users/myuser/llvm3/git/llvm-project/lldb/test/API/dotest.py -S nm -u CXXFLAGS -u CFLAGS --codesign-identity - --env LLVM_LIBS_DIR=/Users/myuser/llvm3/build/release_assert/./lib --arch x86_64 --build-dir /Users/myuser/llvm3/build/release_assert/lldb-test-build.noindex -s /Users/myuser/llvm3/build/release_assert/lldb-test-traces --lldb-module-cache-dir /Users/myuser/llvm3/build/release_assert/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /Users/myuser/llvm3/build/release_assert/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /Users/myuser/llvm3/build/release_assert/./bin/lldb --compiler /Users/myuser/llvm3/build/release_assert/./bin/clang --dsymutil /Users/myuser/llvm3/build/release_assert/./bin/dsymutil --filecheck /Users/myuser/llvm3/build/release_assert/./bin/FileCheck --yaml2obj /Users/myuser/llvm3/build/release_assert/./bin/yaml2obj --server /Users/myuser/llvm3/build/release_assert/./bin/debugserver --lldb-libs-dir /Users/myuser/llvm3/build/release_assert/./lib /Users/myuser/llvm3/git/llvm-project/lldb/test/API/commands/expression/import_builtin_fileid/ -p TestImportBuiltinFileID.py -t
lldb version 12.0.99 (https://github.com/llvm/llvm-project.git revision d454328ea88562a6ec6260529a040035ab9c4a06)
  clang revision d454328ea88562a6ec6260529a040035ab9c4a06
  llvm revision d454328ea88562a6ec6260529a040035ab9c4a06
libstdcxx tests will not be run because: Don't know how to build with libstdcxx on macosx
Skipping following debug info categories: ['dwo']

Adding @NoQ as a reviewer, he might have some valuable comments, as he is very keen on this feature.

as it is something which is actually in production already
...
At the very least we need it for compatibility - this is already a shipping feature.

What is the context here? Did it cause any crash/bug or were you just reading through the code for a good night sleep? :D

Seems like this patch handles already existing attributes in Clang, so this might not be affected by the concerns brought up here by James:
http://lists.llvm.org/pipermail/cfe-dev/2020-October/066996.html