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

[Zachary Turner]

+David Majnemer <majnemer at google.com>​ +Chandler Carruth
<chandlerc at google.com>​

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