<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 2, 2013, at 2:21 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>I think some tradeoffs need to be made in order to improve startup time as a result of global static constructors, and I’m not sure if there is an ideal way we can do that in all cases. I’m not proposing we have a very complicated context system, but I do think a really simple and optional unique string table could make transitioning a lot easier.</div>
</div></blockquote><div><br></div><div>Cheating a little bit, I'll single out this paragraph/point: If the intention is to transition and remove the old API in any case, I don't think we necessarily benefit from having backwards compatible functionality - because we can change callers to meet the new requirement first. So the goal should be to look at all callers, update them so they preserve the arguments passed in with sufficient lifetime for their use case, then change the underlying API to depend on that contract.<br>
<br>The alternative seems to be to modify the API to provide safety, then transition each caller - the only benefit to doing it in this order is that we transition some callers sooner (but this work shouldn't take long - certainly not as long as this thread has gone on for) with the risk that we leave the codebase in an intermediate state, supporting both modes, which doesn't seem desirable.<br>
<br></div></div></div></div></blockquote><div><br></div><div>I agree with this, but I just don’t feel familiar enough with the clang driver at the moment to make those changes. Ignoring the clang issues, I am a bit unsure what the best course would be for addressing use of cl::opt<StringRef> with ParseEnvironmentOptions().</div><div><br></div><div>Some of the behavior I am proposing already exists inside of the CommandLine library itself. See class StrDupSaver: it practically does what I proposed; it just doesn’t persist, and it doesn’t unique the strings. In fact it causes the exact behavior we don’t want when using StringRef.</div><div><br></div><div>I think at the moment it could be perfectly fine to specify in the documentation that parsing environment variable options should be done with cl::opt<std::string> and just leave clang alone since it seems that use after free is practically not going to happen.</div><div><br></div><div>-Puyan</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>(and an addendum to reply to Reid's point regarding API safety - I don't think it's ever been a requirement that we make compile-time API breaks to help out of tree users migrate safely. It's just a quirk of circumstance that it's rare to make a semantics changing but API-preserving change, not something we deliberately avoid doing. That being said, if there's some other name for this function that seems useful (& possibly conveys the fact that it doesn't make safe copies), I don't mind changing it when the semantics are changed... )</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><span class="HOEnZb"><font color="#888888"><div><br></div><div>-Puyan</div>
</font></span><div><div class="h5"><div><br></div><br><div><div>On Oct 1, 2013, at 3:02 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><blockquote type="cite">
<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Oct 1, 2013 at 2:50 PM, Puyan Lotfi <span dir="ltr"><<a href="mailto:plotfi@apple.com" target="_blank">plotfi@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"><div style="word-wrap:break-word"><div>EmitAssemblyHelper::CreateTargetMachine() in clang/lib/CodeGen/BackendUtil.cpp </div>
<div><br></div><div>The CodeGenOptons class holds option std:strings copied from ArgsLists that came from either argv in clangs main, </div></div></blockquote><div><br></div><div>So it sounds like for the caller from 'main' this is safe, yes? It comes from argv, which stays live long enough and isn't tainted/modified in undesirable ways?</div>
<div> </div><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"><div>or from whoever is calling into CompilerInvocation in libclang. </div>
</div></blockquote><div><br></div><div>OK, so if I understand you correctly this is an API question for libclang? </div><div><br></div><div>A quick grep (so maybe I've missed things - I likely have) of libclang for CompilerInvocation finds clang_indexSourceFile_Impl which uses a temporary/local std::vector Args that is passed to the createInvocationFromCommandLine but the result of that call is used and destroyed within the lifetime of this function, so again it looks like the backing store of the caller is sufficiently live to keep the strings for the region of usage. <br>
<br>Are there other callers in libclang or elsewhere where you believe the caller is not currently keeping things sufficiently live? (and it's not practical to modify it to keep it live for that period)</div><div> </div>
<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"><div>It’s not very easy to know if the compiler invocation instance that lead to that point in the code in clang will be alive for the lifetime of the program. </div>
<div><br></div><div>I think we’ll have a similar set of problems with ParseEnvironmentOptions in unit tests/Support/CommandLineTest.cpp if we were to change the cl::opt<std::string>’s to cl::opt<StringRef>’s.</div>
</div></blockquote><div><br></div><div>Which callers are you concerned about here? Each test seems fairly standalone and the Input strings are scoped for the life of the function.</div><div> </div><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"><span><font color="#888888"><div><br></div><div>-Puyan</div></font></span><div><div><br></div><br><div><div>On Oct 1, 2013, at 2:36 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr">I've been following this thread only tangentially for the last few weeks. Personally, I would like to live in a world where callers of ParseCommandLineOptions handle the lifetime of those strings to ensure they remain live for the lifetime of the code that needs the command line options.<br>
<br>Can you point me to a particular situation where a caller is unable to do this that ends up necessitating the GetPersistentClArgStr you're referring to?<br><div class="gmail_extra"><br><br><div class="gmail_quote">
On Tue, Oct 1, 2013 at 2:33 PM, Puyan Lotfi <span dir="ltr"><<a href="mailto:plotfi@apple.com" target="_blank">plotfi@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">
<div style="word-wrap:break-word"><div>David, Chris:</div><div><br></div><div>It seems that in certain cases it not very easy for callers of ParseCommandLineOptions to get to the global static instances of a given string and that is being parsed (when the strings come from some dynamically populated string table rather than main’s argv that is passed, as is the case with the driver code in clang).</div>
<div><br></div><div>I’ve found that in one case (in clang), a uniquing table fixes the problem of handing strings given to the ParseCommandLineOptions function; something like this:</div><div><br></div><div><font face="Monaco">static const char* GetPersistentClArgStr(const StringRef &key) {<br>
static StringMap<const char*> PersistentClArgString;<br> StringMap<const char*>::const_iterator I = PersistentClArgString.find(key);<br> if (I == PersistentClArgString.end()) {<br> const char* persistVal = strdup(key.data());<br>
PersistentClArgString[key] = persistVal;<br> return persistVal;<br> }<br> return I->second;<br>}</font></div><div><br></div><div>I used this in one particular case in clang, but it now seems like it would be useful to integrate into the CommandLine library and optionally have it turned on with an optional parameter to Parse<b>CommandLine</b>Options. For example, the Parse<b>Environment</b>Options (which calls ParseCommandLineOptions) function will fail when used with a cl::opt<StringRef> rather than std::string. It actually looks like ParseEnvironmentOptions has a StrDup class it uses to hold on to argument string duplicates but it seems to then go and delete them. afterward. </div>
<div><br></div><div>I am very eager for suggestions on this.</div><div><br></div><div>Thanks</div><span><font color="#888888"><div><br></div><div>-Puyan</div></font></span><div><br><div><div>
On Aug 15, 2013, at 10:57 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><blockquote type="cite">On Thu, Aug 15, 2013 at 4:56 PM, Puyan Lotfi <<a href="mailto:plotfi@apple.com" target="_blank">plotfi@apple.com</a>> wrote:<br>
<blockquote type="cite"><br>This sounds like a workable compromise.<br>To avoid leaking how about a CommandLine.cpp handled string pool that allocates copies on demand (specified maybe by an optional flag to ParseCommandLineOptions/ParseEnvironmentOptions) but also has the option of flushing/deallocating the pool?<br>
</blockquote><br>I'm with Chris - I think it should be simpler to have<br>ParseCommandLineOptions/CommandLine in general not be concerned with<br>task of owning the strings. That should be the responsibility of the<br>
caller.<br><br><blockquote type="cite"><br>-Puyan<br><br>On Aug 15, 2013, at 1:47 PM, Chris Lattner <<a href="mailto:clattner@apple.com" target="_blank">clattner@apple.com</a>> wrote:<br><br><blockquote type="cite">
<br>On Aug 14, 2013, at 4:06 PM, Puyan Lotfi <<a href="mailto:plotfi@apple.com" target="_blank">plotfi@apple.com</a>> wrote:<br><br><blockquote type="cite">Oh wow. I see that.<br><br>I am not completely sure how the char* arrays would get handled this way.<br>
I did not make any clang modifications in this patch (although since clang is using the cl library it is affected) but I noticed that the use of cl:: in the llvm source tree made use of global cl::opt variables that specify the opt type.<br>
I don’t see that in the files handling the -mllvm options; I’ll have to look at the CommandLine.cpp code a little more closely to be sure in this case that the char*’s aren’t being implicitly put into StringRefs that might have their data pointers deallocated after cl::ParseCommandLineOptions() is called.<br>
</blockquote><br>After thinking about it for a bit, how about we go with this approach:<br><br>1. Document ParseCommandLineOptions/ParseEnvironmentOptions to only work with strings that live forever in CommandLine.h<br>2. Change clang to leak (or store globally) the strings that are affected.<br>
<br>I don't think it is worth designing a context system or anything like that to handle this case.<br><br>-Chris<br></blockquote><br><br>_______________________________________________<br>llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">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>
</blockquote></blockquote></div><br></div></div></blockquote></div><br></div></div>
</blockquote></div><br></div></div></blockquote></div><br></div></div>
</blockquote></div><br></div></div></div></blockquote></div><br></div></div>
</blockquote></div><br></body></html>