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

[Zachary Turner]

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 <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/20150109/c3c77a48/attachment.html>