In D85319#2197104, @rjmccall wrote:

This seems a huge architectural change that we need to talk about.

In D85319#2196648, @whisperity wrote:

What will happen with the ability to analyse a translation unit whose target was not part of LLVM_TARGETS_TO_BUILD of the current clang binary? Will it break, because the binary lacks the information on how to generate for the given target?

• Remove dummy test file

LGTM! Thanks for the test!

AFAIK, BuiltinBug is deprecated and we should be using BugType anyway.
LGTM!

In D83174#2158275, @aaron.ballman wrote:

In D83174#2156454, @v.g.vassilev wrote:

Also, I would like to add that the current test present in this diff does not fail in the existing system but it was the best I and Vassil could come up with to replicate our problem.

Is there a way to run something like creduce to come up with a test case that does replicate the issue? I'd like to ensure we have a test case that fails without the patch applied so that we can be sure we don't regress the behavior.

Not easily. That bug comes from internal infrastructure which uses clang's API and happens when calling directly the mangler. Running creduce would require failure to be reproducible with bare clang. I was hoping @rsmith had something up his sleeve.

Drat. While the code looks reasonable, this makes me less confident in the fix. I'm adding some more reviewers who have touched the AST serialization and modules before in case they also have ideas for test cases.

• Use Twine

In D79431#2020951, @Szelethus wrote:

Sure, this is an improvement because we display more information, but I'd argue that it isn't really a more readable warning message :) How about

<argno>th argument to the call to <function name>

• cannot be represented with a character
• is a null pointer
• ...

, which violates the function's preconditions.

WDYT?

• Rebase to master

Unlike RecordDecl case which uses DefinitionCompleter to force completeDefinition we don't seem to have a similar mechanism for ObjCInterfaceDecl.

It is unfortunate that the AST does not have something similar for ObjC interfaces.
So, this seems to be the right way unless we want to modify DeclObjC.h.
LGTM!

Ok, LGTM! Thanks!

Ping :)

Thanks!

• Make Signature to be an Optional member of the Summary

• Reuse summaries with accept and other functions when sockaddr is a union of pointers

• Remove extra whitespace

In D83407#2141075, @balazske wrote:

Are not some functions missing from the list (shutdown, sockatmark, socket) (these come from <sys/socket.h>)? And getnameinfo comes from <netdb.h> with many other functions that are not added.

• Remove redundant line from test code

• Add getRestrictTy

Ping :)

Thanks, looks good!

Abandoning for a better fix in D83009

In D83009#2127106, @vabridgers wrote:

I cherry picked this change and retried the case that was failing. This change addresses the issue I was seeing. Thanks.

In D82940#2125183, @riccibruno wrote:

I'm not certain I understand, but I'm removing my LG while we figure it out. Can you expound a bit further?

Take a look at the AST dump of a small modification of the reduced test case which doesn't trigger the assertion:

void should_not_crash_with_switch_in_lambda() {
  switch (1)
  default:;
  auto f = [] {
    switch (1)
    default:;
  };
}

Before the serialization of LambdaExpr for the LambdaExpr *E, we have E->getBody() == E->getCallOperator()->getBody().
After serialization this is not true anymore because we are writing and reading the body directly when visiting the LambdaExpr
(the captures have the same problem).

In case of the reproducer the problem is that we clear the switch ids in GetExternalDeclStmt. But we read the SwitchCase with the same ID twice from the same call of GetExternalDeclStmt (bold-italic in the below call stacks):

• 1)

clang::ASTReader::RecordSwitchCaseID (this=0x4d5710, SC=0x5022f0, ID=0) at ../../git/llvm-project/clang/lib/Serialization/ASTReader.cpp:9070
0x00007ffff0313097 in clang::ASTRecordReader::recordSwitchCaseID (this=0x7fffffff7ca8, SC=0x5022f0, ID=0) at ../../git/llvm-project/clang/include/clang/Serialization/ASTRecordReader.h:331
0x00007ffff0300ebc in clang::ASTStmtReader::VisitSwitchCase (this=0x7fffffff7c98, S=0x5022f0) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:165
0x00007ffff0300f3f in clang::ASTStmtReader::VisitCaseStmt (this=0x7fffffff7c98, S=0x5022f0) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:171
0x00007ffff031e768 in clang::StmtVisitorBase<std::add_pointer, clang::ASTStmtReader, void>::Visit (this=0x7fffffff7c98, S=0x5022f0) at tools/clang/include/clang/AST/StmtNodes.inc:561
0x00007ffff0312b7c in clang::ASTReader::ReadStmtFromStream (this=0x4d5710, F=...) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:3896
0x00007ffff030eb2e in clang::ASTReader::ReadStmt (this=0x4d5710, F=...) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:2696
0x00007ffff0312e2d in clang::ASTReader::ReadExpr (this=0x4d5710, F=...) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:2705
0x00007ffff01cbb10 in clang::ASTRecordReader::readExpr (this=0x7fffffff8450) at ../../git/llvm-project/clang/include/clang/Serialization/ASTRecordReader.h:131
0x00007ffff0293b23 in clang::ASTDeclReader::VisitVarDeclImpl (this=0x7fffffff8408, VD=0x5010d0) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:1419
0x00007ffff02c40ad in clang::ASTDeclReader::VisitVarDecl (this=0x7fffffff8408, VD=0x5010d0) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:371
0x00007ffff02be24c in clang::declvisitor::Base<std::add_pointer, clang::ASTDeclReader, void>::Visit (this=0x7fffffff8408, D=0x5010d0) at tools/clang/include/clang/AST/DeclNodes.inc:453
0x00007ffff028e5ca in clang::ASTDeclReader::Visit (this=0x7fffffff8408, D=0x5010d0) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:519
0x00007ffff02b938f in clang::ASTReader::ReadDeclRecord (this=0x4d5710, ID=29) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:4044
0x00007ffff018c714 in clang::ASTReader::GetDecl (this=0x4d5710, ID=29) at ../../git/llvm-project/clang/lib/Serialization/ASTReader.cpp:7422
0x00007ffff021f9fa in clang::ASTReader::ReadDecl (this=0x4d5710, F=..., R=llvm::SmallVector of Size 3, Capacity 64 = {...}, I=@0x7fffffff9830: 3) at ../../git/llvm-project/clang/include/clang/Serialization/ASTReader.h:1863
0x00007ffff021f9a6 in clang::ASTRecordReader::readDecl (this=0x7fffffff9818) at ../../git/llvm-project/clang/include/clang/Serialization/ASTRecordReader.h:192
0x00007ffff0313d88 in clang::ASTStmtReader::readDecl (this=0x7fffffff9808) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:92
0x00007ffff0301d6a in clang::ASTStmtReader::VisitDeclStmt (this=0x7fffffff9808, S=0x5010b0) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:340
0x00007ffff031e078 in clang::StmtVisitorBase<std::add_pointer, clang::ASTStmtReader, void>::Visit (this=0x7fffffff9808, S=0x5010b0) at tools/clang/include/clang/AST/StmtNodes.inc:97
0x00007ffff0312b7c in clang::ASTReader::ReadStmtFromStream (this=0x4d5710, F=...) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:3896
0x00007ffff019a07c in clang::ASTReader::GetExternalDeclStmt (this=0x4d5710, Offset=107033) at ../../git/llvm-project/clang/lib/Serialization/ASTReader.cpp:7476

Ping

This patch causes a regression (crash) during the deserialization of certain functions. Please see the details and review the supposed fix here: https://reviews.llvm.org/D82940

Ok, I checked and it seems the parent map context is used only by the AST matchers, and this is okay because it looks like there is not any storing or caching of the parent map objects (the type is never copied).

Thanks Vince, looks good to me!

We use NoEvalCall in all functions in this patch

I just now realized that it is actually not true, sorry about that. So, seems like fnmatch, strcasecmp, strncasecmp are EvalCallAsPure. I could skip them from this patch, if you guys consider this as dangerous and to bolster my previous comment.

Thanks for the review guys!
I understand there are concerns about evalCall, so I am trying to dissolve those concerns below. :)

• Add 'Hide' and 'InAlpha', move Off_tMax inside the brace

In D79425#2022245, @xazax.hun wrote:

I'd prefer to have this functionality committed together its first actual use with tests.

Okay, at the current state we add a summary individually for each name, that makes it easier to modify a summary for each function without effecting other functions.

At the current state we add a summary individually for each name, that makes it easier to modify a summary for each function without effecting other functions. I'll add this functionality in the future when a real use cases comes up, if that really comes up.

In D79432#2022207, @xazax.hun wrote:

I think testing summaries this way can be really hard to manage in the future.
I see two possible ways forward to make this easier:
a) Make something like https://reviews.llvm.org/D78118 that will also dump the actual summary in a textual form, not only the fact that a summary was loaded and check for that textual form.
b) Make a small helper library for testing summaries. E.g., most of the buffer handling functions have similar summaries, so we could have only a couple of functions for testing buffer related summaries and we could just pass in function pointers (as templates or params) instead of duplicating code. Similarly, testing output ranges could be generalized.

I think b) might be a bit better solution.
Although I do see why some people like to keep tests simple without additional layers of abstractions, so I do not insist :)

Since D80016 we are able to look up types in the TU, so we can get the maximum value from the found type.

Since D80016 we are able to look up types in the TU, so this patch is no longer needed. We'd like to be as precise with the function signatures as possible.

This patch is no longer needed because I am going to upstream the POSIX functions in groups, otherwise the patch would be hideously large.
The groups are: file handling, networking, pthread_, mq_, dbm_, regex related, sched_, time, signal, terminal related, ...

In D77658#2065174, @MaskRay wrote:

arc adds many unneeded tags from Phabricator. You can drop Reviewers: Subscribers: Tags: and the text Summary: with the following script:

arcfilter () {
  arc amend
  git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people (https://lists.llvm.org/pipermail/llvm-dev/2020-January/137889.html). You should keep the tag. (I started to use --date=now because some people find author date != committer date annoying. The committer date is usually what people care.))

In D81916#2095106, @NoQ wrote:

Thanks!

@martong, thoughts on this? :)

Also i guess in this test we're unable to obtain the value for EOF, what would be a good fixme-test to demonstrate that?

In D72705#2041779, @balazske wrote:

I can make comparisons if more functions are added to the checker, fputs only is not sufficient.

Thank you Kristof for working on this. Looks good to me as it is!

In D80016#2063234, @xazax.hun wrote:

A high level comment.

Trying to match function signatures is not a unique problem! In fact, almost all of the checks the analyzer have is trying to solve the very some problem.
One of the methods we have at this point is called CallDescription, see here: https://github.com/llvm/llvm-project/blob/master/clang/lib/StaticAnalyzer/Core/CallEvent.cpp#L358

Moreover, I would assume something similar needs to be done for APINotes.

Do you think it would be possible to extend the CallDescription interface to match your needs? In that case all of the checks could profit from this work.
What do you think?

• Remove SValBuilder param

Thanks! LGTM!

Just out of curiosity. In what way do you prepare to share these test? For which component are you planning to reuse this test infrastructure?

Thank you for working on this.

• Add comments, remove auto from 'for' loops, matches -> matchesAndSet

LGTM!

• Rebase to D77066

• Rename: validate -> checkValidity, sanityCheck -> checkSpecificValidity
• Rename: Summary::checkValidity -> validateByConstraints
• Add more docs to the Summary class
• Rename matchesSignature -> matches

• Rebase to master

• Add if (FileTy)

• Rebase to master

In D80366#2059765, @martong wrote:

In this version of the patch you are using handleConstructionContext to "retrieve" or read an SVal for the construction. However, it seems like that function was designed to store values in the GDM for individual construction context items. To mix a read-only functionality into a setter seems to be error-prone to me.

Please instead re-use the code that computes the object under construction. That'll save you ~50 lines of code and will be more future-proof

I am not sure that this is the best option here. In my viewpoint, ConstructionContext and ConstructionContextItem are both "variants" (i.e. tagged unions) and we perform operations on these. Each operation is actually a visitation on the different kind of values (i.e. a switch). One operation is transforming a ConstructionContext to a ConstructionContextItem. This is what is needed to be able to call the getObjectUnderConstruction function. And this operation - the reading - is way different than the other - the store -, see Adam's concerns. This yields to different functions for each op. But this is perfectly natural once we work with "variants". I mean we can easily extend the operations on a variant with a new operation as a form of a visitation of the kinds (contrary to traditional class hierarchies where adding a new op is hard, but adding a new type is easy).

In this version of the patch you are using handleConstructionContext to "retrieve" or read an SVal for the construction. However, it seems like that function was designed to store values in the GDM for individual construction context items. To mix a read-only functionality into a setter seems to be error-prone to me.