<div dir="ltr">I'm resisting this change because while LLVM isn't guaranteed to be API compatible, we don't usually change old APIs in a way that makes old code subtly broken.  We usually add, remove, or replace functionality, which is easily detected when the user tries to do a build and sees that the old API is gone.<div>
<br></div><div>Can you either remove ParseCommandLineOptions altogether, or make it implicitly save copies of strings in a static local?  Then you could add a new entry point or optional parameter where the caller promises that the strings are persistent.  Clang, ParseEnvironmentOptions, and other tools can continue using the old API and share the persisting functionality.  Embedders like WebKit and other JITs won't have any initializers or finalizers if they don't call the old API.</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Oct 1, 2013 at 5: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:0 0 0 .8ex;border-left:1px #ccc 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 class="HOEnZb"><font color="#888888"><div><br></div><div>-Puyan</div></font></span><div><div class="h5"><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></div><br>_______________________________________________<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>
<br></blockquote></div><br></div>