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

Puyan Lotfi plotfi at apple.com
Wed Sep 25 17:06:28 PDT 2013

I’ve run check-asan and check-all runs on the current patches.
Aside from the clang change, I get the same failures on both of my machines that I do without any of the changes (clean top of tree). 

For the patch to code in  tools/clang/lib/CodeGen/BackendUtil.cpp I had not hoisted the SmallVector high enough in the call stack, causing some address sanitizer failures.
I simply hadn’t defined the SmallVector high enough in the call stack. 
I am actually not sure what the cleanest solution would be at the moment, since I don’t think adding   SmallVector<const char *, 16> &BackendArgs to every function in the call stack to CreateTargetMachine().

The following code is the culprit:

  SmallVector<const char *, 16> BackendArgs;
  BackendArgs.push_back("clang"); // Fake program name.                                                  
  if (!CodeGenOpts.DebugPass.empty()) {
    BackendArgs.push_back(CodeGenOpts.DebugPass.c_str()); // <- just the lines pulling the string.c_str() pout of CodeGenOpts

Note that the static strings (“clang”) are just going to persist for the entire program execution, they are fine.
A very hackish fix would be to leak:


Of course a better solution would just be to figure our where strings in CodeGenOpts came from (looks like they might be from main argv), and use StringRefs to the original string if available.

Anyhow, given the StringRef changes proposed in my patch it does in fact mean that the arguments need to come from main argv or some memory allocation that persists. 
I’ll certainly update any related docks after submitting. 




On Sep 23, 2013, at 6:50 PM, Puyan Lotfi <plotfi at apple.com> wrote:

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

More information about the llvm-commits mailing list