Page MenuHomePhabricator

MTC (Henry Wong)
Engineering

Projects

User does not belong to any projects.

User Details

User Since
Jul 19 2017, 4:18 AM (73 w, 6 d)

Recent Activity

Sun, Dec 9

MTC added a comment to D55388: [analyzer] MoveChecker Pt.8: Add checks for dereferencing a smart pointer after move..

Hm. I wonder if it would also make sense to model e.g. the get method to return nullptr for moved from smart ptrs. This could help null dereference checker and also aid false path prunning.

Sun, Dec 9, 6:45 AM

Thu, Dec 6

MTC accepted D55388: [analyzer] MoveChecker Pt.8: Add checks for dereferencing a smart pointer after move..
Thu, Dec 6, 10:52 PM

Tue, Dec 4

MTC accepted D55226: [Fix][StaticAnalyzer] Bug 39792 - False positive on strcpy targeting struct member.
Tue, Dec 4, 4:12 AM
MTC added a comment to D55226: [Fix][StaticAnalyzer] Bug 39792 - False positive on strcpy targeting struct member.

Please add more context using the -U option, like git diff -U99999 ....

Tue, Dec 4, 4:12 AM

Mon, Nov 26

MTC added a comment to D54878: [clangd] NFC: Prefer `isa<>` to `dyn_cast<>` to do the checking..

But given that isa<> are still better than dyn_cast<>, this change might still be worth landing.

We can land this change this time or do the cleaning job in other patches in the future, it's all up to you guys, the active clangd contributors.

Mon, Nov 26, 5:51 AM
MTC retitled D54878: [clangd] NFC: Prefer `isa<>` to `dyn_cast<>` to do the checking. from [clangd] NFC: Eliminate the unused variable warning. to [clangd] NFC: Prefer `isa<>` to `dyn_cast<>` to do the checking..
Mon, Nov 26, 5:43 AM
MTC updated the diff for D54878: [clangd] NFC: Prefer `isa<>` to `dyn_cast<>` to do the checking..

Use more concise form.

Mon, Nov 26, 5:35 AM

Sun, Nov 25

MTC added inline comments to D54878: [clangd] NFC: Prefer `isa<>` to `dyn_cast<>` to do the checking..
Sun, Nov 25, 7:10 PM
MTC created D54878: [clangd] NFC: Prefer `isa<>` to `dyn_cast<>` to do the checking..
Sun, Nov 25, 6:42 AM

Sat, Nov 24

MTC added inline comments to D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions.
Sat, Nov 24, 7:34 AM

Thu, Nov 22

MTC added a comment to D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions.

Thank you for doing the cleaning that no one else is interested in! Comments is inline below.

Thu, Nov 22, 8:36 AM

Nov 16 2018

MTC added a comment to D54560: [analyzer] MoveChecker Pt.3: Improve warning messages a bit..

The "moved-from" terminology we adopt here still feels a bit weird to me, but i don't have a better suggestion, so i just removed the single-quotes so that to at least feel proud about what we have.

I am personally fine with this terminology, this checker corresponds to the cert rule EXP63-CPP. Do not rely on the value of a moved-from object, and moved from is also used in many places in CppCoreGuidelines.

Nov 16 2018, 11:44 PM
MTC closed D54528: [lldb] NFC: Remove the extra ';'.
Nov 16 2018, 5:04 AM

Nov 14 2018

MTC updated the summary of D54528: [lldb] NFC: Remove the extra ';'.
Nov 14 2018, 8:06 AM
MTC set the repository for D54528: [lldb] NFC: Remove the extra ';' to rLLDB LLDB.
Nov 14 2018, 8:03 AM
MTC created D54528: [lldb] NFC: Remove the extra ';'.
Nov 14 2018, 8:02 AM

Nov 12 2018

MTC added a comment to D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp.

I'm totally fine with this patch personally. However I am not familiar with this part, so can't give substantial help :).

Nov 12 2018, 5:47 AM
MTC added inline comments to D54138: [WebAssembly] Read prefixed opcodes as ULEB128s.
Nov 12 2018, 4:34 AM

Nov 1 2018

MTC added a comment to D52949: [Diagnostics] Implement -Wsizeof-pointer-div .

Sorry for the long delay for this patch! The implementation is fine for me. However, I'm the newbie on clang diagnostics and have no further insight into this checker. @aaron.ballman may have more valuable insights into this checker.

Nov 1 2018, 4:04 AM

Oct 29 2018

MTC added a comment to D53856: [analyzer] Put llvm.Conventions back in alpha.

In addition, clang/lib/StaticAnalyzer/README.txt and other related docs in clang/lib/docs/analyzer are also out of date.

Oct 29 2018, 7:47 PM

Oct 23 2018

MTC added inline comments to D53543: [analyzer] MallocChecker: pr39348: Realize that sized delete isn't a custom delete..
Oct 23 2018, 7:10 PM
MTC added a comment to D52949: [Diagnostics] Implement -Wsizeof-pointer-div .

Second thought, I don't think we should recommend std::size here (maybe it should be okay for clang static analyzers)

uint32_t data[] = {10, 20, 30, 40};
len = sizeof(data)/sizeof(*data); // "warn" on valid code to recommend std::size? I dont agree with such behaviour.

Oct 23 2018, 9:19 AM
MTC added inline comments to D53543: [analyzer] MallocChecker: pr39348: Realize that sized delete isn't a custom delete..
Oct 23 2018, 6:53 AM

Oct 10 2018

MTC updated subscribers of D53024: [analyzer][www] Add more open projects.

Also, a lot of items on this list is outdated, but I joined the project relatively recently, so I'm not sure what's the state on all of them.

Oct 10 2018, 11:10 PM

Oct 9 2018

MTC added a comment to D52984: [analyzer] Checker reviewer's checklist.

Greate idea! If we can enrich this list, it will not only help the reviewer, but also help beginners, like me, avoid some pitfalls when developing a new checker.

Oct 9 2018, 5:29 AM · Restricted Project

Aug 29 2018

MTC accepted D51390: [analyzer] CallDescription: Improve array management..

Thank you for digging in to delete that meaningless constructor, NoQ! And sorry for my technology debt : ).

Aug 29 2018, 7:20 PM

Aug 23 2018

MTC added a comment to D48027: [analyzer] Improve `CallDescription` to handle c++ method..
In D48027#1209844, @NoQ wrote:

So i believe that one of the important remaining problems with CallDescription is to teach it to discriminate between global functions and methods. We can do it in a number of ways:

  1. Make a special sub-class for CallDescription depending on the sort of the function (too verbose),
  2. Tag all items in the list as "record" vs. "namespace" (too many tags),
  3. Dunno, tons of other boring and verbose approaches,
  4. Change our PreCall/PostCall callbacks to callback templates that let allow the user subscribe to specific sub-classes of CallEvent similarly to how he can subscribe to different kinds of statements in PreStmt<Class>/PostStmt<Class>, and then the user doesn't need to write any code to check it dynamically.
Aug 23 2018, 7:13 PM

Aug 22 2018

MTC updated the summary of D48027: [analyzer] Improve `CallDescription` to handle c++ method..
Aug 22 2018, 6:30 AM
MTC added a comment to D48027: [analyzer] Improve `CallDescription` to handle c++ method..

Sorry for the delay, I think this is OK to commit.
As a possible improvement, I can imagine making it slightly stricter, e.g. you could only skip QualifiedNames for inline namespaces and the beginning. Maybe add support for staring with "" or :: to even disable skipping namespaces at the beginning?
But these are just nice to have features, I am perfectly ok with not having them or doing it in a followup patch.

Aug 22 2018, 6:29 AM

Aug 21 2018

MTC added a comment to D48027: [analyzer] Improve `CallDescription` to handle c++ method..

@xazax.hun What do you think?

Aug 21 2018, 4:23 AM

Aug 17 2018

MTC added a comment to D48027: [analyzer] Improve `CallDescription` to handle c++ method..

Generally looks good, I only wonder if this works well with inline namespaces. Could you test? Probably it will.

Aug 17 2018, 5:38 AM
MTC updated the diff for D48027: [analyzer] Improve `CallDescription` to handle c++ method..
  • rebase
  • Since we have enhanced the ability of CallDescription, remove the helper method isCalledOnStringObject().
Aug 17 2018, 5:24 AM

Aug 13 2018

MTC added a comment to D48027: [analyzer] Improve `CallDescription` to handle c++ method..

kindly ping!

Aug 13 2018, 7:28 AM

Aug 7 2018

MTC created D50382: [analyzer] Fix a typo in `RegionStore.txt`..
Aug 7 2018, 5:25 AM

Jul 26 2018

MTC updated the diff for D48027: [analyzer] Improve `CallDescription` to handle c++ method..

@xazax.hun Thanks for your tips! After some investigation, MatchFinder::match just traverse one ASTNode, that means match(namedDecl(HasNameMatcher())) and match(namedDecl(matchesName())) both not traverse children. So there are three ways to match the specified AST node.

Jul 26 2018, 9:36 AM

Jul 4 2018

MTC added a comment to D48027: [analyzer] Improve `CallDescription` to handle c++ method..

Thanks for your review, NoQ!

Jul 4 2018, 7:15 AM

Jun 29 2018

MTC added a comment to D48027: [analyzer] Improve `CallDescription` to handle c++ method..

kindly ping!

Jun 29 2018, 7:30 PM

Jun 25 2018

MTC updated the diff for D48027: [analyzer] Improve `CallDescription` to handle c++ method..

Sorry for the long long delay, I was on the Dragon Boat Festival a few days ago.

Jun 25 2018, 8:59 AM

Jun 13 2018

MTC added inline comments to D48027: [analyzer] Improve `CallDescription` to handle c++ method..
Jun 13 2018, 7:21 PM
MTC added a reviewer for D48027: [analyzer] Improve `CallDescription` to handle c++ method.: rnkovacs.
Jun 13 2018, 7:49 AM
MTC updated the diff for D48027: [analyzer] Improve `CallDescription` to handle c++ method..
  • Use hasName matcher to match the qualified name.
Jun 13 2018, 7:48 AM

Jun 12 2018

MTC abandoned D47973: [ADT] Change the behavior of `StringRef::rsplit()` when the separator is not found..
Jun 12 2018, 5:53 AM
MTC added inline comments to D47616: [CFG] [analyzer] Explain copy elision through construction contexts..
Jun 12 2018, 12:00 AM

Jun 11 2018

MTC added a comment to D48027: [analyzer] Improve `CallDescription` to handle c++ method..

Having C++ support would be awesome!
Thanks for working on this!

While I do agree matching is not trivial with qualified names, this problem is already solved with AST matchers.

I wonder if using matchers or reusing the HasNameMatcher class would make this easier. That code path is already well tested and optimized.

Jun 11 2018, 7:13 PM
MTC updated the diff for D48027: [analyzer] Improve `CallDescription` to handle c++ method..

Remove useless header files for testing.

Jun 11 2018, 7:56 AM
MTC added a comment to D48027: [analyzer] Improve `CallDescription` to handle c++ method..

The implementation is not complicated, the difficulty is that there is no good way to get the qualified name without template arguments. For std::basic_string::c_str(), its qualified name may be std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::c_str, it is almost impossible for users to provide such a name. So one possible implementation is to use std, basic_string and c_str to match in the std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::c_str sequentially.

Jun 11 2018, 7:47 AM
MTC created D48027: [analyzer] Improve `CallDescription` to handle c++ method..
Jun 11 2018, 7:39 AM

Jun 9 2018

MTC added a comment to D47973: [ADT] Change the behavior of `StringRef::rsplit()` when the separator is not found..
In D47973#1127309, @vsk wrote:

Thanks for the patch. The StringRef change itself looks fine, but please make sure to update the callsites in-tree (e.g in llvm, clang, lldb etc).

Jun 9 2018, 7:27 AM

Jun 8 2018

MTC added a comment to D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available..

LGTM, @NoQ May have further feedback. Thanks!

Jun 8 2018, 11:55 PM
MTC created D47973: [ADT] Change the behavior of `StringRef::rsplit()` when the separator is not found..
Jun 8 2018, 10:07 PM
MTC added a comment to D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available..

I didn't test the code, but the code seems correct. Thanks!

Jun 8 2018, 7:31 AM

Jun 7 2018

MTC added inline comments to D47406: [ADT] Add `StringRef::rsplit(StringRef Separator)`..
Jun 7 2018, 7:35 PM
MTC added a comment to D47406: [ADT] Add `StringRef::rsplit(StringRef Separator)`..

rsplit(char) already exists. That said, another way to reuse code would be
to have rsplit(char) call the StringRef version

Jun 7 2018, 8:24 AM
MTC added a comment to D47406: [ADT] Add `StringRef::rsplit(StringRef Separator)`..
In D47406#1120355, @vsk wrote:

Hi @MTC, thanks for the patch.

With this patch, the logic for split would be duplicated 3 times now in StringRef.h. I think it'd be nice to reuse some code by moving the core logic into a private method of StringRef, say, splitAtIndex(size_t). Then, each (r)split(char | StringRef) can use the common helper.

Relatedly, an rsplit(char) overload seems to be missing here.

Jun 7 2018, 8:18 AM
MTC updated the diff for D47406: [ADT] Add `StringRef::rsplit(StringRef Separator)`..

This update consists of two parts

  • Make rsplit(char) reuse rsplit(StringRef).
  • Make split(char) reuse split(StringRef).
Jun 7 2018, 8:18 AM

Jun 4 2018

MTC added inline comments to D47658: [analyzer] Re-enable lifetime extension for temporaries without destructors and bring back static temporaries..
Jun 4 2018, 12:07 AM

Jun 3 2018

MTC added inline comments to D47616: [CFG] [analyzer] Explain copy elision through construction contexts..
Jun 3 2018, 11:41 PM
MTC added a comment to D47406: [ADT] Add `StringRef::rsplit(StringRef Separator)`..

ping.

Jun 3 2018, 7:14 AM

May 28 2018

MTC created D47451: [analyzer] Remove the redundant check about same state transition in `ArrayBoundCheckerV2.cpp`..
May 28 2018, 7:01 AM
MTC added inline comments to D47417: [analyzer] Add missing state transition in IteratorChecker.
May 28 2018, 1:59 AM

May 27 2018

MTC added inline comments to D47417: [analyzer] Add missing state transition in IteratorChecker.
May 27 2018, 7:08 PM

May 26 2018

MTC created D47406: [ADT] Add `StringRef::rsplit(StringRef Separator)`..
May 26 2018, 1:38 AM

May 21 2018

MTC added inline comments to D47135: [analyzer] A checker for dangling internal buffer pointers in C++.
May 21 2018, 6:28 AM

May 15 2018

MTC updated the diff for D44934: [analyzer] Improve the modeling of `memset()`..
  • According to NoQ's suggestion, use assumeZero() instead of isZeroConstant() to determine whether the value is 0.
  • Add test memset26_upper_UCHAR_MAX() and memset27_symbol()
  • Since memset( void *dest, int ch, size_t count) will converts the value ch to unsigned char, we call evalCast() accordingly.
May 15 2018, 7:07 AM
MTC added inline comments to D44934: [analyzer] Improve the modeling of `memset()`..
May 15 2018, 6:51 AM

May 10 2018

MTC added a comment to D44934: [analyzer] Improve the modeling of `memset()`..

ping.

May 10 2018, 8:13 AM

May 5 2018

MTC updated the diff for D44934: [analyzer] Improve the modeling of `memset()`..
  • Since there is no perfect way to handle the default binding of non-zero character, remove the default binding of non-zero character. Use bindDefaulrZero() instead of overwriteRegion() to bind the zero character.
  • Reuse assume() instead of isZeroConstant() to determine whether it is zero character. The purpose of this is to be able to set the string length when dealing with non-zero symbol character.
May 5 2018, 3:55 AM

May 4 2018

MTC added a comment to D44934: [analyzer] Improve the modeling of `memset()`..
In D44934#1088771, @NoQ wrote:

Hmm, ok, it seems that i've just changed the API in D46368, and i should have thought about this use case. Well, at least i have some background understanding of these problems now. Sorry for not keeping eye on this problem.

In D44934#1051002, @NoQ wrote:

Why do you need separate code for null and non-null character? The function's semantics doesn't seem to care.

I guess i can answer myself here:

int32_t x;
memset(&x, 1, sizeof(int32_t));
clang_analyzer_eval(x == 0x1010101); // should be TRUE

I really doubt that we support this case.

So, yeah, zero character is indeed special.

Thank you, Artem! I did not consider this common situation. This patch does not really support this situation, in this patch the value of x will be 1, it's not correct!

May 4 2018, 9:31 PM

May 3 2018

MTC updated the diff for D44934: [analyzer] Improve the modeling of `memset()`..
  • fix typos
  • code refactoring, add auxiliary method memsetAux()
  • according to a.sidorin's suggestions, remove the useless state splitting.
  • make StoreManager::overwriteRegion() pure virtual
May 3 2018, 7:41 AM

May 2 2018

MTC added a comment to D44934: [analyzer] Improve the modeling of `memset()`..

Sorry for the long delay, I have just finished my holiday.

May 2 2018, 5:58 AM

Apr 27 2018

MTC added a comment to D44934: [analyzer] Improve the modeling of `memset()`..

ping^2

Apr 27 2018, 12:36 AM

Apr 25 2018

MTC updated the diff for D46007: [analyzer] Add `TaintBugVisitor` to the ArrayBoundV2, DivideZero and VLASize..

Since BugReport::addVisitor() has checks for the null Visitor, remove the checks before BugReport->addVisitor().

Apr 25 2018, 5:31 AM
MTC added inline comments to D46007: [analyzer] Add `TaintBugVisitor` to the ArrayBoundV2, DivideZero and VLASize..
Apr 25 2018, 5:25 AM

Apr 24 2018

MTC created D46007: [analyzer] Add `TaintBugVisitor` to the ArrayBoundV2, DivideZero and VLASize..
Apr 24 2018, 6:15 AM

Apr 22 2018

MTC added a comment to D45682: [analyzer] Move `TaintBugVisitor` from `GenericTaintChecker.cpp` to `BugReporterVisitors.h`..

I'm new to the taint visitor, but I am quite confused by your change description.

and many checkers rely on it

How can other checkers rely on it if it's private to the taint checker?

Thanks for your review, george! TaintBugVisitor is an utility to add extra information to illustrate where the taint information originated from. There are several checkers use taint information, e.g. ArrayBoundCheckerV2.cpp, in some cases it will report a warning, like warning: Out of bound memory access (index is tainted). If TaintBugVisitor moves to BugReporterVisitors.h, ArrayBoundCheckerV2 can add extra notes like Taint originated here to the report by adding TaintBugVisitor.

Apr 22 2018, 6:55 AM

Apr 18 2018

MTC added a comment to D45774: [analyzer] cover more cases where a Loc can be bound to constants.

Test files for initialization missing? : )

Apr 18 2018, 7:35 PM

Apr 17 2018

MTC added a comment to D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call.

There is something that came up in my mind:

Consider a construct like this:

class A
{
  A()
  {
    memset(X, 0, 10 * sizeof(int));
  }

  int X[10];
};

I think it's worthy for a test case if this X is considered unitialised at the end of the constructor or not. (And if not, a ticket, or a fix for SA in a subpatch.)

Apr 17 2018, 4:11 AM

Apr 16 2018

MTC created D45682: [analyzer] Move `TaintBugVisitor` from `GenericTaintChecker.cpp` to `BugReporterVisitors.h`..
Apr 16 2018, 5:17 AM

Apr 14 2018

MTC added a comment to D45491: [analyzer] Do not invalidate the `this` pointer..
In D45491#1067852, @NoQ wrote:

Yeah, i think this makes sense, thanks! It feels a bit weird that we have to add it as an exception - i wonder if there are other exceptions that we need to make. Widening over the stack memory space should be a whitelist, not a blacklist, because we can easily enumerate all stack variables and see which of them can be modified at all from the loop. But until we have that, this looks like a reasonable workaround.

Apr 14 2018, 3:01 AM

Apr 13 2018

MTC added inline comments to D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call.
Apr 13 2018, 4:46 AM

Apr 11 2018

MTC updated the diff for D45491: [analyzer] Do not invalidate the `this` pointer..
  • Move the CXXThisRegion's check to LoopWidening.cpp
  • Use isa<CXXThisRegion>(R) instead of CXXThisRegion::classof(R).
Apr 11 2018, 8:59 AM
MTC added inline comments to D45491: [analyzer] Do not invalidate the `this` pointer..
Apr 11 2018, 6:43 AM
MTC added a comment to D45491: [analyzer] Do not invalidate the `this` pointer..

@MTC what happens for

this.j = 0;
for (int i=0; i<100; i++)
   this.j++;

?

Apr 11 2018, 6:33 AM

Apr 10 2018

MTC updated the summary of D45491: [analyzer] Do not invalidate the `this` pointer..
Apr 10 2018, 8:36 AM
MTC created D45491: [analyzer] Do not invalidate the `this` pointer..
Apr 10 2018, 8:36 AM

Apr 2 2018

MTC added a comment to D44934: [analyzer] Improve the modeling of `memset()`..

Kindly ping!

Apr 2 2018, 8:47 AM
MTC updated the diff for D44934: [analyzer] Improve the modeling of `memset()`..

Thank you for your reminding, I overlooked this point. However for non-concrete character, the symbol value, if we just invalidate the region, the constraint information of the non-concrete character will be lost. Do we need to consider this?

Apr 2 2018, 8:44 AM

Mar 30 2018

MTC updated the diff for D45086: [analyzer] Unroll the loop when it has a unsigned counter..

Fix typo, unsinged -> unsigned

Mar 30 2018, 8:40 PM
MTC created D45086: [analyzer] Unroll the loop when it has a unsigned counter..
Mar 30 2018, 6:36 AM
MTC updated the summary of D45086: [analyzer] Unroll the loop when it has a unsigned counter..
Mar 30 2018, 6:36 AM
MTC created D45081: [analyzer] Remove the unused method declaration in `ValistChecker.cpp`..
Mar 30 2018, 4:52 AM
MTC updated the diff for D44934: [analyzer] Improve the modeling of `memset()`..

According to @NoQ's suggestion, remove the duplicated code.

Mar 30 2018, 4:37 AM
MTC added a comment to D44934: [analyzer] Improve the modeling of `memset()`..

Thanks for your review, NoQ!

Mar 30 2018, 4:32 AM

Mar 27 2018

Herald added a reviewer for D18860: [analyzer] Fix the "Zombie symbols" issue.: george.karpenkov.
Mar 27 2018, 10:18 PM
MTC updated the summary of D44934: [analyzer] Improve the modeling of `memset()`..
Mar 27 2018, 7:48 AM
MTC created D44934: [analyzer] Improve the modeling of `memset()`..
Mar 27 2018, 7:39 AM

Mar 22 2018

Herald added a reviewer for D34260: [StaticAnalyzer] Completely unrolling specific loops with known bound option : george.karpenkov.
Mar 22 2018, 5:09 AM

Mar 21 2018

MTC updated subscribers of rC328067: Revert r326782 "[analyzer] CStringChecker.cpp: Remove the duplicated check..."..

Thank you for taking the time to pay attention to this problem, @NoQ. The reason for the test regression is that CheckBufferAccess() does not guarantee that CheckNonNull() must be called for the second buffer, see https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/CStringChecker.cpp#L385.

Mar 21 2018, 9:40 AM

Mar 19 2018

MTC updated the diff for D44606: [analyzer] Fix the crash in `IteratorChecker.cpp` when `SymbolConjured` has a null Stmt..

Add the comments as suggested by @szepet .

Mar 19 2018, 10:08 PM
MTC added a comment to D44557: [analyzer] CStringChecker.cpp - Code refactoring on bug report..
In D44557#1042357, @NoQ wrote:

Sorry, one moment, i'm seeing a few regressions after the previous refactoring but i didn't look at them closely yet to provide a reproducer. I'll get back to this.

Mar 19 2018, 6:58 PM