[Lldb-commits] [PATCH] Fix IRInterpreter guard variable replacement.

Mon Jan 12 12:42:09 PST 2015

Just wanted to revisit this quickly.  It looks like the issue with Sean's
implementation is that it doesn't work correctly in the presence of
invalidated iterators that result from replacing intermediate values with
0, due to the fact that it uses a ranged based for loop.

On Fri Jan 09 2015 at 12:40:37 PM <jingham at apple.com> wrote:

> I don't see a problem with that.  If we start seeing other issues we can
> always back it out.
>
> Jim
>
>
> > On Jan 9, 2015, at 11:10 AM, Zachary Turner <zturner at google.com> wrote:
> >
> > FWIW I ran the test suite on Linux with this patch applied and nothing
> regressed.  Since Windows is the only platform with a different compiler
> ABI, I think it should be ok to check in since it passes on Linux.
> Thoughts?
> >
> > On Thu Jan 08 2015 at 11:42:32 AM Zachary Turner <zturner at google.com>
> wrote:
> > +David Majnemer +Chandler Carruth
> >
> > David works on the LLVM side and he is the one that actually worked with
> me at my desk to figure this out, and it was his suggestion that led me to
> try out replaceAllUsesWith to begin with.  Also adding Chandler as another
> LLVM person for general commentary since many other people hare are also
> still out on various types of holiday leave.
> >
> > From what I can tell, Windows just generates different LLVM IR for guard
> variables, and the particular arrangement of the guard users wasn't being
> correctly handled by Sean's original implementation of the function.
> >
> > The most obvious difference I can see from looking at the two
> implementations is that the LLVM version handles constants, whereas the
> implementation I've "fixed" here didn't, but had a comment indicating that
> they should be handled in the future.  I guess without Sean's thoughts on
> why this function wasn't used originally, it's hard to know for sure the
> history there.
> >
> > I'll let David and/or Chandler offer more commentary, although David did
> mention that he thinks a better, but more involved fix is to simply allow
> LLDB's IR Interpreter to correctly handle loads and stores to global
> variables, and then none of this would have ever happened.
> >
> >
> > On Thu Jan 08 2015 at 11:29:44 AM <jingham at apple.com> wrote:
> > Sean is on paternity leave, so he'll probably be slow to answer this
> sort of ping for a while.
> >
> > Value::replaceAllUsesWith does things very differently from this little
> function Sean wrote.  From what I can tell, it looks like it is the right
> thing to use, but that function has been in llvm for a while so I don't
> know why Sean didn't originally use it here.  Do you know what about
> Windows in particular caused Sean's version to fail, since it doesn't look
> like it causes any trouble on other platforms?
> >
> > If one of the llvm folks reading this list knows more about the IR Value
> class and has an opinion about the use of replaceAllUsesWith as opposed to
> the simpler operation Sean did, I'd love to hear that too...
> >
> > Jim
> >
> >
> > > On Jan 6, 2015, at 3:45 PM, Zachary Turner <zturner at google.com> wrote:
> > >
> > > +Sean Callanan
> > >
> > > I couldn't include you on the Phabricator issue Sean, so I'm CC'ing
> you directly.  PTAL
> > >
> > > On Tue Jan 06 2015 at 3:45:04 PM Zachary Turner <zturner at google.com>
> wrote:
> > > MS ABI guard variables end with @4IA, so this patch teaches the
> interpreter about that.  Additionally, there was an issue with
> TurnGuardLoadIntoZero which was causing some guard uses of a variable to be
> missed.  This fixes that by calling Instruction::replaceAllUsesWith()
> instead of trying to replicate that function.
> > >
> > > This fixes between 8 and 10 tests on Windows, and in particular fixes
> evaluation of C / C++ local variables.
> > >
> > > http://reviews.llvm.org/D6859
> > >
> > > Files:
> > >   source/Expression/IRForTarget.cpp
> > >
> > > Index: source/Expression/IRForTarget.cpp
> > > ===================================================================
> > > --- source/Expression/IRForTarget.cpp
> > > +++ source/Expression/IRForTarget.cpp
> > > @@ -2043,8 +2043,12 @@
> > >
> > >      GlobalVariable *GV = dyn_cast<GlobalVariable>(Old);
> > >
> > > -    if (!GV || !GV->hasName() || !GV->getName().startswith("_ZGV"))
> > > +    if (!GV || !GV->hasName() ||
> > > +        (!GV->getName().startswith("_ZGV") && // Itanium ABI guard
> variable
> > > +         !GV->getName().endswith("@4IA")))    // Microsoft ABI guard
> variable
> > > +    {
> > >          return false;
> > > +    }
> > >
> > >      return true;
> > >  }
> > > @@ -2052,20 +2056,8 @@
> > >  void
> > >  IRForTarget::TurnGuardLoadIntoZero(llvm::Instruction* guard_load)
> > >  {
> > > -    Constant* zero(ConstantInt::get(Type::getInt8Ty(m_module->getContext()),
> 0, true));
> > > -
> > > -    for (llvm::User *u : guard_load->users())
> > > -    {
> > > -        if (isa<Constant>(u))
> > > -        {
> > > -            // do nothing for the moment
> > > -        }
> > > -        else
> > > -        {
> > > -            u->replaceUsesOfWith(guard_load, zero);
> > > -        }
> > > -    }
> > > -
> > > +    Constant *zero(Constant::getNullValue(guard_load->getType()));
> > > +    guard_load->replaceAllUsesWith(zero);
> > >      guard_load->eraseFromParent();
> > >  }
> > >
> > > EMAIL PREFERENCES
> > >   http://reviews.llvm.org/settings/panel/emailpreferences/
> > > _______________________________________________
> > > lldb-commits mailing list
> > > lldb-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150112/e8d9d751/attachment.html>