[PATCH] D39086: Performance tracing facility for clangd.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added inline comments.


================
Comment at: clangd/Trace.cpp:87
+
+static Tracer* T = nullptr;
+} // namespace
----------------
ilya-biryukov wrote:
> Maybe use `unique_ptr<Tracer>` (or even `llvm::Optional<Tracer>`)? Otherwise we leak memory and don't flush the stream if users of tracing API forget to call `stop()`. (I think I'm on the same page with @ioeric here  about forgetting a call to `stop()`).
>  
> Having a static global of non-pod type is a no-go, but we could use static local variables:
> 
> ```
> llvm::Optional<Tracer>& getTracerInstance() {
>  static llvm::Optional<Tracer> Tracer;
>  return Tracer;
> }
> ```
So Eric's solution of a scoped object that would be owned by main() makes sense to me, and I'm happy to add it (though personally not sure it carries its weight).

However I don't understand *your* suggestion :-)
> Having a static global of non-pod type is a no-go, but we could use static local variables:
>     llvm::Optional<Tracer>& getTracerInstance() {
>       static llvm::Optional<Tracer> Tracer;
>       return Tracer;
>     }
So it's not global, which means we're not calling Optional() on startup, which would actually be totally safe (I guess the constructor should be marked as `constexpr`).

But we *are* calling `~Optional()` and therefore `~Tracer()` at shutdown. And maybe `~Tracer()` calls something that logs, and llvm::errs() has already been destroyed... *this* is why avoid static objects, right?

> Otherwise we leak memory and don't flush the stream if users of tracing API forget to call stop()
The intended time to call `stop()` is at the end of the program. If you forget to call it, we won't be leaking memory :-) We indeed won't flush the stream, but that wouldn't be safe (see above).

The other option is the user is calling `start()` and `stop()` multiple times, and they call `start()` twice without stopping. We already assert on this, and anyone who doesn't reproducibly hit that assert is well into racy UB-land (logging methods may be called concurrently with each other, but everything else must be synchronized).

Overall I'm not really seeing what scenario we can detect and safely do something about.


https://reviews.llvm.org/D39086