On second thought, yes, that patch is a total think-o (right idea, wrong fix)..
It should probably just say if (success) assert(what you said) Better patch incoming.. and I wonder if we can in any way force ourselves into this scenario to test the sanity of any change in this area. Let me think about that > On Jan 8, 2015, at 10:45 AM, Zachary Turner <[email protected]> wrote: > > Ok so it looks like your patch doesn't fix the assert for me, but now looking > more closely at your patch, I wonder if it's correct. As per the > description, the purpose of your patch is to fix the case where success == > false (which is what's happening to me), but you are adding a requirement to > the assert that success be true. Shouldn't this be something like the > following: > > assert (!need_compare_checksums || (!old_checksum.empty() && > !m_value_checksum.empty())); > > > > On Wed Jan 07 2015 at 5:50:44 PM Zachary Turner <[email protected] > <mailto:[email protected]>> wrote: > It just happens every time i run one of the tests on windows. It might be > triggered because something earlier is broken, but I'll send you a stack > trace or something tomorrow. Won't it be nice if/when we can debug windows > core dumps from Mac? :P > On Wed, Jan 7, 2015 at 5:21 PM Enrico Granata <[email protected] > <mailto:[email protected]>> wrote: > If you end up with a reproducible case of the assertion firing, totally let > me know - bugzilla or just an email > Hopefully it’s not so Windows-specific that I can’t get it to happen on Mac > >> On Jan 7, 2015, at 4:33 PM, Zachary Turner <[email protected] >> <mailto:[email protected]>> wrote: >> >> Cool, i think this assertion was actually firing on windows, making it very >> annoying to run the test suite. I hope this fixes that >> On Wed, Jan 7, 2015 at 4:30 PM Enrico Granata <[email protected] >> <mailto:[email protected]>> wrote: >> Author: enrico >> Date: Wed Jan 7 18:29:12 2015 >> New Revision: 225418 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=225418&view=rev >> <http://llvm.org/viewvc/llvm-project?rev=225418&view=rev> >> Log: >> Fix a problem where a ValueObject could fail to update itself, but since it >> was previously valid, we'd have an old checksum to compare aginst no new >> checksum (because failure to update), and assert() and die. Fix the problem >> by only caring about this assertion logic if updates succeed >> >> Modified: >> lldb/trunk/source/Core/ValueObject.cpp >> >> Modified: lldb/trunk/source/Core/ValueObject.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/ValueObject.cpp?rev=225418&r1=225417&r2=225418&view=diff >> >> <http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/ValueObject.cpp?rev=225418&r1=225417&r2=225418&view=diff> >> ============================================================================== >> --- lldb/trunk/source/Core/ValueObject.cpp (original) >> +++ lldb/trunk/source/Core/ValueObject.cpp Wed Jan 7 18:29:12 2015 >> @@ -250,7 +250,7 @@ ValueObject::UpdateValueIfNeeded (bool u >> m_value_checksum.clear(); >> } >> >> - assert (old_checksum.empty() == !need_compare_checksums); >> + assert (success && (old_checksum.empty() == >> !need_compare_checksums)); >> >> if (first_update) >> SetValueDidChange (false); >> >> >> _______________________________________________ >> lldb-commits mailing list >> [email protected] <mailto:[email protected]> >> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits >> <http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits> > > Thanks, > - Enrico > 📩 egranata@.com ☎️ 27683 > > > > Thanks, - Enrico 📩 egranata@.com ☎️ 27683
_______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
