<div dir="ltr">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.<div><br></div><div>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.</div><div><br></div><div>(Also not sure why our sanitizer bots didn't catch it).</div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Jul 9, 2018 at 1:42 PM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Did this fail on an existing regression test, or is there a need for more test coverage? (guessing it failed on existing tests)<br><br>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?</div><br><div class="gmail_quote"><div dir="ltr">On Tue, Jul 3, 2018 at 11:17 AM Zachary Turner via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: zturner<br>
Date: Tue Jul  3 11:12:39 2018<br>
New Revision: 336219<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=336219&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=336219&view=rev</a><br>
Log:<br>
Fix crash in clang.<br>
<br>
This happened during a recent refactor.  toStringRefArray() returns<br>
a vector<StringRef>, which was being implicitly converted to an<br>
ArrayRef<StringRef>, and then the vector was immediately being<br>
destroyed, so the ArrayRef<> was losing its backing storage.<br>
Fix this by making sure the vector gets permanent storage.<br>
<br>
Modified:<br>
    cfe/trunk/lib/Driver/Job.cpp<br>
<br>
Modified: cfe/trunk/lib/Driver/Job.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Job.cpp?rev=336219&r1=336218&r2=336219&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Job.cpp?rev=336219&r1=336218&r2=336219&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Driver/Job.cpp (original)<br>
+++ cfe/trunk/lib/Driver/Job.cpp Tue Jul  3 11:12:39 2018<br>
@@ -318,10 +318,12 @@ int Command::Execute(ArrayRef<llvm::Opti<br>
   SmallVector<const char*, 128> Argv;<br>
<br>
   Optional<ArrayRef<StringRef>> Env;<br>
+  std::vector<StringRef> ArgvVectorStorage;<br>
   if (!Environment.empty()) {<br>
     assert(Environment.back() == nullptr &&<br>
            "Environment vector should be null-terminated by now");<br>
-    Env = llvm::toStringRefArray(Environment.data());<br>
+    ArgvVectorStorage = llvm::toStringRefArray(Environment.data());<br>
+    Env = makeArrayRef(ArgvVectorStorage);<br>
   }<br>
<br>
   if (ResponseFile == nullptr) {<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>
</blockquote></div>