In the PR for placeholder types I ran into an intermittently reproducible issue with diagnostics that was causing error messages to get corrupted in memory. I've addressed the issue but don't really understand the full story, so if anyone has the bandwidth to take a look and see if anything jumps out as obviously incorrect, I'd really appreciate it!
The issue arose when running the diagnostic verifier on the following code:
// expected-error @+1 {{result type '(U) -> Float' does not conform to 'Differentiable', but the enclosing function type is '@differentiable'}}
func test2<T: Differentiable, U: Differentiable>(_: @differentiable(reverse) (T) -> (U) -> Float) {}
Using the address sanitizer let me trace the issue back to the following call to DiagnosticEngine::diagnose:
auto diag = ctx.Diags.diagnose(
diagLoc, diag::differentiable_function_type_invalid_result,
fnTy->getResult()->getString(), isLinear);
Specifically, the sanitizer complained about a stack-use-after-scope when the diagnostic was flushed at the end of the scope. I was able to (apparently) solve the issue by pulling out fnTy->getResult()->getString() into a local resultStr variable before passing it to diagnose (as seen in the current state of the PR), but I'm not entirely clear why the previous formulation was problematic.
DiagnosticVerifier::diagnose does a lot of template metaprogramming along with std::move to forward its arguments on to the Diagnostic that gets produced, and I'm not quite clear enough on the subtleties of those semantics to know whether that would cause the issue.
My best guess is that DiagnosticEnginge::diagnose expects that all its arguments will live at least as long as the InFlightDiagnostic, and passing the result of getString() as a temporary breaks that assumption since the produced string's lifetime ends as soon as the expression is evaluated (right?). But if that's wrong or if there's more to the story, I'd love to know!
In the original code, fnTy->getResult()->getString() creates a temporary std::string which is guaranteed to only live for the duration of the call to diagnose. The diagnostic takes a StringRef argument as defined in DiagnosticSema.def, which refers to the same storage as the std::string thanks to this implicit conversion.
/// Construct a string ref from an std::string.
/*implicit*/ StringRef(const std::string &Str)
: Data(Str.data()), Length(Str.length()) {}
By creating an explicit variable, the lifetime of the temporary becomes longer, so it is still alive when the diagnostic is flushed (since destructors run in reverse order, when the diagnostic's destructor is run, the std::string which was created before the diagnostic is still alive).
--
That's a long way of saying that this
My best guess is that DiagnosticEnginge::diagnose expects that all its arguments will live at least as long as the InFlightDiagnostic , and passing the result of getString() as a temporary breaks that assumption since the produced string's lifetime ends as soon as the expression is evaluated (right?).
In this case, is that because the temporary InFlightDiagnostic that results from the call to diagnose() is part of the same expression as the temporary std::string resulting from the call to getString() and so reverse-construction order dictates that the diagnostic will be flushed while the std::string temporary is still alive?
All temporary objects are destroyed as the last step in evaluating the full-expression that (lexically) contains the point where they were created, and if multiple temporary objects were created, they are destroyed in the order opposite to the order of creation.
So yeah, since the InFlightDiagnostic is created later as part of the same full-expression but later than the std::string, it is destroyed earlier than the std::string.
Great, that's super helpful. Just wanted to get a better understanding of the precise semantics here to make sure that the fix was actually solving the underlying issue and not just making it less likely to manifest in corruption.
