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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 > > [email protected] > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits > _______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
