On Mon, Jul 9, 2018 at 1:52 PM Zachary Turner <ztur...@google.com> 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 <dblai...@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@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@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
