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

[Puyan Lotfi]

I’ve already been looking into that. It does look like strings in CodeGenOptions are populated from code passing argv from main in most clang projects.
Another thing I noticed was that most code calling into CreateMachineTarget is providing CodeGenOptions from a persisting CompilerInvocation instance.
I’ve actually not seen any asan failures with or without the clang patch either. So I’m guessing CompilerInvocation persists in most clang tools or there aren’t enough libclang asan tests?

-Puyan


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
>> 
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130930/23eb8033/attachment.html>