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

[Puyan Lotfi]

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?

So far I’ve added support for parser and OptionValue to handle StringRefs on top of the existing string support.
I believe the common case is that ParseCommandLineOptions receive it’s argv from main.
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.

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.

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

Yes, that makes sense. I’ll have to take a look at those.

Thanks 

-Puyan

> 
> 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/20130923/e79fd045/attachment.html>