thr3ads.net - llvm dev - [LLVMdev] Am I missing something in this Value debug output code? [Mar 2015]

If this information is useful, please help other people find it:
Share via:

Kaylor, Andrew

2015-Mar-10 19:23 UTC

[LLVMdev] Am I missing something in this Value debug output code?

In the course of adding some new code I came across a debug output message
before an assert that was extremely unhelpful.  If I incorrectly try to delete a
Value object that still has uses in a debug build, an debug message is printed
just before an assertion that says it is telling me where the value is still
being used but actually just tries to print the value itself multiple times,
which in my case was crashing because the Value was already partially
destructed.

It looked like a pretty simple and obvious fix and I was just going to go ahead
and commit it, but when I looked at the revision history I saw that the current
behavior has been in place since 2002.  So I thought maybe it would be a good
idea to get a reality check and make sure I wasn't being an idiot in some
way.

Can someone give me a second opinion?

Here's the change I am proposing:

Index: lib/IR/Value.cpp
==================================================================---
lib/IR/Value.cpp         (revision 231388)
+++ lib/IR/Value.cpp      (working copy)
@@ -69,15 +69,15 @@
#ifndef NDEBUG      // Only in -g mode...
   // Check to make sure that there are no uses of this value that are still
   // around when the value is destroyed.  If there are, then we have a dangling
-  // reference and something is wrong.  This code is here to print out what is
-  // still being referenced.  The value in question should be printed as
-  // a <badref>
+  // reference and something is wrong.  This code is here to print out where
+  // the value is still being referenced.  The value will be printed as a
+  // <badref>.
   //
   if (!use_empty()) {
     dbgs() << "While deleting: " << *VTy << "
%" << getName() << "\n";
     for (use_iterator I = use_begin(), E = use_end(); I != E; ++I)
       dbgs() << "Use still stuck around after Def is
destroyed:"
-           << **I << "\n";
+           << *(I->getUser()) << "\n";
   }
#endif
   assert(use_empty() && "Uses remain when a value is
destroyed!");

======================================================================
This code appears in the Value::~Value().  I'm pretty sure that
"**I" here is just going to be a reference to the Value being
destructed, whereas *(I->getUser()) will provide useful information.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150310/4402c473/attachment.html>

Daniel Berlin

2015-Mar-10 19:45 UTC

head link

[LLVMdev] Am I missing something in this Value debug output code?

I think this just a bug.
I actually had the same bug in some other code.
Nowadays, i think this should just be "for (I : users())"

The main use of the use_begin/use_end idiom nowadays is to be able to
modify the use list itself (IE people removing uses) without iterator
invalidation.



On Tue, Mar 10, 2015 at 12:23 PM, Kaylor, Andrew <andrew.kaylor at
intel.com>
wrote:
>  In the course of adding some new code I came across a debug output
> message before an assert that was extremely unhelpful.  If I incorrectly
> try to delete a Value object that still has uses in a debug build, an debug
> message is printed just before an assertion that says it is telling me
> where the value is still being used but actually just tries to print the
> value itself multiple times, which in my case was crashing because the
> Value was already partially destructed.
>
>
>
> It looked like a pretty simple and obvious fix and I was just going to go
> ahead and commit it, but when I looked at the revision history I saw that
> the current behavior has been in place since 2002.  So I thought maybe it
> would be a good idea to get a reality check and make sure I wasn’t being an
> idiot in some way.
>
>
>
> Can someone give me a second opinion?
>
>
>
> Here’s the change I am proposing:
>
>
>
> Index: lib/IR/Value.cpp
>
> ==================================================================>
> --- lib/IR/Value.cpp         (revision 231388)
>
> +++ lib/IR/Value.cpp      (working copy)
>
> @@ -69,15 +69,15 @@
>
> #ifndef NDEBUG      // Only in -g mode...
>
>    // Check to make sure that there are no uses of this value that are
> still
>
>    // around when the value is destroyed.  If there are, then we have a
> dangling
>
> -  // reference and something is wrong.  This code is here to print out
> what is
>
> -  // still being referenced.  The value in question should be printed as
>
> -  // a <badref>
>
> +  // reference and something is wrong.  This code is here to print out
> where
>
> +  // the value is still being referenced.  The value will be printed as a
>
> +  // <badref>.
>
>    //
>
>    if (!use_empty()) {
>
>      dbgs() << "While deleting: " << *VTy <<
" %" << getName() << "\n";
>
>      for (use_iterator I = use_begin(), E = use_end(); I != E; ++I)
>
>        dbgs() << "Use still stuck around after Def is
destroyed:"
>
> -           << **I << "\n";
>
> +           << *(I->getUser()) << "\n";
>
>    }
>
> #endif
>
>    assert(use_empty() && "Uses remain when a value is
destroyed!");
>
>
>
> ======================================================================>
>
>
> This code appears in the Value::~Value().  I’m pretty sure that “**I” here
> is just going to be a reference to the Value being destructed, whereas
> *(I->getUser()) will provide useful information.
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150310/f298d23b/attachment.html>

Reid Kleckner

2015-Mar-10 19:59 UTC

head link

[LLVMdev] Am I missing something in this Value debug output code?

I think Chandler changed the result of the operator* overload on
use_iterator some time last year to produce a Use instead of a User. Both
are implicitly convertible to Values, so this code compiles either way.
It's functionality isn't actually covered by tests so... your fix looks
correct. :)

On Tue, Mar 10, 2015 at 12:23 PM, Kaylor, Andrew <andrew.kaylor at
intel.com>
wrote:
>  In the course of adding some new code I came across a debug output
> message before an assert that was extremely unhelpful.  If I incorrectly
> try to delete a Value object that still has uses in a debug build, an debug
> message is printed just before an assertion that says it is telling me
> where the value is still being used but actually just tries to print the
> value itself multiple times, which in my case was crashing because the
> Value was already partially destructed.
>
>
>
> It looked like a pretty simple and obvious fix and I was just going to go
> ahead and commit it, but when I looked at the revision history I saw that
> the current behavior has been in place since 2002.  So I thought maybe it
> would be a good idea to get a reality check and make sure I wasn’t being an
> idiot in some way.
>
>
>
> Can someone give me a second opinion?
>
>
>
> Here’s the change I am proposing:
>
>
>
> Index: lib/IR/Value.cpp
>
> ==================================================================>
> --- lib/IR/Value.cpp         (revision 231388)
>
> +++ lib/IR/Value.cpp      (working copy)
>
> @@ -69,15 +69,15 @@
>
> #ifndef NDEBUG      // Only in -g mode...
>
>    // Check to make sure that there are no uses of this value that are
> still
>
>    // around when the value is destroyed.  If there are, then we have a
> dangling
>
> -  // reference and something is wrong.  This code is here to print out
> what is
>
> -  // still being referenced.  The value in question should be printed as
>
> -  // a <badref>
>
> +  // reference and something is wrong.  This code is here to print out
> where
>
> +  // the value is still being referenced.  The value will be printed as a
>
> +  // <badref>.
>
>    //
>
>    if (!use_empty()) {
>
>      dbgs() << "While deleting: " << *VTy <<
" %" << getName() << "\n";
>
>      for (use_iterator I = use_begin(), E = use_end(); I != E; ++I)
>
>        dbgs() << "Use still stuck around after Def is
destroyed:"
>
> -           << **I << "\n";
>
> +           << *(I->getUser()) << "\n";
>
>    }
>
> #endif
>
>    assert(use_empty() && "Uses remain when a value is
destroyed!");
>
>
>
> ======================================================================>
>
>
> This code appears in the Value::~Value().  I’m pretty sure that “**I” here
> is just going to be a reference to the Value being destructed, whereas
> *(I->getUser()) will provide useful information.
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150310/89b6d0cf/attachment.html>

llvm dev - Mar 2015 - [LLVMdev] Am I missing something in this Value debug output code?

[LLVMdev] Am I missing something in this Value debug output code?

[LLVMdev] Am I missing something in this Value debug output code?

[LLVMdev] Am I missing something in this Value debug output code?