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

[Reid Kleckner]

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/45dff22b/attachment.html>