[PATCH] Allow for the use of StringRef command line options instead of cl::opt<std::string>
plotfi at apple.com
Mon Sep 23 18:50:18 PDT 2013
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.
> 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,
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-commits