Skip to content

Commit 6175f0b

Browse files
author
Advenam Tacet
committed
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: - #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 #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): - #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.
1 parent 4792f91 commit 6175f0b

File tree

1 file changed

+14
-3
lines changed

1 file changed

+14
-3
lines changed

clang/lib/AST/ExprConstant.cpp

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13288,9 +13288,20 @@ EvaluateComparisonBinaryOperator(EvalInfo &Info, const BinaryOperator *E,
1328813288
// Reject differing bases from the normal codepath; we special-case
1328913289
// comparisons to null.
1329013290
if (!HasSameBase(LHSValue, RHSValue)) {
13291-
auto DiagComparison = [&] (unsigned DiagID, bool Reversed = false) {
13292-
std::string LHS = LHSValue.toString(Info.Ctx, E->getLHS()->getType());
13293-
std::string RHS = RHSValue.toString(Info.Ctx, E->getRHS()->getType());
13291+
auto DiagComparison = [&](unsigned DiagID, bool Reversed = false) {
13292+
static std::string LHS, RHS;
13293+
// FIXME: To prevent the use of variables beyond their lifetime, we have
13294+
// made them static. However, this approach may not fully address the
13295+
// underlying issue. StringRef objects (made from those strings) can
13296+
// potentially change their contents unexpectedly.
13297+
// Or potentially use of freed memory may happen. Therefore, further
13298+
// investigation is required to ensure that making those variables
13299+
// static effectively resolves the problem.
13300+
// We should investigate why buildbots were failing with ASan short
13301+
// string annotations turned on. Related PR:
13302+
// https://github.com/llvm/llvm-project/pull/79049
13303+
LHS = LHSValue.toString(Info.Ctx, E->getLHS()->getType());
13304+
RHS = RHSValue.toString(Info.Ctx, E->getRHS()->getType());
1329413305
Info.FFDiag(E, DiagID)
1329513306
<< (Reversed ? RHS : LHS) << (Reversed ? LHS : RHS);
1329613307
return false;

0 commit comments

Comments
 (0)