The patch doesn't seem to affect the reports.
It only introduced 3 new reports during the analysis of clang + clang-tidy.
The analysis times indeed increased slightly, ~~ +3%.

Nothing changed besides moving some parts to the parent revision.

• registerForPreprocessor member function
• Optional<StringRef> return type
• NoneForNonExpansionLocations unittest-case

No changes made besides backporting API changes from the immediate child revision, D93223.
This way the API of MacroExpansionContext won't be changed in later parts of this stack.

For the record, please rename this revision to have the analyzer tag.

Please @ASDenysPetrov, give full context with the option -U99999999 when you export the diff from git.
I would like to quickly swipe through the code before I accept this.

Updated the comment of CrossTranslationUnitContext::getMacroExpansionContextForSourceLocation.

Update: resolve @balazske's comment D94673#2498165.

• remove ASTImporter::FileIDImportHandler etc.
• updates the revision's summary accordingly

In D94673#2498165, @balazske wrote:

Probably the CrossTranslationUnitContext::ImportedFileIDs can be removed. The FileIDImportHandler in ASTImporter was used only for this (if I know correctly) too but it can be useful if a similar mechanism is needed later, but it remains there as "dead code" if not removed.

Excellent catch!

It seems quite a challenge to hook the Preprocessor for all possible configurations for every CompilerInvocation.
The underlying machinery is somewhat complex and spaghetti to me.

In D93224#2493710, @steakhal wrote:

In D93224#2493515, @xazax.hun wrote:

Could you validate that this solution works for the latter [ctu-on-demand]?

Sure, I suspect that will have the required PP events since the Preprocessor is parsing source code :D
I'm going to check this just to be sure.

On-demand parsing does indeed populate the required PP events.
The 4-th element of this patch stack will aim to implement macro expansions for that scenario.

In D93224#2493515, @xazax.hun wrote:

In D93224#2493434, @steakhal wrote:

How should I continue to get this working with CTU?

We have two CTU modes. One loading the dump and the other loading from AST source file. I assume that you refer to the former?

Yes, I was referring to that.

Could you validate that this solution works for the latter?

Sure, I suspect that will have the required PP events since the Preprocessor is parsing source code :D
I'm going to check this just to be sure.

Unfortunately, I could not come up with a proper CTU implementation.
It seems that when we load the AST/dump, no preprocessor events are replayed.
Without those events, my PPCallbacks implementation and tokenwatcher would not record anything, drawing macro expansion useless for CTU.

Updates:

• Rebase stack.
• Add FileCheck context for plist-macros-with-expansion.cpp.
• Remove the Inputs/expected-plists/plist-macros-with-expansion.cpp.plist file.
• Create an extra test file, testing C related stuff, such as Implicit function declaration in plist-macros-with-expansion.c.
• Return None for CTU macro expansions instead of an arbitrary string.

Updates:

• New the construction of MacroExpansionContext won't hook the Preprocessor in the constructor. Hooking is done via the registerForPreprocessor(PP) member function.
• Hooking the PP now depends on the ShouldDisplayMacroExpansions analyzer option.
• Both getExpandedText and getOriginalText returns Optional<StringRef>. Semantics and comments changed accordingly. If the ShouldDisplayMacroExpansions analyzer flag is not set, both of these functions always return None.
• Removed the accidental createHTMLSingleFileDiagnosticConsumer prototype in PathDiagnostic.h.
• New unittest case added NoneForNonExpansionLocations.
• The rest of the tests were uplifted to unwrap the Optional to accommodate the new API.
• The last assertion of the RedefUndef unittest case was changed to match the new behavior for non-expansion locations.

Seems pretty straightforward and clean.

Here is a link for our results on a few more projects. It might be useful for you.
https://codechecker-demo.eastus.cloudapp.azure.com/Default/runs?run=D92634&items-per-page=50&sort-by=name&sort-desc=false
Note: use the diff to filter only for the new reports.

• move MacroExpansionRangeRecorder to clang::detail and mark it as a friend class
• fix comment typo in getExpandedMacroForLocation
• rename getExpandedMacroForLocation -> getExpandedText
• rename getSubstitutedTextForLocation -> getOriginalText
• introduce llvm_unreachable where applicable.

No tests were changed.

In D92634#2478503, @OikawaKirie wrote:

Besides, as far as I am thinking, the compiler optimizes the expression as it is literally inferable, i.e. the result should be determinable literally during compilation. Otherwise, it will be computed during runtime. Therefore I suggest you can do a similar check with the ASTMatcher, since the and and or conjunctions will be removed in the CFG.

I disagree on this.
You can never be sure if a given function gets inlined or not, thus the context does matter for compiler optimizations. The given value might get constant folded and get the comparison/branch optimized away.
In a path sensitive way, we will always suffer from the limitations of the constraint solver.

This form of macro expansion is obsolete - I hope that the community agrees on this.
Crashes for many more cases then just the one you mentioned. Its a really hard and bugrone to mimic the preprocessor, thus we seek to substitute this with the clang's preprocessor implementation.
I recommend pushing an XFAIL lit test demonstrating the current limitation (without your proposed fix), while we can show that this will pass after we accept my patches.
For the record those are in an early phase at D93222.

In D91000#2469898, @lebedev.ri wrote:

I think the question is, *why* are these checks being implemented?
Just to claim that for some particular rule there is a check, and cross it off a list?

Initially, yes. I think one could learn a lot from contributing to any project.
It's not inherently a bad thing to combine these two.

In D91000#2469884, @lebedev.ri wrote:

In D91000#2469880, @steakhal wrote:

In D91000#2465514, @ktomi996 wrote:

In D91000#2382562, @steakhal wrote:

Quoting the revision summary:

This checker guards against using some vulnerable C functions which are mentioned in MSC24-C in obsolescent functions table.

Why don't we check the rest of the functions as well?
asctime, atof, atoi, atol, atoll, ctime, fopen, freopen, rewind, setbuf

Hm, I get that cert-err34-c will already diagnose the uses of atof, atoi, atol, atoll, but then why do we check vfscanf, vscanf then?
We should probably omit these, while documenting this.
On the other hand, I would recommend checking asctime, ctime, fopen, freopen, rewind, setbuf for the sake of completeness.

I only check functions which are in Unchecked Obsolescent Functions without setbuf because setbuf does not have a safer alternative in Annex K.
https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions

From the user's point of view, it does not matter if the safer alternative is inside annex K or not.

I suspect that should be a flag in the check's options.

I don't see any value in disabling the check for eg. setbuf. Even the man page says that you should use the setvbuf instead.
Why would anyone disable this if one already wants diagnostics for the CERT rule it implements?

In D91000#2465514, @ktomi996 wrote:

In D91000#2382562, @steakhal wrote:

Quoting the revision summary:

This checker guards against using some vulnerable C functions which are mentioned in MSC24-C in obsolescent functions table.

Why don't we check the rest of the functions as well?
asctime, atof, atoi, atol, atoll, ctime, fopen, freopen, rewind, setbuf

Hm, I get that cert-err34-c will already diagnose the uses of atof, atoi, atol, atoll, but then why do we check vfscanf, vscanf then?
We should probably omit these, while documenting this.
On the other hand, I would recommend checking asctime, ctime, fopen, freopen, rewind, setbuf for the sake of completeness.

I only check functions which are in Unchecked Obsolescent Functions without setbuf because setbuf does not have a safer alternative in Annex K.
https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions

@kovacs-levent @aaron.ballman
On this CodeChecker instance, you can find the reports on several projects.
The relevant runs are suffixed with kovacs_D92777. @kovacs-levent, you can use the demo/demo user as well, for categorizing the reports into false/true positives.
Once you clicked on a run, you can filter out the relevant reports by the Checker's name bugprone-strlen-in-array-index.

Thanks for the review.

I want to highlight, that the 4th part of the stack is not yet done.
Partially because I'm not quite familiar with CTU.
AFAIK, CTU creates compiler invocations, which are going to call Parse at some point.
I'm not sure how to configure, apply the callbacks, etc. on the Preprocessor in the middle of that process.
I'm also reluctant to pass this domain-specific MacroExpansionContext as the 23rd parameter to that generic function (ASTUnit::LoadFromCommandLine).

Internally we analyzed 16 projects with this patch-stack. No reports were changed!
Now we don't crash on macro expansions in our test set, yey.

This would be a serious change. Especially if we internally already depend on modulo arithmetic.
For example, the ArrayBoundV2 might exploit this fact - when it rearranges the inequality. I might be wrong on this though.
Besides that checker, I can't mention any others.

One more thing.
Please reflow the path's summary, to keep the range constraint in a single line.

It must have been a tedious task to collect all these - without any copy-paste errors, really impressive!

Thank you.

LGTM besides my inline comment.

In D90157#2361773, @ASDenysPetrov wrote:

Who can confirm if this is correct or somewhere it needs fixes? Here is a generated result of evalCast from the origin branch(before the patch):

void foo(int* x,  // &SymRegion{reg_$0<int * x>}
         int** y, // &SymRegion{reg_$1<int ** y>}
         int***z) // &SymRegion{reg_$2<int *** z>}
{
 (void*)x;    // &SymRegion{reg_$0<int * x>}
 (void*)y;    // &SymRegion{reg_$1<int ** y>}
 (void*)z;    // &SymRegion{reg_$2<int *** z>}
 (void**)x;   // &Element{SymRegion{reg_$0<int * x>},0 S64b,void *}
 (void**)y;   // &SymRegion{reg_$1<int ** y>}
 (void**)z;   // &SymRegion{reg_$2<int *** z>}
 (void***)x;  // &Element{SymRegion{reg_$0<int * x>},0 S64b,void **}
 (void***)y;  // &Element{SymRegion{reg_$1<int ** y>},0 S64b,void **}
 (void***)z;  // &SymRegion{reg_$2<int *** z>}
 (void****)x; // &Element{SymRegion{reg_$0<int * x>},0 S64b,void ***}
 (void****)y; // &Element{SymRegion{reg_$1<int ** y>},0 S64b,void ***}
 (void****)z; // &Element{SymRegion{reg_$2<int *** z>},0 S64b,void ***}
}

AFAIK, Element regions represent the reinterpret casts, and static-casts doesn't require anything to do as the loading from the region would be cast to the appropriate type.
So these dumps seem to be correct.

Awesome, thank you.

I've double-checked the return values of each touched summary.
Everything seems fine to me, besides the two I've highlighted.

In D92307#2422624, @joerg wrote:

off_t is s signed type. Please fix the description.

Oh, thanks. Updated.

In D52957#2379330, @NoQ wrote:

You cannot have an argument expression because there's no argument expression anywhere in the AST. There's an argument, but it's not computed as a value of any syntactic expression. If there was no argument, getArgExpr(0) would have crashed; but it returns a nullptr which indicates that there's no expression to return.

Aa, now I see. Thanks.

Quoting the revision summary:

This checker guards against using some vulnerable C functions which are mentioned in MSC24-C in obsolescent functions table.

Why don't we check the rest of the functions as well?
asctime, atof, atoi, atol, atoll, ctime, fopen, freopen, rewind, setbuf

In D52957#2377914, @NoQ wrote:

Everything looks good to me here. The new-expression new int; has 1 implicit argument (allocation size passed to the implementation of operator new, the value is probably 4) and 0 placement arguments (the ones that are explicitly written down after new and before int). See also https://en.cppreference.com/w/cpp/language/new#Placement_new.

I'm not sure if this implementation is correct.

In D90157#2356170, @martong wrote:

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.

This is not going to be easy. I guess we should cover both implicit and explicit type conversions. C++ is ridiculously complex in these domains. For maximal precision I'd suggest to systematically go through the below pages and try to cover each major case:
https://en.cppreference.com/w/cpp/language/explicit_cast
https://en.cppreference.com/w/cpp/language/implicit_conversion

In D89987#2349959, @ASDenysPetrov wrote:

@OikawaKirie

Different from ProgramStateRef which is an alias to IntrusiveRefCntPtr, or StoreRef which is a wrapper object, an alias to a const SymExpr * makes no sense to me.

Yes. I omit this, because in such case we should go further and rename all those which are not real Ref to Ptr or smth that would emphasise that it's just a pointer alias, not a wrapper or another class.
That's why I prefered to change the name a little in favor of complex approach of renaming all the rest.

And this is also where I have been confused for a long while.

So have been I. The patch is called to make it more clear :)

Thanks to @steakhal comment I investigated more in terms of what other names use Symbol but mean SymExpr. They are:

class SymbolManager;
using SymbolID;
using SymbolDependTy;
class SymbolData;
class SymbolMetadata;
class SymbolReaper;
enum SymbolStatus;
using SymbolSetTy;
using SymbolMapTy;
class SymbolCast;
class SymbolVal;
class symbol_iterator;
etc.

This is not a full list! I also didn't count methods and file names.
There is also a list of objects names which straightly use SymExpr. They are less spread. Mostly they are derived classes:

class BinarySymExprImpl;
class SymIntExpr;
class IntSymExpr;
class SymSymExpr;
some enums, several methods, etc.

As a result we should accurately define the difference between Symbol and SymExpr.

There are some cases, when Symbol refers to the base-class of the inheritance hierachy. In those cases the renaming would be appropiate.
Though, I'm sure there are cases, when the Symbol refers simply to SymbolData - in which cases we should rename accordingly.

@ASDenysPetrov
Please grep for the SymbolRef and rename the other symbols/comments as well, especially the compound names.

If you don't mind I would also request some credit in the summary since I've spent some time on this as well.
Besides that I don't have much objection about this patch. It's defenately not my expertiese.
Good job coming up with the fix though, I had to focus on other things.

In D88477#2319959, @ASDenysPetrov wrote:

@NoQ @steakhal

Hi, guys.

I've just uploaded a patch for solving this and related D77062. Welcome to review D89055.

In D71524#2304742, @boga95 wrote:

As far as I remember I tried to make std::cin tainted, but it was complicated.

What sort of issues did you find by implementing that approach? Could you elaborate?

In D85528#2307785, @NoQ wrote:

Aha, ok, thanks, I now understand what the problem is because I was able to run the test before the patch and see how the patch changes the behavior.

What do you think about flattening the enum type out entirely? I.e., instead of (unsigned char) conj_$2{enum ScopedSugared} have conj_$2{unsigned char} from the start. Possibly same for unscoped enums. I don't think we actually extract any value from knowing that a symbol is an enum value. We also don't track enum types for concrete values (i.e., nonloc::ConcreteInt doesn't know whether it belongs to an enum).

That's a great idea. It should work. I will investigate this direction.

In D77062#2305856, @ASDenysPetrov wrote:

@steakhal

@ASDenysPetrov Do you still want to rework the API of the assumeZero?

This patch more looks like NFC, being just refactored.

Fine.

In D88477#2304708, @NoQ wrote:

I'm trying to say that the value produced by the load should not be the same as the stored value, because these two values are of different types. When exactly does the first value change into the second value is a different story; the current grand vision around which the code is written says that it changes during load, therefore it's the load code (step #2) that's to blame.

Are you implying that the second dereference of b should produce an lvalue of the type char *, instead of the type of the SVal &Element{SymRegion{reg_$0<int * a>},0 S64b,unsigned char}.
So, I should do this cast when we evaluate the dereference, and produce an lvalue of the static type, aka binding the SVal &Element{SymRegion{reg_$0<int * a>},0 S64b,char*} to it.
In the AST, it is the line @1:

`-IfStmt
  -BinaryOperator 'bool' '=='
   |-ImplicitCastExpr 'char *' <LValueToRValue>
@1:| `-UnaryOperator 'char *' lvalue prefix '*' cannot overflow
   |   `-ImplicitCastExpr 'char **' <LValueToRValue>
   |     `-UnaryOperator 'char **' lvalue prefix '*' cannot overflow
   |       `-ImplicitCastExpr 'char ***' <LValueToRValue>
   |         `-DeclRefExpr 'char ***' lvalue ParmVar 0x55e808fe8188 'b' 'char ***'
   | `-ImplicitCastExpr 'char *' <NullToPointer>
    `-IntegerLiteral 'int' 0

ping

I'm getting lost :D

In D88019#2303078, @martong wrote:

I like the second approach, i.e. to have a debug checker. But I don't see, how would this checker validate all constraints at the moment when they are added to the State.

ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond, bool Assumption) checker callback is called after any assume call.
So, we would have a State which has all? the constraints stored in the appropriate GDM. I'm not sure if we should aggregate all the constraints till this node - like we do for refutation. I think, in this case, we should just make sure that the current state does not have any contradiction.

In D88477#2301641, @NoQ wrote:

A load from a region of type T should always yield a value of type T because that's what a load is.

I'd rather have it as an assertion: if the region is typed, the type shouldn't be specified. If such assertion is hard to satisfy, we could allow the same canonical type to be specified. But having a conflict between two sources of type information and resolving it by arbitrarily choosing one of them sounds scary and unreliable. This patch doesn't eliminate the source of the conflict, it simply changes the way we flip the coin to resolve it.

Oh, now I see. Thank you.

Thank you for your time @balazske!
Your catches were really valuable.

In D88477#2300687, @martong wrote:

In this example, it cast to the unsigned char (which is the type of the stored value of **b, stored at #1) instead of the static type of the access (the type of **b is charat #2).

Possible typo in the summary. At #2 the type should be char * shouldn't it?

Yup, thanks. Updated revision summary accordingly.

In D77062#2298663, @ASDenysPetrov wrote:

@steakhal
You told that you suppose a potential fix. It would be nice, if you share the patch to review.

I mean, I already told you my suggestion, there was no concrete fix in my mind at the time.

In D88359#2297074, @NoQ wrote:

So, ArrayBoundCheckerV3 then?

:D