[PATCH] Allow for the use of StringRef command line options instead of cl::opt<std::string>

[Michael Ilseman]

Did you mean to make those static, or can they be referred to externally?

On Sep 30, 2013, at 8:10 PM, Puyan Lotfi <plotfi at apple.com> wrote:

> Reid,
> 
> After going over the code (specifically in libclang), I just can’t guarantee that all entry points into CompilerInvocation::CreateFromArgs() are passing arguments that aren’t stack or dynamically allocated then freed (For example, whatever calling clang::createInvocationFromCommandLine could pass in args pointing to strings from its stack). I think a practical solution is to have a persisting unique string pool in this instance. I’ve decided to change my code to use StringMap instead, so that should ease your concern over the complexity. If there is an easy to reuse global string pool within clang, I’d certainly like to know (I’ve heard that there are such pools in several LLVM projects for exactly this purpose, but I am not familiar with them). The global in this case is local to the file; I think for the sake of practicality it might be ok to allow this? Either that, or assume that CompilerInvocation instances are live for the lifetime of whatever main() is calling libclang because I’m not sure if StringRef on CodeGenOptions is any better of an assumption. 
> 
> I am open to suggestions.
> 
> Thanks
> 
> Puyan
> 
> <clangWorkaroundParseCommandLineOptionsStringMap.diff>
> 
> On Sep 30, 2013, at 2:47 PM, Reid Kleckner <rnk at google.com> wrote:
> 
>> Can you try changing the std::strings in CodeGenOptions to StringRefs?  The string persistance patch is n^2 and uses globals.
>> 
>> 
>> On Mon, Sep 30, 2013 at 2:24 PM, Puyan Lotfi <plotfi at apple.com> wrote:
>> Hi
>> 
>> As requested, I have ran the address sanitizer tests. I ran them many times, and could not find any reproducible failures.
>> I did notice some intermittent failures that came and went depending on what revision I had gotten from SVN.
>> 
>> Due to concern for use-after-free I have included a patch to clang that allows strings passed to ParseCommandLineOptions() from the CreateTargetMachine() function to persist (this was the only location in LLVM or clang where I found ParseCommandLineOptions being called with automatic memory); what it does is strdups the args for -debug-pass, -limit-float-precision as well as backend args and stores the result to a global vector of StringRefs. This vector is only populated for unique strings to avoid leaking repeatedly.
>> 
>> The other three patches include changes to the CommandLine library to enable StringRef (which I already sent out), changes to places using cl::opt<std::string> to use StringRef (also already sent out), and also the requested changes to the CommandLine documentation.
>> 
>> I think this should cover all the basis. I’ll check in if this looks ok to you all.
>> 
>> Thanks
>> 
>> -Puyan
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> On Sep 23, 2013, at 10:43 AM, Reid Kleckner <rnk at google.com> wrote:
>> 
>>> I worry that this patch creates a significant risk for use-after-free bugs.  Can you:
>>> 
>>> 1. Explain how string use-after-free is supposed to be avoided?
>>> 2. Update the docs if ParseCommandLine options is no longer going to save copies of strings.
>>> 3. Make sure the internal tests pass with an AddressSanitizer build, because I see things like ParseEnvironmentOptions which look broken with this change.
>>> 
>>> 
>>> On Mon, Sep 23, 2013 at 10:00 AM, Chris Lattner <clattner at apple.com> wrote:
>>> 
>>> On Sep 19, 2013, at 3:01 PM, Puyan Lotfi <plotfi at apple.com> wrote:
>>> 
>>> > Hi All
>>> >
>>> > I’ve improved this StringRef patch and I’d like some more feedback and hopefully a submission.
>>> > My changes to the original patch thus far have been:
>>> >
>>> > - I added changes to CreateTargerMachine() in clang to have the SmallVector passed in from the caller.
>>> > - I found more places where cl::opt<str::string> was used, and I changed them to use cl::opt<StringRef>.
>>> >
>>> > I’ve attached a patch that should apply cleanly with an llvm checkout that includes clang checked out into the tools directory.
>>> 
>>> This looks great to me.  When you commit it, please split it into two pieces though:
>>> 
>>> 1) The part that adds cl compatibility with StringRef.
>>> 2) The part that switches lots of cl::opt<> to use StringRef instead of std::string.
>>> 
>>> Thanks Puyan,
>>> 
>>> -Chris
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>> 
>> 
>> 
>> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131001/0c580c27/attachment.html>