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

[Puyan Lotfi]

To be clear, I do intend to try and change at least the options used in
CreateTargetMachine() (-debug-pass, -limit-float-precision and backend
args) to use StringRefs. Just need to confirm I can get the correct
pointers with those strings from main's argv before I can do that.

-Puyan


On Mon, Sep 30, 2013 at 3:12 PM, Puyan Lotfi <plotfi at apple.com> wrote:

> 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
>>>
>>
>>
>>
>>
>
>
> _______________________________________________
> 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/c6c4d313/attachment.html>