r214064 - Factoring DataflowWorklist out of LiveVariables and UninitializedValues analyses

[Alexander Kornienko]

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

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

Fair enough. On the other hand, compiling an average open-source project
nowadays isn't that complicated. With the one I pointed you to, it's a
matter of:
   svn checkout -r 2229 http://dynamorio.googlecode.com/svn/trunk/
dynamorio && cd dynamorio && cmake . && make all
This configures the project and generates all missing headers. But if you
prefer to look at the preprocessed source, I'm sending it to you. Please
see options.i.gz. If sending an attachment to the list doesn't work, let me
know and I'll send it to you directly.


> 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? Actually,
> I've sent you the link to the file. Getting a compilable copy locally is a
> matter of:
>
>    svn checkout -r 2229 http://dynamorio.googlecode.com/svn/trunk/
> dynamorio && cd dynamorio && cmake . && make all
>
>
>
> 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 nov 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/53227252/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: options.i.gz
Type: application/x-gzip
Size: 248606 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140804/53227252/attachment.bin>