r214064 - Factoring DataflowWorklist out of LiveVariables and UninitializedValues analyses

[Artyom Skrobov]

If I’m not able to reproduce the issue, how do you expect me to see if I fixed it?

 

Please run clang -E in your environment that has all headers that options.c requires, creating a single isolated source file that shows the problem.

 

A fix would need a regression test to go in, anyway.

 

 

From: Alexander Kornienko [mailto:alexfh at google.com] 
Sent: 04 August 2014 14:03
To: Artyom Skrobov
Cc: Hal Finkel; Ted Kremenek; cfe-commits at cs.uiuc.edu Commits
Subject: Re: r214064 - Factoring DataflowWorklist out of LiveVariables and UninitializedValues analyses

 

On Mon, Aug 4, 2014 at 2:31 PM, Artyom Skrobov <Artyom.Skrobov at arm.com> wrote:

May I ask you for an isolated source file that would reproduce this issue? 

 

options.c seems to require a whole tree of headers, including event_strings.h which is not present in the repository at all.

 

I think, the issue is obvious: you've taken two different versions of the code and dropped one of them on the floor. Apparently, they were different for a reason. The fix could be as easy as adding back the version from LiveVariables.cpp (as another derived class, for example, once you already have inheritance there), and using it in LiveVariables.

 

If you want to go further and understand the reason why they should be different, and what exactly breaks, the code I pointed at is probably the best I can provide. You can try looking at revision 2229: https://code.google.com/p/dynamorio/source/browse/trunk/core/options.c?r=2229, it should be the closest one to the version I found the issue on.

 

But please first send a patch to fix the issue in the simpliest manner.

 

Thank you!

 

 

 

From: Alexander Kornienko [mailto:alexfh at google.com] 
Sent: 04 August 2014 13:25
To: Artyom Skrobov
Cc: Hal Finkel; Ted Kremenek; cfe-commits at cs.uiuc.edu Commits


Subject: Re: r214064 - Factoring DataflowWorklist out of LiveVariables and UninitializedValues analyses

 

On Mon, Aug 4, 2014 at 2:22 PM, Alexander Kornienko <alexfh at google.com> wrote:

This patch makes the deadcode.DeadStores analyzer hang on this file: https://code.google.com/p/dynamorio/source/browse/trunk/core/options.c

 

The relevant part of stack trace looks like this:

  0x00e189ed: llvm::ImutAVLFactory<llvm::ImutContainerInfo<clang::Stmt const*> >::balanceTree(llvm::ImutAVLTree<llvm::ImutContainerInfo<clang::Stmt const*> >*, clang::Stmt const*, llvm::ImutAVLTree<llvm::ImutContainerInfo<clang::Stmt const*> >*)
  0x00e18d2e: llvm::ImutAVLFactory<llvm::ImutContainerInfo<clang::Stmt const*> >::add_internal(clang::Stmt const*, llvm::ImutAVLTree<llvm::ImutContainerInfo<clang::Stmt const*> >*)
  0x00e18ec5: llvm::ImutAVLFactory<llvm::ImutContainerInfo<clang::Stmt const*> >::add_internal(clang::Stmt const*, llvm::ImutAVLTree<llvm::ImutContainerInfo<clang::Stmt const*> >*)
  0x00e1b292: llvm::ImutAVLFactory<llvm::ImutContainerInfo<clang::Stmt const*> >::add(llvm::ImutAVLTree<llvm::ImutContainerInfo<clang::Stmt const*> >*, clang::Stmt const*)
  0x00e1f2d1: clang::LiveVariables::computeLiveness(clang::AnalysisDeclContext&, bool)
  0x00673969: void clang::ento::check::ASTCodeBody::_checkBody<(anonymous namespace)::DeadStoresChecker>(void*, clang::Decl const*, clang::ento::AnalysisManager&, clang::ento::BugReporter&)

Did you notice that the DataflowWorklist class was a bit different in these two classes? 

 

I meant, "files" (UninitializedValues.cpp and LiveVariables.cpp), not "classes". 

 

Notably, the dequeue method and initialization of the enqueuedBlocks bit-vector.

 

Please fix or revert the patch.

 

Thank you!

 

On Tue, Jul 29, 2014 at 6:27 PM, Artyom Skrobov <Artyom.Skrobov at arm.com> wrote:

Hal, thank you for the suggestion! I've expanded that comment four-fold in r214183.

Ted, thank you for reviewing the original patch, and no worries it took a while.
You might want to also check that the comments I've added when committing r214064 are correct -- although they're essentially a rephrasing of comments from your own emails.



-----Original Message-----
From: Hal Finkel [mailto:hfinkel at anl.gov]
Sent: 28 July 2014 13:12
To: Artyom Skrobov
Cc: cfe-commits at cs.uiuc.edu
Subject: Re: r214064 - Factoring DataflowWorklist out of LiveVariables and UninitializedValues analyses


We should have a description here of what this code does, not just where it's used. One can get some idea by reading the comments above DataflowWorklist::enqueueSuccessors in the source file, but it is not clear that gives a complete picture.

 -Hal





_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140804/503d28cd/attachment.html>