<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Jul 9, 2018 at 1:52 PM Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.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">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></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><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.<br></div></div></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div></div><div>(Also not sure why our sanitizer bots didn't catch it).<br></div></div></blockquote><div><br>Might be interesting to look into/reduce a test case, but may also be a bit arduous - not sure.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div></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" target="_blank">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>
</blockquote></div></div>