From dc2538405e6b0f8ea19af23783afabfb4721ce0c Mon Sep 17 00:00:00 2001
From: Advenam Tacet <advenam.tacet@trailofbits.com>
Date: Thu, 25 Jan 2024 23:07:48 +0100
Subject: [PATCH] Extend life of variables in `DiagComparison` in
 `ExprConstant`

This commit makes two variables static extending their life span.
This patch is designed to address the issue of buildbots failing when AddressSanitizer's (ASan) short string annotations are enabled.
It's esentially same as:
- https://github.com/llvm/llvm-project/pull/79489
however, it's less likely to solve the real problem as those strings change (aren't `const`).
I suspect that there may be use after end of life bug (in StringRef), but it requires confirmation.
In that case, one alternative solution, which unfortunately results in memory leaks, is to always allocate new strings instead of overwriting existing (static) ones. This approach would prevent potential data corruption, but I don't suggest it in this PR.

This patch makes `Clang :: SemaCXX/builtins.cpp` test pass with short string annotations (ASan).
With https://github.com/llvm/llvm-project/pull/79489 it fixes known problems with buildbots, while running with short string annotations.
However, the potential issue still requires more investigation therefore FIXME comment is added in that patch.

Short string annotations PR (reverted):
- https://github.com/llvm/llvm-project/pull/79049

Buildbots (failure) output:
- https://lab.llvm.org/buildbot/#/builders/5/builds/40364/steps/9/logs/stdio

While buildbots should not fail with proposed changes, we still should investigate why buildbots were failing with ASan short string annotations turned on.
StringRef objects (made from those strings) can potentially change their contents unexpectedly or even (potentially) use of freed memory may happen.
That interpretation is only my educated guess, I still didn't understand exactly why those buildbots are failing.
---
 clang/lib/AST/ExprConstant.cpp | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index f1d07d022b2584..579f38648b0f22 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -13288,9 +13288,20 @@ EvaluateComparisonBinaryOperator(EvalInfo &Info, const BinaryOperator *E,
     // Reject differing bases from the normal codepath; we special-case
     // comparisons to null.
     if (!HasSameBase(LHSValue, RHSValue)) {
-      auto DiagComparison = [&] (unsigned DiagID, bool Reversed = false) {
-        std::string LHS = LHSValue.toString(Info.Ctx, E->getLHS()->getType());
-        std::string RHS = RHSValue.toString(Info.Ctx, E->getRHS()->getType());
+      auto DiagComparison = [&](unsigned DiagID, bool Reversed = false) {
+        static std::string LHS, RHS;
+        // FIXME: To prevent the use of variables beyond their lifetime, we have
+        // made them static. However, this approach may not fully address the
+        // underlying issue. StringRef objects (made from those strings) can
+        // potentially change their contents unexpectedly.
+        // Or potentially use of freed memory may happen. Therefore, further
+        // investigation is required to ensure that making those variables
+        // static effectively resolves the problem.
+        // We should investigate why buildbots were failing with ASan short
+        // string annotations turned on. Related PR:
+        // https://github.com/llvm/llvm-project/pull/79049
+        LHS = LHSValue.toString(Info.Ctx, E->getLHS()->getType());
+        RHS = RHSValue.toString(Info.Ctx, E->getRHS()->getType());
         Info.FFDiag(E, DiagID)
             << (Reversed ? RHS : LHS) << (Reversed ? LHS : RHS);
         return false;