<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 Sep 23, 2013, at 10:43 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">I worry that this patch creates a significant risk for use-after-free bugs. Can you:<div><br><div>1. Explain how string use-after-free is supposed to be avoided?</div></div></div></blockquote><div><br></div><div>So far I’ve added support for parser and OptionValue to handle StringRefs on top of the existing string support.</div><div>I believe the common case is that ParseCommandLineOptions receive it’s argv from main.</div><div>In a few cases this was not true and I found that either memory was already being leaked, and in one case I found that the memory allocated needed to be hoisted to a calling function.</div><div><br></div><div>I think it may be the case that unmodified code using cl::opt<string> would continue to work and that the parser<std::string> functions handling those args would continue managing their own copies.</div><br><blockquote type="cite"><div dir="ltr"><div><div>2. Update the docs if ParseCommandLine options is no longer going to save copies of strings.</div>
<div>3. Make sure the internal tests pass with an AddressSanitizer build, because I see things like ParseEnvironmentOptions which look broken with this change.</div></div></div><div class="gmail_extra"><br></div></blockquote><div><br></div><div>Yes, that makes sense. I’ll have to take a look at those.</div><div><br></div><div>Thanks </div><div><br></div><div>-Puyan</div><div><br></div><blockquote type="cite"><div class="gmail_extra"><br><div class="gmail_quote">
On Mon, Sep 23, 2013 at 10:00 AM, Chris Lattner <span dir="ltr"><<a href="mailto:clattner@apple.com" target="_blank">clattner@apple.com</a>></span> wrote:<br><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; position: static; z-index: auto;">
<div class="HOEnZb"><div class="h5"><br>
On Sep 19, 2013, at 3:01 PM, Puyan Lotfi <<a href="mailto:plotfi@apple.com">plotfi@apple.com</a>> wrote:<br>
<br>
> Hi All<br>
><br>
> I’ve improved this StringRef patch and I’d like some more feedback and hopefully a submission.<br>
> My changes to the original patch thus far have been:<br>
><br>
> - I added changes to CreateTargerMachine() in clang to have the SmallVector passed in from the caller.<br>
> - I found more places where cl::opt<str::string> was used, and I changed them to use cl::opt<StringRef>.<br>
><br>
> I’ve attached a patch that should apply cleanly with an llvm checkout that includes clang checked out into the tools directory.<br>
<br>
</div></div>This looks great to me. When you commit it, please split it into two pieces though:<br>
<br>
1) The part that adds cl compatibility with StringRef.<br>
2) The part that switches lots of cl::opt<> to use StringRef instead of std::string.<br>
<br>
Thanks Puyan,<br>
<br>
-Chris<br>
<div class="HOEnZb"><div class="h5">_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>
</blockquote></div><br></body></html>