https://github.com/AdvenamTacet created https://github.com/llvm/llvm-project/pull/79522
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. >From 804b9c1bf634585537306ac092508e7eb4e7ca60 Mon Sep 17 00:00:00 2001 From: Advenam Tacet <advenam.ta...@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 f1d07d022b25848..75cd16c0ae63d28 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; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
