<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Oct 4, 2013, at 7:18 AM, Reid Kleckner <<a href="mailto:rnk@google.com">rnk@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr">On Thu, Oct 3, 2013 at 6:45 PM, Puyan Lotfi <span dir="ltr"><<a href="mailto:plotfi@apple.com" target="_blank">plotfi@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word">Also agreed. I prefer the simpler solution myself. As far as I can tell, some of the cases involving UAF bugs would be averted if cl::opt<std::string> were used (as in the case of parse environment options). There’s no evidence thus far that suggests existing code would encounter UAF bugs. </div>
</blockquote><div><br></div><div>OK, but many CommandLine users (Mono, previously I know we wanted this for Unladen Swallow) are trying to set flags in LLVM libraries. You are changing LLVM to use cl::opt<StringRef> instead of cl::opt<std::string>. So, it seems to me that there really are bugs.</div>
<div><br></div></div></div></div></blockquote><div><br></div><div>Nowhere in this patch am I removing cl::opt<std::string>. In fact, some of our tools would fail (like llc, or llvm-as) because they often use a cl::opt<std::string> for the output filename arg (which they modify). The changes I made were removing the use of cl::opt<std::string> in most places in LLVM so that startup time as a result of static constructors be reduced. </div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>David:</div><div><div class="im"><blockquote class="gmail_quote" style="font-family:arial,sans-serif;font-size:13px;margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Long term, we'll probably want to store these global options on the LLVMContext, and then we can kill off this string persisting code.</blockquote>
</blockquote><blockquote class="gmail_quote" style="font-family: arial, sans-serif; font-size: 13px; margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; position: static; z-index: auto;"><br>
Any reason not to do that now, if that's the preference? (I'm not suggesting changing the opt APIs from static globals to accessors on LLVMContext immediately (I guess that's what you were talking about) but if LLVMContext has the desired lifetime we could just wrap the ParseCommandLineOptions in an LLVMContext entry point that persists the string first, then calls the parse routine, for example)</blockquote>
<div><br></div><div>There's no reason not to do it now. I just don't like asking for lots of yak shaving work in code review. Maybe it's the right thing though. :) </div></div></div></div></div></div></blockquote></div><div><br></div><div>I’m with David on this. I just don’t see a reason for the complexity. I’ll take a look at the mono code again and see if I can find anything to actually worry about. </div><div><br></div><div><br></div></body></html>