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

[jingham at apple.com]

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
>