Page MenuHomePhabricator

steakhal (Balázs Benics)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 7 2019, 1:49 AM (84 w, 5 d)

Recent Activity

Sat, Oct 10

steakhal added a comment to D89055: [analyzer] Wrong type cast occures during pointer dereferencing after type punning.

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.

Sat, Oct 10, 7:06 AM · Restricted Project
steakhal abandoned D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally.
Sat, Oct 10, 6:57 AM · Restricted Project
steakhal added a comment to D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally.

@NoQ @steakhal

Hi, guys.

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

Sat, Oct 10, 6:57 AM · Restricted Project

Fri, Oct 2

steakhal added a comment to D71524: [analyzer] Support tainted objects in GenericTaintChecker.

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?

Fri, Oct 2, 4:45 AM · Restricted Project
steakhal added a comment to D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine.
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.

Fri, Oct 2, 1:07 AM · Restricted Project

Thu, Oct 1

steakhal added a comment to D77062: [analyzer] [NFC] Simplify CStringChecke::assumeZero function.

@steakhal

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

This patch more looks like NFC, being just refactored.

Fine.

Thu, Oct 1, 11:13 AM · Restricted Project
steakhal added a comment to D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally.
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
Thu, Oct 1, 11:04 AM · Restricted Project
steakhal added a comment to D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine.

ping

Thu, Oct 1, 1:49 AM · Restricted Project

Wed, Sep 30

steakhal added a comment to D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally.

I'm getting lost :D

Wed, Sep 30, 12:39 PM · Restricted Project
steakhal added a comment to D88019: [analyzer][solver] Fix issue with symbol non-equality tracking.

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.

Wed, Sep 30, 9:49 AM · Restricted Project
steakhal added a comment to D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally.
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.

Wed, Sep 30, 7:11 AM · Restricted Project

Tue, Sep 29

steakhal added a comment to D88359: [analyzer][RFC] Complete rewrite of ArrayBoundCheckerV2.

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

Tue, Sep 29, 9:39 AM · Restricted Project
steakhal added a comment to D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally.

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.

Tue, Sep 29, 9:04 AM · Restricted Project
steakhal updated the summary of D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally.
Tue, Sep 29, 9:03 AM · Restricted Project
steakhal added a comment to D77062: [analyzer] [NFC] Simplify CStringChecke::assumeZero function.

@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.

Tue, Sep 29, 4:44 AM · Restricted Project
steakhal requested review of D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally.
Tue, Sep 29, 4:39 AM · Restricted Project

Mon, Sep 28

steakhal added a comment to D88359: [analyzer][RFC] Complete rewrite of ArrayBoundCheckerV2.
In D88359#2297074, @NoQ wrote:

So, ArrayBoundCheckerV3 then?

:D

Mon, Sep 28, 1:37 AM · Restricted Project

Sat, Sep 26

steakhal requested review of D88359: [analyzer][RFC] Complete rewrite of ArrayBoundCheckerV2.
Sat, Sep 26, 7:39 AM · Restricted Project
steakhal added a comment to D88019: [analyzer][solver] Fix issue with symbol non-equality tracking.

What are our options mitigating anything similar happening in the future?

This way any change touching the SymbolicRangeInferrer and any related parts of the analyzer seems to be way too fragile.
Especially, since we might want to add support for comparing SymSyms, just like we try to do in D77792.

Sat, Sep 26, 2:29 AM · Restricted Project

Fri, Sep 25

steakhal added a comment to D77062: [analyzer] [NFC] Simplify CStringChecke::assumeZero function.

Though, the fix probably will not be simple, because the issue itself always requires a 3x indirection. The code that is presented by @steakhal is the least minimal example to get this crash. The reason why we cannot have a crash with a ** is a mystic at the moment.

I think probably the representation of casts is behind this.

Fri, Sep 25, 2:53 AM · Restricted Project
steakhal added a comment to D77062: [analyzer] [NFC] Simplify CStringChecke::assumeZero function.

And of course, repro:

./bin/clang -cc1 -analyze -setup-static-analyzer -analyzer-checker=core example.c
Fri, Sep 25, 2:40 AM · Restricted Project
steakhal added a comment to D77062: [analyzer] [NFC] Simplify CStringChecke::assumeZero function.

Finally, I made my investigations and I come up with this code:

void strcpy(char *, char *);
void test(int *a, char ***b) {
  *(unsigned char **)b = (unsigned char*)a; // #1
  if (**b == nullptr) // will-crash
    ;
}

So, this issue does not relate to CStringChecker. It will crash at ExprEngineC.cpp:100.
It seems that we have to make the choice of how to model type punning.
As you can see in the example, we overwrite the pointer value of *b to point to an unsigned char value (#1).
The static type of b (char*) does not reflect the associated value's type which (unsigned char) - (note the number of indirections!) in other words, an obvious type pun happened at that line.
If we get the value of **b, we get a NonLoc of type unsigned char.
The dump of **b confirms this: reg_$4<unsigned char Element{SymRegion{reg_$0<int * a>},0 S64b,unsigned char}>, which is a NonLoc in deed.

Fri, Sep 25, 2:18 AM · Restricted Project

Thu, Sep 24

steakhal added a comment to D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size.

Should I create more measurements?

Thu, Sep 24, 1:54 AM · Restricted Project
steakhal added a comment to D88019: [analyzer][solver] Fix issue with symbol non-equality tracking.

What are our options mitigating anything similar happening in the future?

Thu, Sep 24, 1:43 AM · Restricted Project
steakhal added a comment to D77062: [analyzer] [NFC] Simplify CStringChecke::assumeZero function.

I think what what be great to see here (and this seems to be the thing @NoQ is fishing for) is not to change an if branch and avoid running into the crash, but rather find out why assumeZero was ever called with a nonloc value, which shouldn't really happen. We're treating the symptom, not curing the illness, if you will. The SVal (if its DefinedSVal) is supposed to be always MemRegionVal here, is that right? Maybe if we tried to add an assert here, that could help nail where the actual issue is coming from. It's probably in evalStrcpyCommon, judging from the bug report you linked in your summary.

I would not accept this patch unless this investigation is done. However, I'm not inherently against this change.

Thu, Sep 24, 1:36 AM · Restricted Project
steakhal added a comment to D71524: [analyzer] Support tainted objects in GenericTaintChecker.

I figured you're still working on this, sorry! I'd really like to chat about my earlier comment D71524#1917251, as it kind of challenges the high level idea.

Thu, Sep 24, 1:29 AM · Restricted Project

Sep 16 2020

steakhal added inline comments to D87785: [analyzer][StdLibraryFunctionsChecker] Fix a BufferSize constraint crash.
Sep 16 2020, 12:53 PM · Restricted Project

Sep 14 2020

Herald added a reviewer for D61657: Add implementation to two OMPT API routines: jdoerfert.
Sep 14 2020, 12:42 AM · Restricted Project

Sep 13 2020

steakhal committed rGcdacffe4acc0: [analyzer][z3] Use more elaborate Z3 variable names (authored by steakhal).
[analyzer][z3] Use more elaborate Z3 variable names
Sep 13 2020, 11:45 PM
steakhal committed rGd7ae9696e31f: [analyzer][docs][NFC] Document the ento namespace in the llvm/Lexicon (authored by steakhal).
[analyzer][docs][NFC] Document the ento namespace in the llvm/Lexicon
Sep 13 2020, 11:44 PM
steakhal committed rG163863604f9c: [analyzer] Evaluate PredefinedExpressions (authored by steakhal).
[analyzer] Evaluate PredefinedExpressions
Sep 13 2020, 11:44 PM
steakhal closed D86223: [analyzer][z3] Use more elaborate Z3 variable names.
Sep 13 2020, 11:44 PM · Restricted Project
steakhal closed D86446: [analyzer][docs][NFC] Document the ento namespace in the llvm/Lexicon.
Sep 13 2020, 11:44 PM · Restricted Project
steakhal closed D87004: [analyzer] Evaluate PredefinedExpressions.
Sep 13 2020, 11:44 PM · Restricted Project
steakhal updated subscribers of D84544: [ConstraintSystem] Add helpers to deal with linear constraints..

I'm curious if we could make use of this in the Clang Static Analyzer.
We always hit the limitations of the current Constraint manager implementation.
What do you think about this @NoQ @xazax.hun @Szelethus?

Sep 13 2020, 6:56 AM · Restricted Project

Sep 10 2020

steakhal added a comment to D83660: [analyzer] Fix a crash for dereferencing an empty llvm::Optional variable in SMTConstraintManager.h..

After reviewing the code of this snippet, I think it would be very difficult to make a regression test case for the crash, as far as what I know about Z3 and SMT solvers.

First of all, all calls to Solver->check() will return true for sat, false for unsat, and empty for a timeout.
On line 132, the manager invokes Z3 for solving the constraints under the current state.
On line 137, the manager invokes Z3 for getting a model (a valid result) satisfying the constraints.
On line 141, the manager adds another constraint to exclude the model gotten from the model.
On line 149, the manager invokes Z3 for solving the excluded constraint.
In summary, the manager will first solver the constraint for a result of the queried symbolic variable. If there is a valid value, it excludes the value and solves again to check whether it is the only valid result.

To trigger the crash, we need to construct a group of constraints that are sat. Then, we need to exclude a valid value for a symbolic variable and make the constraints lead to a timeout (rather than an unsat). Simple linear constraints have very few chances to lead to a timeout. I tried to create a group of constraints from https://stackoverflow.com/questions/20536435/z3-what-might-be-the-reason-for-timeout, which are a group of non-linear unsat constraints that can trigger a timeout (under a timeout of 10 seconds). However, I have not successfully made one, as it has too many things to do with mathematics. And my SMT solver colleagues also think it is quite difficult to make one.

I suspected this one due to my previous investigation.

Sep 10 2020, 12:56 AM · Restricted Project

Sep 9 2020

steakhal accepted D87239: [analyzer][StdLibraryFunctionsChecker] Remove strcasecmp.

So, the problem is rather that the constraint check should be done in checkPreCall, but that should be in an NFC refactoring.

Sep 9 2020, 11:13 AM · Restricted Project
steakhal added a comment to D84316: [analyzer][NFC] Split CStringChecker to modeling and reporting.

I would like to discuss why don't we have a distinct checker managing the bookkeeping stuff of the CString lengths.
I just want a clean understanding and wide consensus about this.

Sep 9 2020, 1:15 AM · Restricted Project

Sep 8 2020

steakhal added a comment to D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME.

But if the string is invalidated (or the new length is completely unknown), do not remove the length from the state; instead, set it to SymbolConjured.

Where exactly do you implement the above?

When strlen(R) is used for the first time on a region R, produce $meta<size_t R> as the answer, but *do not store* it in the string length map.

And this?

Sep 8 2020, 7:45 AM · Restricted Project

Sep 7 2020

steakhal accepted D87138: [analyzer][NFC] Introduce refactoring of PthreadLockChecker.

Seems fine to me.

Sep 7 2020, 8:21 AM · Restricted Project
steakhal added a comment to D87239: [analyzer][StdLibraryFunctionsChecker] Remove strcasecmp.

I completely agree with you.
I plan to further refactor the CStringChecker, but the related patches are pretty much stuck :D

Sep 7 2020, 8:18 AM · Restricted Project
steakhal added a comment to D86135: [analyzer][MacroExpansion] Fix a crash where multiple parameters resolved to __VA_ARGS__.

Perfectly clear, thank you. However, I would still rely on the others to accept this :|

Sep 7 2020, 8:05 AM · Restricted Project
steakhal added a comment to D85984: [analyzer] Add a new checker alpha.cplusplus.CPlusPlus11Lock.

I have checked only your test, but the readability of the reports should be improved.
You frequently refer to previous events, such as This lock has already been unlocked, This lock has already been acquired, etc.
It isn't clear to the reader where do you refer to. IMO you should put a NoteTag at the interesting locations to achieve more readable diagnostics.

Sep 7 2020, 7:58 AM · Restricted Project
steakhal added a comment to D83660: [analyzer] Fix a crash for dereferencing an empty llvm::Optional variable in SMTConstraintManager.h..

Ping @OikawaKirie .
How should we proceed?
I would happily participate in creating a minimal repro for this, but I need at least one crash.

Sep 7 2020, 7:41 AM · Restricted Project
steakhal updated the diff for D86446: [analyzer][docs][NFC] Document the ento namespace in the llvm/Lexicon.
  • Fixed typo in the link Clang Static Analyser -> Clang Static Analyzer
Sep 7 2020, 7:37 AM · Restricted Project
steakhal added a comment to D86874: [analyzer] Fix ArrayBoundCheckerV2 false positive regarding size_t indexer.

I can only imagine how long it took for you to write all this, because reading it wasn't that short either. Thank you so much! It really gave me a perspective on how you see this problem, as well as what is actually happening (and should happen) in the checker.

Thank you very much. I tied to make it as short as possible while keeping all the necessary details to make my reasoning perfectly clean. Now I think I was on a bad track, but @martong helped me to pinpoint some issues.

Sep 7 2020, 7:28 AM · Restricted Project
steakhal added a comment to D86874: [analyzer] Fix ArrayBoundCheckerV2 false positive regarding size_t indexer.
Calculation of the RegionRawOffsetV2

Let's see how does these subscript expressions used during the calculation of the RegionRawOffsetV2:
For multi-dimension arrays we fold the index expressions of the nested ElementRegions.
We are doing that by simply calculating the byte-offset of the nested region, and adding the current level's index*sizeof(Type).
So, the ByteOffset that we build up will contain mostly multiplications by a constant OR additions of symbolic expressions (like the x+1 in the example).

We have these lines in the case of handling the ElementRegion:

if (!index.getAs<NonLoc>())
  return RegionRawOffsetV2();

So, if the index is symbolic we give up, don't we? So, how could this work with x+1? What am I missing here?

In my example, I'm speaking about the case when it's nonloc::SymbolVal wrapping the expression x+1. So not every NonLoc is ConcreteInt.

Sep 7 2020, 7:11 AM · Restricted Project
steakhal updated the diff for D86446: [analyzer][docs][NFC] Document the ento namespace in the llvm/Lexicon.
  • Mentioned the previous namespaces as well (GR, entoGR)
  • Slightly rephrased previous sentences
  • Updated the summary, to give a deeper dive into the history of the Clang Static Analyzer - for git blame
Sep 7 2020, 5:38 AM · Restricted Project
steakhal added a comment to D85424: [Analyzer] Crash fix for alpha.cplusplus.IteratorRange.

OK, after a few hours of debugging, the test code simplifies to this:

// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true %s -verify
Sep 7 2020, 3:20 AM · Restricted Project

Sep 4 2020

steakhal added a comment to D86874: [analyzer] Fix ArrayBoundCheckerV2 false positive regarding size_t indexer.

Hi Balázs,

Since reviews.llvm.org is offline, I am sending my comments below, inline.
Thanks for your huge effort in explaining all this!

Overall, I have a feeling that this approach targets only one specific
case, which is fine. But I believe we should think about all the other
possible cases, so we could get rid of other false positives too:

  1. In case of multidimensional arrays, there may be a symbolic value in any

dimension.

Yes, obviously - but it's not a problem. See my next comment.

Sep 4 2020, 3:16 AM · Restricted Project

Sep 3 2020

steakhal added a comment to D86874: [analyzer] Fix ArrayBoundCheckerV2 false positive regarding size_t indexer.

I really feel that this check would have a better place in the implementation of eval. This seems really counter-intuitive to do this stuff at the Checker's level. Is there any reason why we can't do this in eval?
evalBinOpNN could return with Unknown, and the state should remain unchanged. Am I missing something?

Sep 3 2020, 7:52 AM · Restricted Project

Sep 2 2020

steakhal added a comment to D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine.

I'm not opposed to landing this to master, as we will have more time there to see whether there are any unwanted side effects in practice.

Sep 2 2020, 9:51 AM · Restricted Project
steakhal added a comment to D87004: [analyzer] Evaluate PredefinedExpressions.

Thank you all for the comments!

Sep 2 2020, 7:51 AM · Restricted Project
steakhal updated the diff for D87004: [analyzer] Evaluate PredefinedExpressions.
  • Added no-warning.
  • Added test-case for __builtin_unique_stable_name as well.
Sep 2 2020, 7:50 AM · Restricted Project
steakhal added inline comments to D87004: [analyzer] Evaluate PredefinedExpressions.
Sep 2 2020, 6:30 AM · Restricted Project
steakhal updated the diff for D87004: [analyzer] Evaluate PredefinedExpressions.

We only analyze instantiated functions, which are not dependently typed.
Safe to assume that every encountered PredefinedExpression has a defined (non-null) function name.

Sep 2 2020, 6:30 AM · Restricted Project
steakhal added inline comments to D87004: [analyzer] Evaluate PredefinedExpressions.
Sep 2 2020, 4:45 AM · Restricted Project
steakhal updated the diff for D87004: [analyzer] Evaluate PredefinedExpressions.
  • Added tests for Microsoft extensions.
  • Added an assert requiring the PredefinedExpression to have a function name.
Sep 2 2020, 4:43 AM · Restricted Project
steakhal requested review of D87004: [analyzer] Evaluate PredefinedExpressions.
Sep 2 2020, 2:30 AM · Restricted Project

Sep 1 2020

steakhal added inline comments to D86870: [analyzer] Add more tests for ArrayBoundCheckerV2.
Sep 1 2020, 2:47 AM · Restricted Project
steakhal added a comment to D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size.

I tried to run the benchmark on the small set of projects but it would take an enormous time to analyze all such projects 20 times each...
Dedicating a box for this is unfeasible for me.
So I stuck with analyzing only tmux with 20 iterations.
The results are not convincing enough IMO.

Sep 1 2020, 2:26 AM · Restricted Project
steakhal added inline comments to D86873: [analyzer][NFC] Refactor ArrayBoundCheckerV2.
Sep 1 2020, 2:02 AM · Restricted Project
steakhal added inline comments to D86870: [analyzer] Add more tests for ArrayBoundCheckerV2.
Sep 1 2020, 1:52 AM · Restricted Project

Aug 31 2020

steakhal added a comment to D85424: [Analyzer] Crash fix for alpha.cplusplus.IteratorRange.

CReduce did not manage to produce any meaningful result after a week worth of runtime (more than ~2000 lines of code still remaining after reduction). We could track this down by tracing the ExprEngine code that assigns the Undefined SVal but that seems a huge effort as well. That could be done by debugging the SVal-assigning statements, and setting conditional breakpoints (ie. only break when the value is Undefined). When a breakpoint is hit, we could dump the statement that triggered it and try to reason about the conditions at that point. I also recommend using the rr tool as it allows you to use fixed pointer values while debugging.

Aug 31 2020, 10:16 AM · Restricted Project
steakhal resigned from D86736: [analyzer][NFC] Don't bind values to ObjCForCollectionStmt, replace it with a GDM trait.

I don't have much to say, but to keep up the good work xD
I will follow this and the rest of your patches @Szelethus.

Aug 31 2020, 5:39 AM · Restricted Project
steakhal requested review of D86874: [analyzer] Fix ArrayBoundCheckerV2 false positive regarding size_t indexer.
Aug 31 2020, 5:18 AM · Restricted Project
steakhal requested review of D86873: [analyzer][NFC] Refactor ArrayBoundCheckerV2.
Aug 31 2020, 4:53 AM · Restricted Project
steakhal updated the summary of D86870: [analyzer] Add more tests for ArrayBoundCheckerV2.
Aug 31 2020, 4:31 AM · Restricted Project
steakhal requested review of D86870: [analyzer] Add more tests for ArrayBoundCheckerV2.
Aug 31 2020, 4:30 AM · Restricted Project

Aug 27 2020

steakhal added a comment to D86465: [analyzer][solver] Redesign constraint ranges data structure.

Previously, I liked this. Now I love it!
The benchmarks look promising as well.

Aug 27 2020, 9:11 AM · Restricted Project

Aug 26 2020

steakhal added inline comments to D86465: [analyzer][solver] Redesign constraint ranges data structure.
Aug 26 2020, 5:49 AM · Restricted Project
steakhal added inline comments to D86533: (Urgent!) [release][docs][analyzer] Add 11.0.0. release notes.
Aug 26 2020, 5:30 AM · Restricted Project
steakhal added inline comments to D86446: [analyzer][docs][NFC] Document the ento namespace in the llvm/Lexicon.
Aug 26 2020, 2:51 AM · Restricted Project
steakhal added a comment to D81254: [analyzer] Produce symbolic values for C-array elements.

We know where it points to (aka. the pointer's value is conjured), but we don't know what the value if there.

Absolutely right. I looked at this patch after a while and don't currently see a proper solution.

I feel like this problem should be handled by some sort of invalidation mechanism.

Definitely it should be some criteria/mark/flag about the region invalidation. Maybe someone more confident could prompt us how to.

How about using the mistical SymbolDerived?
The clang-analyzer-guide says:

Another atomic symbol, closely related to SymbolRegionValue, is SymbolDerived. It represents a value of a region after another symbol was written into a direct or indirect super-region. SymbolDerived contains a reference to both the parent symbol and the parent region. This symbol is mostly a technical hack. Usually SymbolDerived appears after invalidation: the whole structure of a certain type gets smashed with a single SymbolConjured, and then values of its fields become represented with the help of SymbolDerived of that conjured symbol and the region of the field. In any case, SymbolDerived is similar to SymbolRegionValue, just refers to a value after a certain event during analysis rather than at start of analysis.

Could we use that here @NoQ?

Aug 26 2020, 2:41 AM · Restricted Project
steakhal added inline comments to D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME.
Aug 26 2020, 1:28 AM · Restricted Project

Aug 25 2020

steakhal added a comment to D85728: [Analyzer] Support for the new variadic isa<> and isa_and_not_null<> in CastValueChecker.
In D85728#2237006, @NoQ wrote:

Hans is pinging us on Bugzilla because this patch is marked as a release blocker (and i believe that it indeed is). I think we should land these patches.

I believe @baloghadamsoftware is on vacation.
I'm sure he won't be mad if you commit on his behalf.

Aug 25 2020, 12:19 PM · Restricted Project
steakhal added a comment to D86135: [analyzer][MacroExpansion] Fix a crash where multiple parameters resolved to __VA_ARGS__.

I'm not sure about the status of this patch.
If you say that further improvements will be done later and this functionality is enough, I'm fine with that.

Aug 25 2020, 7:59 AM · Restricted Project
steakhal added a comment to D86465: [analyzer][solver] Redesign constraint ranges data structure.

This is a huge change, I've got pretty tired at the end of the review.
I haven't checked the test-cases too deeply.

Aug 25 2020, 7:36 AM · Restricted Project
steakhal requested changes to D86135: [analyzer][MacroExpansion] Fix a crash where multiple parameters resolved to __VA_ARGS__.

This preprocessor expansion stuff is definitely not my expertise, nvm here is my review.

Aug 25 2020, 2:55 AM · Restricted Project

Aug 24 2020

steakhal updated the diff for D86446: [analyzer][docs][NFC] Document the ento namespace in the llvm/Lexicon.
  • Rephrase paragraph
  • Use desktop site wikipedia link
Aug 24 2020, 10:52 AM · Restricted Project
steakhal added inline comments to D86446: [analyzer][docs][NFC] Document the ento namespace in the llvm/Lexicon.
Aug 24 2020, 8:04 AM · Restricted Project
steakhal updated the diff for D86446: [analyzer][docs][NFC] Document the ento namespace in the llvm/Lexicon.

Add link to the static analyzer llvm page.

Aug 24 2020, 8:04 AM · Restricted Project
steakhal added a comment to D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size.

And what is the error right now?

Aug 24 2020, 7:55 AM · Restricted Project
steakhal added a comment to D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size.

Yep, I guess that is the cause. I'll take a look. Did you try it with this fast fix?

I tried, but it lacks further fixes.

Aug 24 2020, 7:26 AM · Restricted Project
steakhal added a comment to D84316: [analyzer][NFC] Split CStringChecker to modeling and reporting.

Do I sense correctly that the only information CSrtingLengthModeling.cpp requires from the actual CStringChecker is a checker tag?

AFAIK yes.

Aug 24 2020, 6:49 AM · Restricted Project
steakhal added inline comments to D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME.
Aug 24 2020, 6:39 AM · Restricted Project
steakhal added inline comments to D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME.
Aug 24 2020, 6:33 AM · Restricted Project
steakhal added a comment to D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size.

Here is a short summary how to do regression testing (check that all warnings are the same):

Oh thanks for the detailed guide, I will make the experiment.

Aug 24 2020, 5:40 AM · Restricted Project
steakhal requested review of D86446: [analyzer][docs][NFC] Document the ento namespace in the llvm/Lexicon.
Aug 24 2020, 5:07 AM · Restricted Project
steakhal requested review of D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME.
Aug 24 2020, 4:15 AM · Restricted Project
steakhal added a comment to D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size.
In D86295#2231760, @NoQ wrote:

I believe @vsavchenko's docker thingy already knows how to do that.

Yep, it sure does! Additionally, there is a benchmark subcommand that can chart memory consumption for measured projects.

Could you point me to some docs to help getting started? I haven't used this docker image.

Aug 24 2020, 3:09 AM · Restricted Project
steakhal updated the diff for D86223: [analyzer][z3] Use more elaborate Z3 variable names.

I don't mind having it for release builds as well, why are you applying it only for debug builds?

It might introduce a slight overhead since Z3 will parse longer the symbol names.

That overhead should be negligible, in the worst case you are adding just a few extra characters to the variable's name.

I rather have it for release builds as well so we don't introduce different outputs depending on the build options, and we can improve debugging for release builds as well.

Also as a bonus, we don't have to change the test scripts for a single test.

Fixed.

Aug 24 2020, 12:44 AM · Restricted Project

Aug 22 2020

steakhal added a comment to D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size.
In D86295#2231760, @NoQ wrote:

I mean, like, you can measure the entire process with time or something like that. I believe @vsavchenko's docker thingy already knows how to do that.

I tried to measure this with time, without much luck:

Aug 22 2020, 9:49 AM · Restricted Project
steakhal added inline comments to D86223: [analyzer][z3] Use more elaborate Z3 variable names.
Aug 22 2020, 9:44 AM · Restricted Project
steakhal updated the diff for D86223: [analyzer][z3] Use more elaborate Z3 variable names.

I don't mind having it for release builds as well, why are you applying it only for debug builds?

It might introduce a slight overhead since Z3 will parse longer the symbol names.

Aug 22 2020, 9:43 AM · Restricted Project
steakhal updated the diff for D86223: [analyzer][z3] Use more elaborate Z3 variable names.

Use virtual getKindStr method for acquiring the kind name.

Aug 22 2020, 8:14 AM · Restricted Project

Aug 21 2020

steakhal added a comment to D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size.
In D86295#2229712, @NoQ wrote:

Heh, nice! Did you try to measure the actual impact of this change on memory and/or performance?

Eh, I don't really know how to bench this.
Here is how I did it in nutshell:
Added STATISTIC counters to MemRegion ctor, dtor, getAsOffset begining and the path of just returning the cached value.

Aug 21 2020, 8:23 AM · Restricted Project
steakhal added a comment to D83660: [analyzer] Fix a crash for dereferencing an empty llvm::Optional variable in SMTConstraintManager.h..

It seems that this patch is stuck.
How can I reproduce the crash? @OikawaKirie

Aug 21 2020, 6:28 AM · Restricted Project

Aug 20 2020

steakhal added a comment to D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine.

It would be nice to have this fix in clang11.
Do you think it's qualified for it?

Aug 20 2020, 2:02 PM · Restricted Project
steakhal added a comment to D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size.

But unless you add a comment one will probably replace the offset with an optional as the part of a refactoring.

Sure, I will leave a note.

Aug 20 2020, 10:11 AM · Restricted Project