[PATCH] Allow for the use of StringRef command line options instead of cl::opt<std::string>
plotfi at apple.com
Fri Oct 4 10:21:08 PDT 2013
On Oct 4, 2013, at 7:18 AM, Reid Kleckner <rnk at google.com> wrote:
> On Thu, Oct 3, 2013 at 6:45 PM, Puyan Lotfi <plotfi at apple.com> wrote:
> Also agreed. I prefer the simpler solution myself. As far as I can tell, some of the cases involving UAF bugs would be averted if cl::opt<std::string> were used (as in the case of parse environment options). There’s no evidence thus far that suggests existing code would encounter UAF bugs.
> OK, but many CommandLine users (Mono, previously I know we wanted this for Unladen Swallow) are trying to set flags in LLVM libraries. You are changing LLVM to use cl::opt<StringRef> instead of cl::opt<std::string>. So, it seems to me that there really are bugs.
Nowhere in this patch am I removing cl::opt<std::string>. In fact, some of our tools would fail (like llc, or llvm-as) because they often use a cl::opt<std::string> for the output filename arg (which they modify). The changes I made were removing the use of cl::opt<std::string> in most places in LLVM so that startup time as a result of static constructors be reduced.
> Long term, we'll probably want to store these global options on the LLVMContext, and then we can kill off this string persisting code.
> Any reason not to do that now, if that's the preference? (I'm not suggesting changing the opt APIs from static globals to accessors on LLVMContext immediately (I guess that's what you were talking about) but if LLVMContext has the desired lifetime we could just wrap the ParseCommandLineOptions in an LLVMContext entry point that persists the string first, then calls the parse routine, for example)
> There's no reason not to do it now. I just don't like asking for lots of yak shaving work in code review. Maybe it's the right thing though. :)
I’m with David on this. I just don’t see a reason for the complexity. I’ll take a look at the mono code again and see if I can find anything to actually worry about.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-commits