Skip to content

Commit

Permalink
Extend life of variables in DiagComparison in ExprConstant
Browse files Browse the repository at this point in the history
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:
- llvm#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 llvm#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):
- llvm#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.
  • Loading branch information
Advenam Tacet committed Jan 25, 2024
1 parent e8a5010 commit dc25384
Showing 1 changed file with 14 additions and 3 deletions.
17 changes: 14 additions & 3 deletions clang/lib/AST/ExprConstant.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit dc25384

Please sign in to comment.