NoQ (Artem Dergachev)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 3 2015, 9:16 AM (129 w, 2 d)

Recent Activity

Yesterday

NoQ created D43714: [analyzer] Don't do anything when trivial-copying an empty class object..
Fri, Feb 23, 8:28 PM
NoQ added inline comments to D43421: [analyzer] Do not analyze bison-generated files.
Fri, Feb 23, 7:17 PM
NoQ added a comment to D43354: [analyzer] Exploration strategy prioritizing unexplored nodes first.

Looks great. Do you have any tests to demonstrate the difference?

Fri, Feb 23, 1:42 PM
NoQ accepted D41848: [analyzer] mark returns of functions where the region passed as parameter was not initialized.

I think this is ready to go. The "stars" are aligned correctly

LGTM. Hope they're still aligned =)

Fri, Feb 23, 12:47 PM
NoQ added a dependency for D43689: [analyzer] Disable constructor inlining when lifetime extension through fields occurs.: D43533: [CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases..
Fri, Feb 23, 11:22 AM
NoQ added a dependent revision for D43533: [CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases.: D43689: [analyzer] Disable constructor inlining when lifetime extension through fields occurs..
Fri, Feb 23, 11:22 AM
NoQ created D43689: [analyzer] Disable constructor inlining when lifetime extension through fields occurs..
Fri, Feb 23, 11:21 AM
NoQ updated the diff for D43533: [CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases..

Missing break!~~

Fri, Feb 23, 11:09 AM

Thu, Feb 22

NoQ added a comment to D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases..

Add a FIXME test case that demonstrates that automatic destructors don't fire after lifetime extension through a POD field, even though lifetime extension itself seems to work correctly.

Thu, Feb 22, 8:17 PM
NoQ added a dependency for D43666: [analyzer] When constructing a temporary without construction context, track it for destruction anyway.: D43533: [CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases..
Thu, Feb 22, 6:16 PM
NoQ added a dependent revision for D43533: [CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases.: D43666: [analyzer] When constructing a temporary without construction context, track it for destruction anyway..
Thu, Feb 22, 6:16 PM
NoQ created D43666: [analyzer] When constructing a temporary without construction context, track it for destruction anyway..
Thu, Feb 22, 6:14 PM
NoQ added a comment to D43659: [analyzer] Don't crash when dynamic type of a concrete region is hard-set with placement new..

In this case, could we emit a warning? If not from CallEvent, then from where?

Thu, Feb 22, 5:28 PM
NoQ added inline comments to D43659: [analyzer] Don't crash when dynamic type of a concrete region is hard-set with placement new..
Thu, Feb 22, 5:24 PM
NoQ updated the diff for D43659: [analyzer] Don't crash when dynamic type of a concrete region is hard-set with placement new..
  • Add a definitely-not-UB example (char buffers are special).
  • Bring back an accidentally deleted blank line.
Thu, Feb 22, 5:22 PM
NoQ created D43659: [analyzer] Don't crash when dynamic type of a concrete region is hard-set with placement new..
Thu, Feb 22, 4:51 PM
NoQ created D43657: [analyzer] dump() dynamic type info and taint into state dumps..
Thu, Feb 22, 4:41 PM
NoQ added inline comments to D43533: [CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases..
Thu, Feb 22, 11:10 AM
NoQ updated the diff for D43533: [CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases..

Address comments.

Thu, Feb 22, 11:09 AM

Wed, Feb 21

NoQ added inline comments to D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases..
Wed, Feb 21, 12:52 PM
NoQ updated the diff for D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases..
  • Address comments.
  • Add a FIXME test case that demonstrates that automatic destructors don't fire after lifetime extension through a POD field, even though lifetime extension itself seems to work correctly. I should probably also add a test case for the situation where sub-object adjustments actually kick in (here they suddenly don't), because even though they are correctly handled in createTemporaryRegionIfNeeded, they aren't implemented in findConstructionContexts.
Wed, Feb 21, 12:52 PM

Tue, Feb 20

NoQ updated the diff for D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases..

Add one more FIXME test (dont_forget_destructor_around_logical_op in temporaries.cpp) which demonstrates a situation where we fail to call the temporary destructor after calling the temporary constructor. It should be fixed once we implement return value construction properly, as described at the "Return by value" part of http://lists.llvm.org/pipermail/cfe-dev/2018-February/056898.html

Tue, Feb 20, 6:41 PM
NoQ updated the diff for D43533: [CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases..

Remove unused methods.

Tue, Feb 20, 3:43 PM
NoQ added a dependency for D43533: [CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases.: D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases..
Tue, Feb 20, 3:36 PM
NoQ added a dependent revision for D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases.: D43533: [CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases..
Tue, Feb 20, 3:36 PM
NoQ created D43533: [CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases..
Tue, Feb 20, 3:36 PM
NoQ added a comment to D16403: Add scope information to CFG.
In D16403#1011218, @NoQ wrote:

Yeah, i mean, like, if we change the scope markers to also appear even when no variables are present in the scope, then it would be possible to replace loop markers with some of the scope markers, right?

Hm, so does this mean that I need to cover the case when no variables are present in loop scope here in this patch?

Tue, Feb 20, 10:15 AM · Restricted Project

Mon, Feb 19

NoQ added a dependency for D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases.: D43483: [CFG] [analyzer] Add construction context when the constructor is on a branch of a ternary operator.
Mon, Feb 19, 5:38 PM
NoQ added a dependent revision for D43483: [CFG] [analyzer] Add construction context when the constructor is on a branch of a ternary operator: D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases..
Mon, Feb 19, 5:38 PM
NoQ created D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases..
Mon, Feb 19, 5:38 PM
NoQ updated the diff for D43483: [CFG] [analyzer] Add construction context when the constructor is on a branch of a ternary operator.

Fix the test.

Mon, Feb 19, 2:54 PM
NoQ added a dependency for D43483: [CFG] [analyzer] Add construction context when the constructor is on a branch of a ternary operator: D43481: [CFG] [analyzer] Add construction context when the constructor is being no-op-casted to a const value type..
Mon, Feb 19, 2:50 PM
NoQ added a dependent revision for D43481: [CFG] [analyzer] Add construction context when the constructor is being no-op-casted to a const value type.: D43483: [CFG] [analyzer] Add construction context when the constructor is on a branch of a ternary operator.
Mon, Feb 19, 2:50 PM
NoQ created D43483: [CFG] [analyzer] Add construction context when the constructor is on a branch of a ternary operator.
Mon, Feb 19, 2:46 PM
NoQ added a dependency for D43481: [CFG] [analyzer] Add construction context when the constructor is being no-op-casted to a const value type.: D43480: [CFG] [analyzer] Add construction context when the constructor is treated like a functional cast..
Mon, Feb 19, 2:14 PM
NoQ added a dependent revision for D43480: [CFG] [analyzer] Add construction context when the constructor is treated like a functional cast.: D43481: [CFG] [analyzer] Add construction context when the constructor is being no-op-casted to a const value type..
Mon, Feb 19, 2:14 PM
NoQ created D43481: [CFG] [analyzer] Add construction context when the constructor is being no-op-casted to a const value type..
Mon, Feb 19, 2:13 PM
NoQ added a dependency for D43480: [CFG] [analyzer] Add construction context when the constructor is treated like a functional cast.: D43477: [CFG] [analyzer] Add MaterializeTemporaryExpr into the construction context..
Mon, Feb 19, 1:26 PM
NoQ added a dependent revision for D43477: [CFG] [analyzer] Add MaterializeTemporaryExpr into the construction context.: D43480: [CFG] [analyzer] Add construction context when the constructor is treated like a functional cast..
Mon, Feb 19, 1:26 PM
NoQ created D43480: [CFG] [analyzer] Add construction context when the constructor is treated like a functional cast..
Mon, Feb 19, 1:25 PM
NoQ added a comment to D43477: [CFG] [analyzer] Add MaterializeTemporaryExpr into the construction context..

eg. const C &c(123); or the actual (not the elidable copy) constructor in C foo() { return C(123); }

Mon, Feb 19, 12:46 PM
NoQ added a dependent revision for D43428: [CFG] [analyzer] NFC: Allow more complicated construction contexts.: D43477: [CFG] [analyzer] Add MaterializeTemporaryExpr into the construction context..
Mon, Feb 19, 12:02 PM
NoQ added a dependency for D43477: [CFG] [analyzer] Add MaterializeTemporaryExpr into the construction context.: D43428: [CFG] [analyzer] NFC: Allow more complicated construction contexts..
Mon, Feb 19, 12:02 PM
NoQ created D43477: [CFG] [analyzer] Add MaterializeTemporaryExpr into the construction context..
Mon, Feb 19, 11:57 AM

Fri, Feb 16

NoQ added a comment to D16403: Add scope information to CFG.
In D16403#992454, @NoQ wrote:

@szepet: so i see that LoopExit goes in the beginning of the cleanup block after the loop, while various ScopeEnds go after the LoopExit. Would loop unrolling be significantly broken if you simply subscribe to ScopeEnd instead of LoopExit and avoid cleaning up the loop state until destructors are processed? I might not be remembering correctly - is LoopExit only used for cleanup, or do we have more work to be done here?

I guess your following comment just answers this:

In D16403#1001466, @NoQ wrote:

We still don't have scopes for segments of code that don't have any variables in them, so i guess it's not yet in the shape where it is super useful for loops, but it's already useful for finding use of stale stack variables, which was the whole point originally, so i think this should definitely land soon.

It could be, however, we would lose cases like:

int i = 0;
int a[32];
for(i = 0;i<32;++i) {a[i] = i;}

Since there is no variable which has the scope of the loop, ScopeEnd would be not enough. Sure, we could remove this case, however, the aim is to extend the loop-patterns for completely unrolling. Another thing that there are the patches which would enhance the covered cases by LoopExit (D39398) and add the LoopEntrance to the CFG(D41150) as well. So, at this point, I feel like it would be a huge step back not to use these elements. (Sorry Maxim that we are discussing this here^^)

Fri, Feb 16, 8:08 PM · Restricted Project
NoQ accepted D42266: [analyzer] Prevent AnalyzerStatsChecker from crash.

LGTM! @george.karpenkov has also tested that when he was gathering statistics about his traversal order improvements and it helped :)

Fri, Feb 16, 7:44 PM
NoQ added a reviewer for D43428: [CFG] [analyzer] NFC: Allow more complicated construction contexts.: szepet.
Fri, Feb 16, 7:32 PM
NoQ retitled D43428: [CFG] [analyzer] NFC: Allow more complicated construction contexts. from [CFG] NFC: Allow more complicated construction contexts. to [CFG] [analyzer] NFC: Allow more complicated construction contexts..
Fri, Feb 16, 7:32 PM
NoQ updated the diff for D43428: [CFG] [analyzer] NFC: Allow more complicated construction contexts..

Fix comment.

Fri, Feb 16, 7:29 PM
NoQ created D43428: [CFG] [analyzer] NFC: Allow more complicated construction contexts..
Fri, Feb 16, 7:27 PM
NoQ accepted D42773: [analyzer] Remove crashing unneeded assert.

All right, so it sounds like it still needs to be investigated, but at least we've made one more step with a brand-new reproducer.

Fri, Feb 16, 6:44 PM
NoQ accepted D43218: [analyzer] Quickfix: do not overflow in calculating offset in RegionManager.

LGTM otherwise!

Fri, Feb 16, 6:43 PM
NoQ added a comment to D43218: [analyzer] Quickfix: do not overflow in calculating offset in RegionManager.

Dunno, it'd be great to come up with some systematic approach to those debug prints. Having some of them is really nice sometimes.

Fri, Feb 16, 6:43 PM
NoQ accepted D43145: [analyzer] Consider switch- and goto- labels when constructing the set of executed lines.

Yep, looks reasonable.

Fri, Feb 16, 6:34 PM

Wed, Feb 14

NoQ added a comment to D42875: [CFG] [analyzer] Add construction context for ReturnStmt..

rC325202 is attached to this revision accidentally; it should have been attached to D42876.

Wed, Feb 14, 6:58 PM
NoQ closed D42876: [analyzer] Support returning objects by value..
Wed, Feb 14, 6:44 PM
NoQ accepted D42876: [analyzer] Support returning objects by value..

Committed this but put a wrong phabricator link in the commit message, sorry >_<
rC325202

Wed, Feb 14, 6:44 PM
NoQ updated the diff for D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible..

Whoops - recall that we still need to cleanup the temporaries map even, now that we know that we cannot assert that it's already empty. Clean up the map and enable the assertion that becomes tautological until the respective FIXME is fixed.

Wed, Feb 14, 6:20 PM
NoQ updated the diff for D42876: [analyzer] Support returning objects by value..

Update the comment with some thoughts from http://lists.llvm.org/pipermail/cfe-dev/2018-February/056898.html

Wed, Feb 14, 5:29 PM
NoQ updated the diff for D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible..

All right, so the assertion that we actually destroy all temporaries in our InitializedTemporaries map is still violated quite often - every time we do any sort of lifetime extension, we're actually calling the destructor on a different object, because createTemporaryRegionIfNeeded() has moved the object to a different place. I made a large brain dump on this matter in http://lists.llvm.org/pipermail/cfe-dev/2018-February/056898.html . For now it means that the assertion is still not going in. I added it in a commented-out form. In fact, @klimek has tried that long before me, and failed in a similar manner. If only i added the assertion in the right place (not in processCallExit, but in processEndOfFunction, which is also called for the top frame), i would have seen it earlier :) So i've reinvented quite a bit of a wheel here.

Wed, Feb 14, 5:13 PM

Mon, Feb 12

NoQ updated the diff for D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible..
  • Test const C &c = coin ? C(x, y) : C(z, w);.
  • Fix comments surrounding the assertion.
Mon, Feb 12, 4:12 PM
NoQ updated the diff for D42876: [analyzer] Support returning objects by value..

Well, it still seems to be a reasonable improvement given how all temporary materialization works now, and it's under the flag (-analyzer-config cfg-temporary-dtors=true), so i guess i'll mark it as TODO and commit, and i'll keep an eye on that when looking at real-world code analysis results. Destructor fixes and tests are also coming in D43104.

Mon, Feb 12, 2:32 PM

Fri, Feb 9

NoQ updated the diff for D42721: [analyzer] NFC: Use construction contexts for finding the target region for the construction..

Rebase.

Fri, Feb 9, 6:50 PM
NoQ accepted D43131: [analyzer] Serialize statistics to plist when serialize-stats=true is set.

Yep, so i guess printing statistics as plist key-value pairs would be tricky. LGTM!~

Fri, Feb 9, 5:29 PM
NoQ added inline comments to D43131: [analyzer] Serialize statistics to plist when serialize-stats=true is set.
Fri, Feb 9, 5:18 PM
NoQ added inline comments to D43138: [analyzer] Update the documentation to show the command line option.
Fri, Feb 9, 5:14 PM
NoQ added a dependency for D43149: [analyzer] Fix a crash on destroying a temporary array.: D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible..
Fri, Feb 9, 4:49 PM
NoQ added a dependent revision for D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible.: D43149: [analyzer] Fix a crash on destroying a temporary array..
Fri, Feb 9, 4:49 PM
NoQ updated the diff for D43149: [analyzer] Fix a crash on destroying a temporary array..

And even then, calling a destructor of a single array element does not invalidate the whole array for us, because destructors are const (unless there are mutable members). So we'd have to do this manually later as well.

Fri, Feb 9, 4:44 PM
NoQ created D43149: [analyzer] Fix a crash on destroying a temporary array..
Fri, Feb 9, 4:07 PM
NoQ updated the diff for D43144: [analyzer] Implement path notes for temporary destructors..

Add a test for returning from destructor.

Fri, Feb 9, 2:55 PM
NoQ added a comment to D43144: [analyzer] Implement path notes for temporary destructors..

Another question is why do we have such inconsistency between Calling constructor for 'C' and Calling '~C', i.e. why not Calling destructor for 'C'. Seems accidental.

Fri, Feb 9, 2:48 PM
NoQ added inline comments to D43144: [analyzer] Implement path notes for temporary destructors..
Fri, Feb 9, 2:47 PM
NoQ updated the diff for D43144: [analyzer] Implement path notes for temporary destructors..

Minor indent fix.

Fri, Feb 9, 2:35 PM
NoQ added a dependency for D43144: [analyzer] Implement path notes for temporary destructors.: D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible..
Fri, Feb 9, 2:29 PM
NoQ added a dependent revision for D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible.: D43144: [analyzer] Implement path notes for temporary destructors..
Fri, Feb 9, 2:29 PM
NoQ created D43144: [analyzer] Implement path notes for temporary destructors..
Fri, Feb 9, 2:29 PM
NoQ updated the diff for D42779: [analyzer] NFC: Make sure we don't ever inline the constructor for which the temporary destructor is noreturn and missing..

Add the comment.

Fri, Feb 9, 1:12 PM
NoQ updated the diff for D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible..

Assert that the destructors are cleaned up. This assertion is pretty important because it says that we didn't miss any destructors for initialized temporaries during analysis.

Fri, Feb 9, 1:05 PM
NoQ added inline comments to D43138: [analyzer] Update the documentation to show the command line option.
Fri, Feb 9, 12:03 PM
NoQ added a comment to D43131: [analyzer] Serialize statistics to plist when serialize-stats=true is set.

Having a test for the plist output would be nice.

The problem is tests can be run on release configuration, which can not generate statistics at all. How could we check for that in a sensible way?

Fri, Feb 9, 11:07 AM
NoQ added a comment to D43131: [analyzer] Serialize statistics to plist when serialize-stats=true is set.
In D43131#1003607, @NoQ wrote:

So are these present only on translation units that actually caused bug reports to appear?

Having a test for the plist output would be nice.

I suspect such test would be super fragile. Every time we change any modeling, many of the stats may change.

Using regex we could omit the actual numbers from the output checking.

Fri, Feb 9, 10:52 AM
NoQ added a comment to D43131: [analyzer] Serialize statistics to plist when serialize-stats=true is set.

So are these present only on translation units that actually caused bug reports to appear?

Fri, Feb 9, 10:45 AM
NoQ added inline comments to D43131: [analyzer] Serialize statistics to plist when serialize-stats=true is set.
Fri, Feb 9, 10:43 AM

Thu, Feb 8

NoQ added dependencies for D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible.: D43062: [analyzer] Support CXXTemporaryObjectExpr constructors that have destructors., D42991: [analyzer] Use EvalCallOptions for destructors as well..
Thu, Feb 8, 5:40 PM
NoQ added a dependent revision for D42991: [analyzer] Use EvalCallOptions for destructors as well.: D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible..
Thu, Feb 8, 5:40 PM
NoQ added a dependent revision for D43062: [analyzer] Support CXXTemporaryObjectExpr constructors that have destructors.: D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible..
Thu, Feb 8, 5:40 PM
NoQ added inline comments to D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible..
Thu, Feb 8, 5:40 PM
NoQ created D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible..
Thu, Feb 8, 5:40 PM
NoQ accepted D43098: [analyzer] [tests] [NFC] Remove a fragile tightly-coupled component emulating parser output.

Yeah, this trick is nicer than the old one.

Thu, Feb 8, 4:15 PM
NoQ added a comment to D43074: [Analyzer] Fix a typo about `categories::MemoryError` in `MallocChecker.cpp`.

I guess this happened due to a race with rL301913. My bad anyways^^

Thu, Feb 8, 3:06 PM
NoQ accepted D43074: [Analyzer] Fix a typo about `categories::MemoryError` in `MallocChecker.cpp`.

Woohoo thanks nice.

Thu, Feb 8, 1:26 PM
NoQ added a comment to D42672: [CFG] [analyzer] Heavier CFGConstructor elements..

All right, so this change is indeed safe, i.e. no crashes or changes in analyzer behavior so far on my large codebase run. Will commit now.

Thu, Feb 8, 1:26 PM
NoQ accepted D42300: [Analyzer] Add PreStmt and PostStmt callbacks for OffsetOfExpr.

All right, i guess we already do have a pair of callbacks for IntegerLiteral and it doesn't hurt, especially because here they'd eventually be actually useful. I think it should land, just to make it easier to add the actual support later.

Thu, Feb 8, 12:57 PM
NoQ updated the diff for D43062: [analyzer] Support CXXTemporaryObjectExpr constructors that have destructors..

Add a simple test for a class with no destructor.

Thu, Feb 8, 12:51 PM
NoQ accepted D43031: [analyzer] [tests] Test different projects concurrently.

Ew, i need to look at stuff more carefully :) Anyway, it still looks good.

Thu, Feb 8, 12:13 PM

Wed, Feb 7

NoQ added a dependent revision for D42876: [analyzer] Support returning objects by value.: D43056: [CFG] [analyzer] Add construction context for CXXBindTemporaryExpr..
Wed, Feb 7, 8:08 PM
NoQ added a dependency for D43056: [CFG] [analyzer] Add construction context for CXXBindTemporaryExpr.: D42876: [analyzer] Support returning objects by value..
Wed, Feb 7, 8:08 PM
NoQ added a dependency for D43062: [analyzer] Support CXXTemporaryObjectExpr constructors that have destructors.: D43056: [CFG] [analyzer] Add construction context for CXXBindTemporaryExpr..
Wed, Feb 7, 8:04 PM
NoQ added a dependent revision for D43056: [CFG] [analyzer] Add construction context for CXXBindTemporaryExpr.: D43062: [analyzer] Support CXXTemporaryObjectExpr constructors that have destructors..
Wed, Feb 7, 8:04 PM