dcoughlin (Devin Coughlin)
User

Projects

User does not belong to any projects.

User Details

User Since
May 26 2015, 5:42 PM (159 w, 6 d)

Recent Activity

Tue, Jun 12

dcoughlin added a comment to D48110: [analyzer] [WIP] Checker for detecting that the function was called with more function arguments than it could take.

This seems like a useful checker!

Tue, Jun 12, 11:05 PM

May 2 2018

dcoughlin added a comment to D46159: [clang-tidy] Add a flag to enable alpha checkers.

Do you have some better choices?

I could do -allow-alpha-checks. What do you think?

May 2 2018, 1:45 PM

Apr 27 2018

dcoughlin added a comment to D46159: [clang-tidy] Add a flag to enable alpha checkers.

Note that it is completely off by default, and is not listed in documentation, clang-tidy --help,
so one would have to know of the existence of that hidden flag first, and only then when that flag is passed, those alpha checks could be enabled.

Apr 27 2018, 11:24 AM
dcoughlin added a comment to D46159: [clang-tidy] Add a flag to enable alpha checkers.

I'm curious about the use case for this, since I don't think it ever makes sense to run all the alpha checks at once. These checks are typically a work in progress (they're not finished yet) and often have false positives that haven't been fixed yet. Often they don't handle all facets of the language and can crash on unhandled constructs. While it makes sense to enable a specific alpha check (or even a set of related alpha checks) when developing these checks or when evaluating whether they are ready to graduate into non-alpha it seems to me that running all of them isn't useful, ever.

Apr 27 2018, 10:55 AM

Apr 22 2018

dcoughlin added reviewers for D45718: [analyzer] Allow registering custom statically-linked analyzer checkers: NoQ, xazax.hun, dcoughlin.
Apr 22 2018, 9:58 AM

Mar 29 2018

dcoughlin accepted D44722: [analyzer] Path-insensitive checker for writes into an auto-releasing pointer from the wrong pool.

Looks good with updated diagnostic text and a test for non-ARC behavior.

Mar 29 2018, 10:59 AM

Mar 21 2018

dcoughlin accepted D44341: [analyzer] Trust _Nonnull annotations for system framework.

Looks good to me.

Mar 21 2018, 9:39 PM
dcoughlin accepted D44653: [analyzer] Extend GCDAntipatternChecker to match group_enter/group_leave pattern.

Looks good to me!

Mar 21 2018, 9:34 PM
dcoughlin accepted D44597: [CFG] [analyzer] Add C++17-specific variable and return value construction contexts..

LGTM. I really appreciate the extra documentation you've added, too.

Mar 21 2018, 9:31 PM

Mar 14 2018

dcoughlin added inline comments to D44341: [analyzer] Trust _Nonnull annotations for system framework.
Mar 14 2018, 11:27 AM
dcoughlin accepted D44409: [analyzer] Fix crashes in RetainCountChecker when underlying region is not a var.

Looks good. It is nice to see this fixed. There is one testing nit inline

Mar 14 2018, 10:56 AM

Mar 9 2018

dcoughlin accepted D44228: [analyzer] Move the GCDAsyncSemaphoreChecker to optin.performance.

LGTM, but I do think Artem is right that we should push the user in the right direction here. There is a suggested addendum inline.

Mar 9 2018, 2:48 PM
dcoughlin added a comment to D44228: [analyzer] Move the GCDAsyncSemaphoreChecker to optin.performance.

I put some post-commit comments in https://reviews.llvm.org/D44059 about diagnostic text and naming that you should address as part of graduating this out of alpha.

Mar 9 2018, 10:31 AM
dcoughlin added a comment to D44059: [analyzer] AST-matching checker to detect global central dispatch performance anti-pattern.

This looks good. Some minor post-commit review inline.

Mar 9 2018, 10:21 AM

Mar 5 2018

dcoughlin accepted D44124: [analyzer] Support destruction and lifetime-extension of inlined function return values..

LGTM!

Mar 5 2018, 5:12 PM
dcoughlin accepted D44120: [CFG] [analyzer] Heavier CFGCXXRecordTypedCall elements..

This looks good to me. I'm not super happy with the name "CFGValueTypedCall" since it doesn't make it obvious that is reflects "a function call that returns a C++ object by value."

Mar 5 2018, 5:05 PM

Feb 28 2018

dcoughlin accepted D43804: [analyzer] Enable cfg-temporary-dtors by default?.

Yay! This looks good to me.

Feb 28 2018, 4:48 PM

Feb 27 2018

dcoughlin accepted D30691: [analyzer] Support for naive cross translational unit analysis.

Thanks Gabor, this looks good to me. Please commit!

Feb 27 2018, 8:13 PM
dcoughlin accepted D43798: [analyzer] UndefinedAssignment: Fix warning message on implicit copy/move constructors..

LGTM. I have a suggestion for a slight modification of the diagnostic text inline.

Feb 27 2018, 1:50 PM

Feb 25 2018

dcoughlin committed rL326062: [www] Update link to analyzer's "Building a Checker in 24 hours" video.
[www] Update link to analyzer's "Building a Checker in 24 hours" video
Feb 25 2018, 4:42 PM
dcoughlin committed rC326062: [www] Update link to analyzer's "Building a Checker in 24 hours" video.
[www] Update link to analyzer's "Building a Checker in 24 hours" video
Feb 25 2018, 4:42 PM

Feb 24 2018

dcoughlin accepted D43714: [analyzer] Don't do anything when trivial-copying an empty class object..

LGTM.

Feb 24 2018, 12:02 PM
dcoughlin accepted D43689: [analyzer] Disable constructor inlining when lifetime extension through fields occurs..
Feb 24 2018, 11:08 AM
dcoughlin added inline comments to D43666: [analyzer] When constructing a temporary without construction context, track it for destruction anyway..
Feb 24 2018, 10:48 AM
dcoughlin added a comment to D43421: [analyzer] Do not analyze bison-generated files.

Actually, after posting this I've realized that this behavior is somewhat inconsistent, and we should still produce an empty plist when requested to: just as we do for files with no generated reports.

Actually, after thinking about this some more, the answer is those plists should not be produced, as they are not produced in similar cases (disableAllChecks is set, etc).

Feb 24 2018, 9:02 AM
dcoughlin added a comment to D43218: [analyzer] Quickfix: do not overflow in calculating offset in RegionManager.

...turns out the previous version had a bug. Re-introduced logging, as I've needed that for the second time for debugging.

Feb 24 2018, 8:58 AM

Feb 20 2018

dcoughlin accepted D43483: [CFG] [analyzer] Add construction context when the constructor is on a branch of a ternary operator.

This seems reasonable to me.

Feb 20 2018, 9:03 PM
dcoughlin accepted D43481: [CFG] [analyzer] Add construction context when the constructor is being no-op-casted to a const value type..
Feb 20 2018, 8:52 PM
dcoughlin accepted D43480: [CFG] [analyzer] Add construction context when the constructor is treated like a functional cast..
Feb 20 2018, 8:50 PM
dcoughlin accepted D43477: [CFG] [analyzer] Add MaterializeTemporaryExpr into the construction context..

LGTM.

Feb 20 2018, 8:44 PM
dcoughlin accepted D43428: [CFG] [analyzer] NFC: Allow more complicated construction contexts..

It seems kind of sketchy to me that we're recursing over an expression to find construction contexts and then later doing it again for sub-expressions. I guess there is precedent here with VisitForTemporaryDtors(), but we should think about whether there is a better way to do this.

Feb 20 2018, 8:39 PM
dcoughlin accepted D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases..

Looks good to me. I have a comment about simplifying createTemporaryRegionIfNeeded() (if possible) inline.

Feb 20 2018, 7:56 PM
dcoughlin added a comment to D43533: [CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases..

This looks great to me Artem! I'll be very curious to see how you extend this to handle initializer lists.

Feb 20 2018, 7:08 PM
dcoughlin accepted D43533: [CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases..
Feb 20 2018, 7:08 PM

Feb 15 2018

dcoughlin committed rL325314: Add static analyzer projects for GSoC 2018 to the open projects page.
Add static analyzer projects for GSoC 2018 to the open projects page
Feb 15 2018, 8:13 PM

Feb 14 2018

dcoughlin added a comment to D43218: [analyzer] Quickfix: do not overflow in calculating offset in RegionManager.

We have a comment documenting the decision. Adding a printf doesn't seem like it would help very much here and adds untested code. I don't think we should have a policy of adding these printfs on every early return. Since these printfs make it harder to read the code and are duplicated with the comments, I think we should avoid adding one until we know it is actually generally useful.

Feb 14 2018, 11:15 AM

Feb 12 2018

dcoughlin added inline comments to D43218: [analyzer] Quickfix: do not overflow in calculating offset in RegionManager.
Feb 12 2018, 9:39 PM

Feb 9 2018

dcoughlin added a comment to D43138: [analyzer] Update the documentation to show the command line option.

We don't consider --analyze to be public UI, which is why it is not documented. The supported interface to the analyzer is scan-build.

Feb 9 2018, 5:56 PM

Feb 8 2018

dcoughlin added inline comments to D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible..
Feb 8 2018, 6:30 PM

Feb 7 2018

dcoughlin accepted D42779: [analyzer] NFC: Make sure we don't ever inline the constructor for which the temporary destructor is noreturn and missing..

This looks good to me, but I think you should include your explanatory comments in the commit message to the comment itself to help future violators of the assertion out!

Feb 7 2018, 3:47 PM

Feb 2 2018

dcoughlin accepted D42672: [CFG] [analyzer] Heavier CFGConstructor elements..

This looks good to me, although as I noted inline I think both the name and the comment for VisitForConstructionContext() are confusing. If you can be more precise I think it would help future maintainers.

Feb 2 2018, 1:10 PM

Jan 31 2018

dcoughlin added a comment to D42777: [analyzer] Fix yet-another-crash in body-farming std::call_once.

This seems reasonable to me.

Jan 31 2018, 11:50 PM
dcoughlin added a comment to D42699: [CFG] [analyzer] Add construction context for DeclStmt..

It is nice to see how cleanly this slides in.

Jan 31 2018, 11:47 PM
dcoughlin added inline comments to D42700: [CFG] [analyzer] Add construction context for CXXCtorInitializer..
Jan 31 2018, 11:47 PM
dcoughlin added a comment to D42672: [CFG] [analyzer] Heavier CFGConstructor elements..

I think it will be great to have a more explicit representation of construction in the CFG! Comments in line.

Jan 31 2018, 11:35 PM
dcoughlin accepted D42457: [analyzer] Don't communicate evaluation failures through memregion hierarchy..

This looks good to me. However, I do think you should take George's suggestion to have makeZeroElementRegion() have a boolean out parameter rather than a EvalCallOptions out parameter. This avoids unnecessarily coupling of makeZeroElementRegion() and EvalallOptions. You will need to make the boolean fields not bitfields (since we can't take a bitfield's address in C++). But EvalCallOptions is only on the stack (and we can pass it by const reference) so I don't think the increased size is something we should worry about.

Jan 31 2018, 6:17 PM

Jan 30 2018

dcoughlin accepted D42456: [analyzer] Fix detection of function-like macros.

Looks good to me.

Jan 30 2018, 3:20 PM

Jan 24 2018

dcoughlin added a comment to D42269: [analyzer] Show full analyzer invocation for reproducibility in HTML reports.

Remember that html reports are not just used by analyzer developers but also by end users. For end users, the clang invocation is an implementation detail of the analyzer and not part of its user model. We shouldn't display this to users.

Jan 24 2018, 9:04 PM

Jan 21 2018

dcoughlin added inline comments to D41384: [analyzer] Suppress false positive warnings form security.insecureAPI.strcpy.
Jan 21 2018, 2:19 PM
dcoughlin accepted D41816: [analyzer] Model and check unrepresentable left shifts.

Looks good to me, thanks!

Jan 21 2018, 1:48 PM
dcoughlin added a comment to D42266: [analyzer] Prevent AnalyzerStatsChecker from crash.

This seems reasonable.

Jan 21 2018, 1:43 PM

Jan 20 2018

dcoughlin committed rL323052: [analyzer] Provide a check name when MallocChecker enables CStringChecker.
[analyzer] Provide a check name when MallocChecker enables CStringChecker
Jan 20 2018, 3:13 PM
dcoughlin committed rC323052: [analyzer] Provide a check name when MallocChecker enables CStringChecker.
[analyzer] Provide a check name when MallocChecker enables CStringChecker
Jan 20 2018, 3:12 PM

Jan 16 2018

dcoughlin added inline comments to D41378: [analyzer] support a mode to only show relevant lines in HTML diagnostics.
Jan 16 2018, 6:04 PM

Jan 13 2018

dcoughlin added a comment to D41378: [analyzer] support a mode to only show relevant lines in HTML diagnostics.

Thanks! This is quite nice.

Jan 13 2018, 11:55 AM

Jan 12 2018

dcoughlin added a comment to D41816: [analyzer] Model and check unrepresentable left shifts.

The diagnostic text looks to me, but I do have a comment about the nested 'if' inline.

Jan 12 2018, 6:34 PM
dcoughlin accepted D42015: [analyzer] NFC: RetainCount: Don't dump() regions to the user..

LGTM. Thanks for fixing this.

Jan 12 2018, 5:52 PM
dcoughlin accepted D41881: [analyzer] Flag bcmp, bcopy and bzero as obsolete.

Thanks for adding these! This looks good to me. Do you have commit access, or do you need someone to commit this?

Jan 12 2018, 5:50 PM
dcoughlin accepted D41797: [analyzer] Suppress escape of this-pointer during construction..

LGTM!

Jan 12 2018, 5:09 PM
dcoughlin accepted D41934: [analyzer] Fix CXXNewExpr callback order..

Looks great. It is nice to have this fixed and cleaned up!

Jan 12 2018, 5:04 PM
dcoughlin accepted D41266: [analyzer] With c++-allocator-inlining, fix memory space for operator new pointers..

LGTM as well.

Jan 12 2018, 4:59 PM
dcoughlin accepted D41250: [analyzer] Model implied cast around operator new()..

This looks good to me, as well.

Jan 12 2018, 4:55 PM
dcoughlin accepted D40560: [analyzer] Get construction into `operator new` running in simple cases..

LGTM with the TODO and the test case I requested inline.

Jan 12 2018, 4:48 PM
dcoughlin accepted D41935: [analyzer] NFC: Mark default constructors for ProgramPoints..

LGTM!

Jan 12 2018, 2:15 PM

Jan 9 2018

dcoughlin added a comment to D41848: [analyzer] mark returns of functions where the region passed as parameter was not initialized.

You'll also probably want to handle when the region of interest is a sub region of 'this'/'self'.
If we find that too many notes are being emitted (once you handle 'self' I think that will be a real possibility) you may want to add a heuristic that only displays the notes when a function has control flow or when some other path may write to the region of interest. (This last could be done with a simple, separate, flow-insensitive syntactic analysis).

I don't think that's a good idea. In fact, I think that's extending this patch to C++/Obj-CPP is already a stretch.

The pattern declare-but-not-initialize-and-pass-address-to-a-func occurs almost exclusively in C,
as in C++ a default constructor is usually invoked (and structs in general are populated using a constructor).

Jan 9 2018, 6:15 PM

Jan 8 2018

dcoughlin added a comment to D41378: [analyzer] support a mode to only show relevant lines in HTML diagnostics.

@dcoughlin your high-level comment makes sense, I wanted a least intrusive change.
What about just adding a metadata field to PathDiagnostic?

Jan 8 2018, 9:43 PM
dcoughlin added a comment to D41848: [analyzer] mark returns of functions where the region passed as parameter was not initialized.

This is great!

Jan 8 2018, 8:59 PM
dcoughlin added inline comments to D41266: [analyzer] With c++-allocator-inlining, fix memory space for operator new pointers..
Jan 8 2018, 6:31 PM
dcoughlin accepted D41800: [analyzer] Use a custom program point for the check::NewAllocator callback..
Jan 8 2018, 6:22 PM
dcoughlin added a comment to D41408: [analyzer] NFC: Fix nothrow operator new definition in a test..

LGTM as well.

Jan 8 2018, 6:12 PM
dcoughlin accepted D41406: [analyzer] Add a new checker callback, check::NewAllocator..

Looks good to me with some minor nits inside (and also a request to consider factoring out common code).

Jan 8 2018, 6:10 PM
dcoughlin added inline comments to D41790: [analyzer] [NFC] minor FindLastStoreBRVisitor refactoring.
Jan 8 2018, 5:21 PM
dcoughlin accepted D41749: [analyzer] suppress nullability inference from a macro when result is used in another macro.

Yes, this looks great!

Jan 8 2018, 4:58 PM
dcoughlin added a comment to D41409: [analyzer] Fix intermediate diagnostics on paths that go through operator new()..

LGTM as well.

Jan 8 2018, 2:43 PM
dcoughlin added a comment to D41378: [analyzer] support a mode to only show relevant lines in HTML diagnostics.

At a high level it seems pretty weird that the relevant lines for a diagnostic is represented a path diagnostic piece. This information is not a piece of the path; rather it is metadata about the entire path. It seems to me like this would be better represented as part of the path diagnostic itself and not pieces.

Jan 8 2018, 10:19 AM

Dec 21 2017

dcoughlin accepted D41478: [analyzer] Fix zero-initialization of stack VLAs under ARC..

LGTM. The tests are great!!

Dec 21 2017, 10:24 AM

Dec 20 2017

dcoughlin accepted D41414: [analyzer] Add keyboard j/k navigation to HTML reports.

Comments inline.

Dec 20 2017, 8:26 PM

Dec 18 2017

dcoughlin accepted D40939: [analyzer] NFC: Avoid element regions of void type..

Looks good to me, although I have a super nit about wording in a comment.

Dec 18 2017, 10:45 AM

Dec 17 2017

dcoughlin accepted D41253: [analyzer] WIP: trackNullOrUndefValue: track last store to symbolic pointers..
Dec 17 2017, 5:35 PM
dcoughlin accepted D41254: [analyzer] WIP: trackNullOrUndefValue: peel off ParenImpCasts before tracking..
Dec 17 2017, 5:34 PM
dcoughlin accepted D41258: [analyzer] trackNullOrUndefValue: deduplicate path pieces for each node..

Looks good to me.

Dec 17 2017, 5:25 PM

Dec 14 2017

dcoughlin added a comment to D30691: [analyzer] Support for naive cross translational unit analysis.

Some comments on the C++ inline.

Dec 14 2017, 9:32 PM
dcoughlin added a comment to D40809: [WIP] [analyzer] Dump counterexample traces as C programs.
In D40809#954890, @NoQ wrote:

One possibility is to turn this into a debug checker similar to debug.ViewExplodedGraph. That checker registers for a checkEndAnalysis() callback and traverses the node graph (see DebugCheckers.cpp). Can you do the same here? It doesn't look like you really need this to be a BugReporterVisitor -- and making it a debug checker would avoid outputting multiple copies for each diagnostic consumer.

These prints are only for actual bugs, not for the whole graph. Even if we identify error nodes in the final graph, we're unlikely to identify which of them are suppressed by visitors.

Dec 14 2017, 2:43 PM

Dec 13 2017

dcoughlin accepted D40841: [analyzer] Fix a crash on C++17 AST for non-trivial construction into a trivial brace initializer..

This looks good to me for a quick fix that we plan to address in a more principled fashion later.

Dec 13 2017, 9:54 PM
dcoughlin added a comment to D40809: [WIP] [analyzer] Dump counterexample traces as C programs.

This is seems like a very useful visualization, *especially* for loops. Can we this patch get it into a state where it can be committed and used for debugging purposes?

Dec 13 2017, 9:07 PM

Dec 11 2017

dcoughlin added a comment to D40715: [analyser] different.LabelInsideSwitch checker implementation.

Thanks for looking into this!

Dec 11 2017, 1:44 PM · Restricted Project

Dec 4 2017

dcoughlin accepted D40793: [analyzer] Improve SymbolicRegion::dump() for heap pointers..

LGTM. Ship it!

Dec 4 2017, 7:39 PM
dcoughlin accepted D40584: [analyzer] do not crash on subscripts into ObjC properties.

Minor nits inline. Other than that, looks great to me.

Dec 4 2017, 6:16 PM

Dec 3 2017

dcoughlin committed rC319638: [analyzer] Don't treat lambda-captures float constexprs as undefined.
[analyzer] Don't treat lambda-captures float constexprs as undefined
Dec 3 2017, 8:47 PM
dcoughlin committed rL319638: [analyzer] Don't treat lambda-captures float constexprs as undefined.
[analyzer] Don't treat lambda-captures float constexprs as undefined
Dec 3 2017, 8:47 PM

Dec 1 2017

dcoughlin added inline comments to D40625: Harmonizing attribute GNU/C++ spellings.
Dec 1 2017, 4:21 PM
dcoughlin updated subscribers of D40625: Harmonizing attribute GNU/C++ spellings.
Dec 1 2017, 8:22 AM

Nov 30 2017

dcoughlin added inline comments to D40584: [analyzer] do not crash on subscripts into ObjC properties.
Nov 30 2017, 9:08 PM

Nov 29 2017

dcoughlin accepted D40463: [analyzer] Fix false negative on post-increment of uninitialized variable..

Thanks, this looks good to me!

Nov 29 2017, 3:03 PM · Restricted Project
dcoughlin committed rC319333: [analyzer] Fix unreachable creating PathDiagnosticLocation with widen-loops=true.
[analyzer] Fix unreachable creating PathDiagnosticLocation with widen-loops=true
Nov 29 2017, 10:26 AM
dcoughlin committed rL319333: [analyzer] Fix unreachable creating PathDiagnosticLocation with widen-loops=true.
[analyzer] Fix unreachable creating PathDiagnosticLocation with widen-loops=true
Nov 29 2017, 10:26 AM
dcoughlin closed D37187: [Analyzer] Fix Bug 25609 - Assertion UNREACHABLE: 'Unexpected ProgramPoint' with widen-loops=true by committing rL319333: [analyzer] Fix unreachable creating PathDiagnosticLocation with widen-loops=true.
Nov 29 2017, 10:26 AM
dcoughlin added inline comments to D40463: [analyzer] Fix false negative on post-increment of uninitialized variable..
Nov 29 2017, 10:05 AM · Restricted Project

Nov 28 2017

dcoughlin added a comment to D40463: [analyzer] Fix false negative on post-increment of uninitialized variable..

Thanks for tackling this! There is one major comment inline (you are generating an extra node that you shouldn't, which is causing the analyzer to explore code it shouldn't) and a suggestion for simpler diagnostic text.

Nov 28 2017, 12:02 PM · Restricted Project

Nov 27 2017

dcoughlin accepted D37187: [Analyzer] Fix Bug 25609 - Assertion UNREACHABLE: 'Unexpected ProgramPoint' with widen-loops=true.

LGTM, but as noted inline you should update the new llvm_unreachable() to have a more descriptive error message.

Nov 27 2017, 8:33 AM