dcoughlin (Devin Coughlin)
User

Projects

User does not belong to any projects.

User Details

User Since
May 26 2015, 5:42 PM (138 w, 1 d)

Recent Activity

Yesterday

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

Sat, Jan 13

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

Thanks! This is quite nice.

Sat, Jan 13, 11:55 AM

Fri, Jan 12

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.

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

LGTM. Thanks for fixing this.

Fri, Jan 12, 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?

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

LGTM!

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

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

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

LGTM as well.

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

This looks good to me, as well.

Fri, Jan 12, 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.

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

LGTM!

Fri, Jan 12, 2:15 PM

Tue, Jan 9

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

Tue, Jan 9, 6:15 PM

Mon, Jan 8

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?

Mon, Jan 8, 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!

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

LGTM as well.

Mon, Jan 8, 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).

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

Yes, this looks great!

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

LGTM as well.

Mon, Jan 8, 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.

Mon, Jan 8, 10:19 AM

Thu, Dec 21

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

LGTM. The tests are great!!

Thu, Dec 21, 10:24 AM

Wed, Dec 20

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

Comments inline.

Wed, Dec 20, 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

Nov 25 2017

dcoughlin committed rL318979: [analyzer] Teach RetainCountChecker about CoreMedia APIs.
[analyzer] Teach RetainCountChecker about CoreMedia APIs
Nov 25 2017, 6:58 AM

Nov 24 2017

dcoughlin added a comment to D39965: [Analyzer] Split Critical Sections.

I don't think LLVM/Clang is a good project to evaluate this on, since it has very little multi-threaded code.

Nov 24 2017, 7:02 AM
dcoughlin added inline comments to D37187: [Analyzer] Fix Bug 25609 - Assertion UNREACHABLE: 'Unexpected ProgramPoint' with widen-loops=true.
Nov 24 2017, 6:57 AM

Nov 21 2017

dcoughlin added a comment to D40073: [Analyzer] Non-determinism: don't sort indirect goto LabelDecl's by addresses.

For clang itself I think we also use a stage-2 clang to build the same version of clang and make sure that it matches the stage-2 clang. This doesn't help for the analyzer though.

Nov 21 2017, 6:30 PM

Nov 20 2017

dcoughlin added a comment to D40073: [Analyzer] Non-determinism: don't sort indirect goto LabelDecl's by addresses.

Thanks! Do you have commit access, or do you need someone to commit this for you?

Nov 20 2017, 9:14 AM

Nov 18 2017

dcoughlin accepted D40073: [Analyzer] Non-determinism: don't sort indirect goto LabelDecl's by addresses.

Thanks for finding and fixing this!

Nov 18 2017, 5:35 PM
dcoughlin added inline comments to D37187: [Analyzer] Fix Bug 25609 - Assertion UNREACHABLE: 'Unexpected ProgramPoint' with widen-loops=true.
Nov 18 2017, 3:42 PM
dcoughlin added reviewers for D39965: [Analyzer] Split Critical Sections: xazax.hun, george.karpenkov.

Can you give examples of the kinds of bugs that this check is designed to find? I'm worried that "writing a variable in one critical section and then reading it in another" is too general of a pattern to warn on and will result in a lot of false positives.

Nov 18 2017, 11:57 AM

Nov 17 2017

dcoughlin accepted D39964: Change code owner for Clang Static Analyzer to Devin Coughlin..

LGTM! Thanks Anna!

Nov 17 2017, 10:21 AM

Nov 13 2017

dcoughlin committed rL318054: [analyzer] ObjCGenerics: Don't warn on cast conversions involving explicit cast.
[analyzer] ObjCGenerics: Don't warn on cast conversions involving explicit cast
Nov 13 2017, 9:37 AM
dcoughlin closed D39711: [analyzer] ObjCGenerics: Don't warn on cast conversions involving explicit cast by committing rL318054: [analyzer] ObjCGenerics: Don't warn on cast conversions involving explicit cast.
Nov 13 2017, 9:37 AM

Nov 10 2017

dcoughlin updated the diff for D39711: [analyzer] ObjCGenerics: Don't warn on cast conversions involving explicit cast.

Update to address Gabor's feedback and adopt "option 3". This changes the casting logic to always forget the specialized type arguments on an explicit cast regardless of whether it is an upcast/downcast or a cross cast.

Nov 10 2017, 4:44 PM
dcoughlin accepted D38801: [analyzer] In getSVal() API, disable auto-detection of void type as char type..
In D38801#910524, @NoQ wrote:

Also the overload between getSVal(Ex, LC) and getSVal(R, Ty), when they do completely different things, seems confusing.

Nov 10 2017, 11:21 AM
dcoughlin accepted D39803: [analyzer] pr34766: Fix a crash on explicit construction of std::initializer_list..

LGTM.

Nov 10 2017, 11:12 AM
dcoughlin accepted D39800: [analyzer] pr34404: Fix a crash on pointers to members in nested anonymous structures..

I suppose we could move the logic that constructs pointers to members for fields here (right now that logic is in ExprEngine::VisitUnaryOperator()'s handling of '&'). However, since the AST's DeclRefExpr for the field is marked as an lvalue this would be slightly odd -- we would have the value of an lvalue expression be a non-Loc.

Nov 10 2017, 11:00 AM

Nov 9 2017

dcoughlin accepted D39584: [analyzer] [NFC] another very minor ExprEngineC refactoring.

Yes, looks great. Thanks!

Nov 9 2017, 3:07 PM
dcoughlin accepted D39862: [analyzer] do not crash when trying to convert an APSInt to an unexpected type.

This seems reasonable to me. Please commit it. @NoQ can do a post-commit review and fix it up if he would rather address the issue differently.

Nov 9 2017, 1:19 PM
dcoughlin accepted D39707: [analyzer] assume bitwise arithmetic axioms.
Nov 9 2017, 10:54 AM
dcoughlin added a comment to D39707: [analyzer] assume bitwise arithmetic axioms.

This looks good to me. It is very clean! But please add a comment in the places where you are assuming a two's complement value representation for signed integers.

Nov 9 2017, 10:54 AM

Nov 8 2017

dcoughlin added a comment to D39707: [analyzer] assume bitwise arithmetic axioms.

I believe your motivating examples used errno_t, which is a signed type.

Nov 8 2017, 10:47 AM

Nov 7 2017

dcoughlin accepted D39682: [analyzer] Fix a crash on logical operators with vectors..

Interesting bug! The workaround looks good to me.

Nov 7 2017, 2:59 PM
dcoughlin added inline comments to D39711: [analyzer] ObjCGenerics: Don't warn on cast conversions involving explicit cast.
Nov 7 2017, 2:31 PM

Nov 6 2017

dcoughlin added a comment to D39711: [analyzer] ObjCGenerics: Don't warn on cast conversions involving explicit cast.

@xazax.hun Would you be willing to take a look?

Nov 6 2017, 6:33 PM
dcoughlin created D39711: [analyzer] ObjCGenerics: Don't warn on cast conversions involving explicit cast.
Nov 6 2017, 6:32 PM
dcoughlin committed rL317516: [analyzer] Model correct dispatch_once() 'done' value in BodyFarm.
[analyzer] Model correct dispatch_once() 'done' value in BodyFarm
Nov 6 2017, 2:12 PM
dcoughlin closed D39691: [analyzer] Model correct dispatch_once() 'done' value in BodyFarm by committing rL317516: [analyzer] Model correct dispatch_once() 'done' value in BodyFarm.
Nov 6 2017, 2:12 PM
dcoughlin added a comment to D39691: [analyzer] Model correct dispatch_once() 'done' value in BodyFarm.

Yeah, I'm not a fan of this style of testing for path diagnostics, either.

Nov 6 2017, 1:42 PM
dcoughlin added inline comments to D39584: [analyzer] [NFC] another very minor ExprEngineC refactoring.
Nov 6 2017, 1:22 PM
dcoughlin accepted D39620: [analyzer] [NFC] Remove unused typedef.

Looks good to me.

Nov 6 2017, 1:04 PM
dcoughlin created D39691: [analyzer] Model correct dispatch_once() 'done' value in BodyFarm.
Nov 6 2017, 11:22 AM

Nov 2 2017

dcoughlin added a comment to D39438: [analyzer] Diagnose stack leaks via block captures.

Another round of comments inline. With those addressed it looks good to me -- but you should wait on Artem's go-ahead before committing.

Nov 2 2017, 5:49 PM
dcoughlin accepted D39577: [analyzer] [NFC] very minor ExprEngineC refactoring.

LGTM. It is a nice cleanup.

Nov 2 2017, 5:35 PM
dcoughlin accepted D39518: [analyzer] do not crash on libcxx03 call_once implementation.

Looks great. Thank you!

Nov 2 2017, 5:30 PM
dcoughlin added a comment to D39518: [analyzer] do not crash on libcxx03 call_once implementation.

Updated the diff, addressed review concerns.
Made it more explicit in comments in tests that we do not crash on libcxx03 implementation, but that we don't model it either.

@dcoughlin sorry I disagree on previous testing notes (requiring to explicitly test that the std::call_once on libcxx03 is ignored).
Current tests test exactly the specification we want them to test: libcxx11 is modeled, libcxx03 does not crash.

Nov 2 2017, 3:52 PM
dcoughlin added a comment to D39551: [analyzer] Make __builtin_debugtrap() a sink.

I believe that the intent of __builtin_debugtrap() is to provide an in-source mechanism so that the developer can trap into the debugger and then continue execution. For example, in the Swift codebase this is used in combination with a debug flag to break into the debugger at the beginning a specified pass. https://github.com/apple/swift/blob/master/lib/SILOptimizer/PassManager/PassManager.cpp#L339

Nov 2 2017, 1:12 PM

Nov 1 2017

dcoughlin added a comment to D38986: [Analyzer] Better unreachable message in enumeration.

Personally, I don't think this is worth it and I find it unpleasant to add untestable code -- especially if that code is going to stick around in release builds.

Nov 1 2017, 7:43 PM

Oct 31 2017

dcoughlin added a reviewer for D39438: [analyzer] Diagnose stack leaks via block captures: george.karpenkov.

I think this is a great idea, and I expect it to find some nasty bugs!

Oct 31 2017, 9:21 PM
dcoughlin accepted D39428: [Analyzer] As suggested, use value storage for BodyFarm.

Looks good to me with the comments by Gabor addressed.

Oct 31 2017, 5:26 PM

Oct 27 2017

dcoughlin accepted D37470: [analyzer] Handle ObjC messages conservatively in CallDescription.

Thanks Gabor. Looks great to me!

Oct 27 2017, 10:08 AM

Oct 25 2017

dcoughlin added a comment to D39220: [Analyzer] Store BodyFarm in std::unique_ptr.

@dcoughlin OK, I'll commit that [I assuming not doing the review would be fine here]

Oct 25 2017, 5:39 PM
dcoughlin added a comment to D39220: [Analyzer] Store BodyFarm in std::unique_ptr.

I agree with Alexander here. The LLVM naming convention is not going to change and we should err on the side of using descriptive names. How about "FunctionBodyFarm"?

Oct 25 2017, 1:32 PM
dcoughlin added a comment to D37470: [analyzer] Handle ObjC messages conservatively in CallDescription.

I think it would be better to add a new "test/Analysis/block-in-critical-section.m" file rather than enabling a random alpha checker in a file that tests the analyzer core.

Oct 25 2017, 9:13 AM

Oct 24 2017

dcoughlin accepted D39220: [Analyzer] Store BodyFarm in std::unique_ptr.

LGTM.

Oct 24 2017, 4:13 PM

Oct 23 2017

dcoughlin added a comment to D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp.

That would require going into the past and requiring everyone back then to run clang-format as well. Unfortunately they didn't -- so human judgment is needed when modifying code that doesn't obey the guidelines.

Oct 23 2017, 5:07 PM
dcoughlin added a comment to D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp.

I think a good strategy is to look at your diffs and consider whether the benefits of normalizing the style outweigh the cost of losing the history. In this case, I think keeping the history makes sense. (Imagine you are a future maintainer and want to know when and why a new option was added. Is is very helpful to use git annotate to understand why the code is the way it is.)

Oct 23 2017, 4:52 PM
dcoughlin accepted D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp.

Content-wise, LGTM. There is a style nit inline.

Oct 23 2017, 4:15 PM
dcoughlin accepted D39201: [Analyzer] Handle implicit function reference in bodyfarming std::call_once.

LGTM!

Oct 23 2017, 3:16 PM

Oct 20 2017

dcoughlin accepted D39031: [Analyzer] Correctly handle parameters passed by reference when bodyfarming std::call_once.

Some nits inline, but looks good to me!

Oct 20 2017, 11:00 AM
dcoughlin accepted D38922: [analyzer] LoopUnrolling: check the bitwidth of the used numbers (pr34943).

Thanks for fixing this!

Oct 20 2017, 10:45 AM
dcoughlin added a comment to D38986: [Analyzer] Better unreachable message in enumeration.

What is the workflow where this is needed? Is it when an end-user is running a build with assertions and can't provide a reproducer but can provide the console output?

Oct 20 2017, 10:19 AM

Oct 17 2017

dcoughlin accepted D39015: [Analyzer] Always use non-reference types when creating expressions in BodyFarm, removes std::call_once crash.

Looks good to me.

Oct 17 2017, 3:26 PM

Oct 13 2017

dcoughlin accepted D38877: [analyzer] RetainCount: Accept "safe" CFRetain wrappers..

Looks good to me!

Oct 13 2017, 10:37 AM
dcoughlin added a reviewer for D38877: [analyzer] RetainCount: Accept "safe" CFRetain wrappers.: george.karpenkov.
Oct 13 2017, 10:36 AM