<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;"><div>I’ve run check-asan and check-all runs on the current patches.</div><div>Aside from the clang change, I get the same failures on both of my machines that I do without any of the changes (clean top of tree). </div><div><br></div><div>For the patch to code in tools/clang/lib/CodeGen/BackendUtil.cpp I had not hoisted the SmallVector high enough in the call stack, causing some address sanitizer failures.</div><div>I simply hadn’t defined the SmallVector high enough in the call stack. </div><div>I am actually not sure what the cleanest solution would be at the moment, since I don’t think adding SmallVector<const char *, 16> &BackendArgs to every function in the call stack to CreateTargetMachine().</div><div><br></div><div>The following code is the culprit:</div><div><br></div><div><div> SmallVector<const char *, 16> BackendArgs;</div><div> BackendArgs.push_back("clang"); // Fake program name. </div><div> if (!CodeGenOpts.DebugPass.empty()) {</div><div> BackendArgs.push_back("-debug-pass");</div><div> BackendArgs.push_back(<b>CodeGenOpts.DebugPass.c_str()</b>); // <- just the lines pulling the string.c_str() pout of CodeGenOpts</div><div> }</div><div><br></div></div><div><br></div><div>Note that the static strings (“clang”) are just going to persist for the entire program execution, they are fine.</div><div>A very hackish fix would be to leak:</div><div><br></div><div> BackendArgs.push_back(<b>strdup(CodeGenOpts.DebugPass.c_str())</b>);</div><div><br></div><div>Of course a better solution would just be to figure our where strings in CodeGenOpts came from (looks like they might be from main argv), and use StringRefs to the original string if available.</div><div><br></div><div>Anyhow, given the StringRef changes proposed in my patch it does in fact mean that the arguments need to come from main argv or some memory allocation that persists. </div><div>I’ll certainly update any related docks after submitting. </div><div><br></div><div>Thoughts?</div><div><br></div><div>Thanks</div><div><br></div><div>-Puyan</div><br><div><div>On Sep 23, 2013, at 6:50 PM, Puyan Lotfi <<a href="mailto:plotfi@apple.com">plotfi@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div 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>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 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></div>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br></blockquote></div><br></body></html>