Re: r336219 - Fix crash in clang.
On Mon, Jul 9, 2018 at 1:52 PM Zachary Turner wrote:
> 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.
>
Fair enough. Given that we do StringRef S; S = x; all the time without an
explicit conversion function, etc, I'm not sure this case merits any
different handling - but I don't mind too much if you prefer to keep it.
> 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.
>
Yeah - UB is UB unfortunately. Given that any execution of this code (well,
one that actually has a non-zero sized result & uses that result later on -
I assume that's the case in at least some of the existing tests in Clang?)
does tickle the bug, but whether or not it actually crashes is unknown -
I'm OK leaving that as-is.
> (Also not sure why our sanitizer bots didn't catch it).
>
Might be interesting to look into/reduce a test case, but may also be a bit
arduous - not sure.
>
> On Mon, Jul 9, 2018 at 1:42 PM David Blaikie 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@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, which was being implicitly converted to an
>>> ArrayRef, 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>>SmallVector Argv;
>>>
>>>Optional> Env;
>>> + std::vector 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@lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r336219 - Fix crash in clang.
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 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@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, which was being implicitly converted to an
>> ArrayRef, 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>SmallVector Argv;
>>
>>Optional> Env;
>> + std::vector 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@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r336219 - Fix crash in clang.
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@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, which was being implicitly converted to an
> ArrayRef, 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(ArrayRefSmallVector Argv;
>
>Optional> Env;
> + std::vector 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@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r336219 - Fix crash in clang.
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, which was being implicitly converted to an
ArrayRef, 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 Argv;
Optional> Env;
+ std::vector 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@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
