r214064 - Factoring DataflowWorklist out of LiveVariables and UninitializedValues analyses

[Alexander Kornienko]

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/e4a9a216/attachment.html>