r336219 - Fix crash in clang.

[Zachary Turner via cfe-commits]

makeArrayRef() isn't necessary, but when I was first looking at this I had
to stare at the code for a bit to see that there was an implicit conversion
happening.  So I put the makeArrayRef() just for the benefit of the person
reading the code.  But no, it's not necessary.

This did not fail on existing tests, it failed when a user reported a
crash.  I'm not sure of a way to write a test to force the original crash
though.  Every invocation of the clang cc1 driver from the cl driver
already goes through this code path, and it wasn't crashing on any bots.
The crash was due to freeing stack memory and then immediately accessing
that memory.  This will accidentally work a lot of the time.

(Also not sure why our sanitizer bots didn't catch it).

On Mon, Jul 9, 2018 at 1:42 PM David Blaikie <dblaikie at gmail.com> wrote:

> Did this fail on an existing regression test, or is there a need for more
> test coverage? (guessing it failed on existing tests)
>
> Also, is the makeArrayRef necessary? Looks like if the original code
> compiled (implicitly converting from vector to ArrayRef) then the new code
> wouldn't need a makeArrayRef either?
>
> On Tue, Jul 3, 2018 at 11:17 AM Zachary Turner via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> Author: zturner
>> Date: Tue Jul  3 11:12:39 2018
>> New Revision: 336219
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=336219&view=rev
>> Log:
>> Fix crash in clang.
>>
>> This happened during a recent refactor.  toStringRefArray() returns
>> a vector<StringRef>, which was being implicitly converted to an
>> ArrayRef<StringRef>, and then the vector was immediately being
>> destroyed, so the ArrayRef<> was losing its backing storage.
>> Fix this by making sure the vector gets permanent storage.
>>
>> Modified:
>>     cfe/trunk/lib/Driver/Job.cpp
>>
>> Modified: cfe/trunk/lib/Driver/Job.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Job.cpp?rev=336219&r1=336218&r2=336219&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Driver/Job.cpp (original)
>> +++ cfe/trunk/lib/Driver/Job.cpp Tue Jul  3 11:12:39 2018
>> @@ -318,10 +318,12 @@ int Command::Execute(ArrayRef<llvm::Opti
>>    SmallVector<const char*, 128> Argv;
>>
>>    Optional<ArrayRef<StringRef>> Env;
>> +  std::vector<StringRef> ArgvVectorStorage;
>>    if (!Environment.empty()) {
>>      assert(Environment.back() == nullptr &&
>>             "Environment vector should be null-terminated by now");
>> -    Env = llvm::toStringRefArray(Environment.data());
>> +    ArgvVectorStorage = llvm::toStringRefArray(Environment.data());
>> +    Env = makeArrayRef(ArgvVectorStorage);
>>    }
>>
>>    if (ResponseFile == nullptr) {
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180709/96cab977/attachment.html>