[PATCH] D44000: [clang] Fix use-after-free on code completion

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

This feels like a bug in the underlying clang libraries, but since none of the lifetimes are documented and everyone just does it this way...



================
Comment at: clangd/CodeComplete.cpp:431
 // It doesn't do scoring or conversion to CompletionItem yet, as we want to
 // merge with index results first.
 struct CompletionRecorder : public CodeCompleteConsumer {
----------------
maybe comment:  'generally the fields/methods should only be used from within the callback'


================
Comment at: clangd/CodeComplete.cpp:470
     }
+    if (ResultsCallback)
+      ResultsCallback();
----------------
I think this check is dead - remove?


================
Comment at: clangd/CodeComplete.cpp:660
 // Invokes Sema code completion on a file.
 // Callback will be invoked once completion is done, but before cleaning up.
 bool semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer,
----------------
remove this comment


================
Comment at: clangd/CodeComplete.cpp:665
-                      llvm::function_ref<void()> Callback = nullptr) {
+                      const SemaCompleteInput &Input) {
   auto Tracer = llvm::make_unique<trace::Span>("Sema completion");
   std::vector<const char *> ArgStrs;
----------------
this can be stack-allocated, as we don't need to reset it


================
Comment at: clangd/CodeComplete.cpp:737
-
   Tracer = llvm::make_unique<trace::Span>("Sema completion cleanup");
   Action.EndSourceFile();
----------------
this tracer isn't needed anymore as discussed


================
Comment at: clangd/CodeComplete.cpp:816
 //
 // So we start Sema completion first, but defer its cleanup until we're done.
 // We use the Sema context information to query the index.
----------------
this comment should probably be ", and do all our work in its callback"


================
Comment at: clangd/CodeComplete.cpp:852
     // We run Sema code completion first. It builds an AST and calculates:
     //   - completion results based on the AST. These are saved for merging.
     //   - partial identifier and context. We need these for the index query.
----------------
remove "these are saved for merging"



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44000