[clang] [AST] Use APIntStorage to fix memory leak in EnumConstantDecl. (PR #78311)
Craig Topper via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 16 10:41:10 PST 2024
https://github.com/topperc updated https://github.com/llvm/llvm-project/pull/78311
>From a8345976354bff4a05a7e0ff28bae152039f5458 Mon Sep 17 00:00:00 2001
From: Craig Topper <craig.topper at sifive.com>
Date: Mon, 15 Jan 2024 22:43:55 -0800
Subject: [PATCH 1/2] [AST] Use APIntStorage to fix memory leak in
EnumConstantDecl.
EnumConstantDecl is allocated by the ASTContext allocator so the
destructor is never called.
This patch takes a similar approach to IntegerLiteral by using
APIntStorage to allocate large APSInts using the ASTContext
allocator as well.
The downside is that an additional heap allocation and copy of the
data needs to be made when calling getInitValue if the APSInt is
large.
Fixes #78160.
---
clang/include/clang/AST/APNumericStorage.h | 71 ++++++++++++++++++++++
clang/include/clang/AST/Decl.h | 21 ++++---
clang/include/clang/AST/Expr.h | 52 +---------------
clang/lib/AST/Decl.cpp | 11 +++-
clang/lib/CodeGen/CGBuiltin.cpp | 2 +-
clang/lib/Sema/SemaDecl.cpp | 2 +-
clang/lib/Serialization/ASTReaderDecl.cpp | 2 +-
7 files changed, 98 insertions(+), 63 deletions(-)
create mode 100644 clang/include/clang/AST/APNumericStorage.h
diff --git a/clang/include/clang/AST/APNumericStorage.h b/clang/include/clang/AST/APNumericStorage.h
new file mode 100644
index 00000000000000..c13014b1bad4de
--- /dev/null
+++ b/clang/include/clang/AST/APNumericStorage.h
@@ -0,0 +1,71 @@
+//===--- APNumericStorage.h - Store APInt/APFloat in ASTContext -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_AST_APNUMERICSTORAGE_H
+#define LLVM_CLANG_AST_APNUMERICSTORAGE_H
+
+#include "llvm/ADT/APFloat.h"
+#include "llvm/ADT/APInt.h"
+
+namespace clang {
+class ASTContext;
+
+/// Used by IntegerLiteral/FloatingLiteral to store the numeric without
+/// leaking memory.
+///
+/// For large floats/integers, APFloat/APInt will allocate memory from the heap
+/// to represent these numbers. Unfortunately, when we use a BumpPtrAllocator
+/// to allocate IntegerLiteral/FloatingLiteral nodes the memory associated with
+/// the APFloat/APInt values will never get freed. APNumericStorage uses
+/// ASTContext's allocator for memory allocation.
+class APNumericStorage {
+ union {
+ uint64_t VAL; ///< Used to store the <= 64 bits integer value.
+ uint64_t *pVal; ///< Used to store the >64 bits integer value.
+ };
+ unsigned BitWidth;
+
+ bool hasAllocation() const { return llvm::APInt::getNumWords(BitWidth) > 1; }
+
+ APNumericStorage(const APNumericStorage &) = delete;
+ void operator=(const APNumericStorage &) = delete;
+
+protected:
+ APNumericStorage() : VAL(0), BitWidth(0) {}
+
+ llvm::APInt getIntValue() const {
+ unsigned NumWords = llvm::APInt::getNumWords(BitWidth);
+ if (NumWords > 1)
+ return llvm::APInt(BitWidth, NumWords, pVal);
+ else
+ return llvm::APInt(BitWidth, VAL);
+ }
+ void setIntValue(const ASTContext &C, const llvm::APInt &Val);
+};
+
+class APIntStorage : private APNumericStorage {
+public:
+ llvm::APInt getValue() const { return getIntValue(); }
+ void setValue(const ASTContext &C, const llvm::APInt &Val) {
+ setIntValue(C, Val);
+ }
+};
+
+class APFloatStorage : private APNumericStorage {
+public:
+ llvm::APFloat getValue(const llvm::fltSemantics &Semantics) const {
+ return llvm::APFloat(Semantics, getIntValue());
+ }
+ void setValue(const ASTContext &C, const llvm::APFloat &Val) {
+ setIntValue(C, Val.bitcastToAPInt());
+ }
+};
+
+} // end namespace clang
+
+#endif // LLVM_CLANG_AST_APNUMERICSTORAGE_H
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index a807bcdd76b30d..98af552d400f3a 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -13,6 +13,7 @@
#ifndef LLVM_CLANG_AST_DECL_H
#define LLVM_CLANG_AST_DECL_H
+#include "clang/AST/APNumericStorage.h"
#include "clang/AST/APValue.h"
#include "clang/AST/ASTContextAllocate.h"
#include "clang/AST/DeclAccessPair.h"
@@ -3251,15 +3252,16 @@ class FieldDecl : public DeclaratorDecl, public Mergeable<FieldDecl> {
/// that is defined. For example, in "enum X {a,b}", each of a/b are
/// EnumConstantDecl's, X is an instance of EnumDecl, and the type of a/b is a
/// TagType for the X EnumDecl.
-class EnumConstantDecl : public ValueDecl, public Mergeable<EnumConstantDecl> {
+class EnumConstantDecl : public ValueDecl,
+ public Mergeable<EnumConstantDecl>,
+ public APIntStorage {
Stmt *Init; // an integer constant expression
- llvm::APSInt Val; // The value.
+ bool IsUnsigned;
protected:
- EnumConstantDecl(DeclContext *DC, SourceLocation L,
+ EnumConstantDecl(const ASTContext &C, DeclContext *DC, SourceLocation L,
IdentifierInfo *Id, QualType T, Expr *E,
- const llvm::APSInt &V)
- : ValueDecl(EnumConstant, DC, L, Id, T), Init((Stmt*)E), Val(V) {}
+ const llvm::APSInt &V);
public:
friend class StmtIteratorBase;
@@ -3272,10 +3274,15 @@ class EnumConstantDecl : public ValueDecl, public Mergeable<EnumConstantDecl> {
const Expr *getInitExpr() const { return (const Expr*) Init; }
Expr *getInitExpr() { return (Expr*) Init; }
- const llvm::APSInt &getInitVal() const { return Val; }
+ llvm::APSInt getInitVal() const {
+ return llvm::APSInt(getValue(), IsUnsigned);
+ }
void setInitExpr(Expr *E) { Init = (Stmt*) E; }
- void setInitVal(const llvm::APSInt &V) { Val = V; }
+ void setInitVal(const ASTContext &C, const llvm::APSInt &V) {
+ setValue(C, V);
+ IsUnsigned = V.isUnsigned();
+ }
SourceRange getSourceRange() const override LLVM_READONLY;
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index a41f2d66b37b69..53e4544888ff66 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -13,6 +13,7 @@
#ifndef LLVM_CLANG_AST_EXPR_H
#define LLVM_CLANG_AST_EXPR_H
+#include "clang/AST/APNumericStorage.h"
#include "clang/AST/APValue.h"
#include "clang/AST/ASTVector.h"
#include "clang/AST/ComputeDependence.h"
@@ -1479,57 +1480,6 @@ class DeclRefExpr final
}
};
-/// Used by IntegerLiteral/FloatingLiteral to store the numeric without
-/// leaking memory.
-///
-/// For large floats/integers, APFloat/APInt will allocate memory from the heap
-/// to represent these numbers. Unfortunately, when we use a BumpPtrAllocator
-/// to allocate IntegerLiteral/FloatingLiteral nodes the memory associated with
-/// the APFloat/APInt values will never get freed. APNumericStorage uses
-/// ASTContext's allocator for memory allocation.
-class APNumericStorage {
- union {
- uint64_t VAL; ///< Used to store the <= 64 bits integer value.
- uint64_t *pVal; ///< Used to store the >64 bits integer value.
- };
- unsigned BitWidth;
-
- bool hasAllocation() const { return llvm::APInt::getNumWords(BitWidth) > 1; }
-
- APNumericStorage(const APNumericStorage &) = delete;
- void operator=(const APNumericStorage &) = delete;
-
-protected:
- APNumericStorage() : VAL(0), BitWidth(0) { }
-
- llvm::APInt getIntValue() const {
- unsigned NumWords = llvm::APInt::getNumWords(BitWidth);
- if (NumWords > 1)
- return llvm::APInt(BitWidth, NumWords, pVal);
- else
- return llvm::APInt(BitWidth, VAL);
- }
- void setIntValue(const ASTContext &C, const llvm::APInt &Val);
-};
-
-class APIntStorage : private APNumericStorage {
-public:
- llvm::APInt getValue() const { return getIntValue(); }
- void setValue(const ASTContext &C, const llvm::APInt &Val) {
- setIntValue(C, Val);
- }
-};
-
-class APFloatStorage : private APNumericStorage {
-public:
- llvm::APFloat getValue(const llvm::fltSemantics &Semantics) const {
- return llvm::APFloat(Semantics, getIntValue());
- }
- void setValue(const ASTContext &C, const llvm::APFloat &Val) {
- setIntValue(C, Val.bitcastToAPInt());
- }
-};
-
class IntegerLiteral : public Expr, public APIntStorage {
SourceLocation Loc;
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index e1440e5183a4e6..d373329d7272a4 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -5369,16 +5369,23 @@ void CapturedDecl::setBody(Stmt *B) { BodyAndNothrow.setPointer(B); }
bool CapturedDecl::isNothrow() const { return BodyAndNothrow.getInt(); }
void CapturedDecl::setNothrow(bool Nothrow) { BodyAndNothrow.setInt(Nothrow); }
+EnumConstantDecl::EnumConstantDecl(const ASTContext &C, DeclContext *DC,
+ SourceLocation L, IdentifierInfo *Id,
+ QualType T, Expr *E, const llvm::APSInt &V)
+ : ValueDecl(EnumConstant, DC, L, Id, T), Init((Stmt *)E) {
+ setInitVal(C, V);
+}
+
EnumConstantDecl *EnumConstantDecl::Create(ASTContext &C, EnumDecl *CD,
SourceLocation L,
IdentifierInfo *Id, QualType T,
Expr *E, const llvm::APSInt &V) {
- return new (C, CD) EnumConstantDecl(CD, L, Id, T, E, V);
+ return new (C, CD) EnumConstantDecl(C, CD, L, Id, T, E, V);
}
EnumConstantDecl *
EnumConstantDecl::CreateDeserialized(ASTContext &C, unsigned ID) {
- return new (C, ID) EnumConstantDecl(nullptr, SourceLocation(), nullptr,
+ return new (C, ID) EnumConstantDecl(C, nullptr, SourceLocation(), nullptr,
QualType(), nullptr, llvm::APSInt());
}
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 1ed35befe1361f..ee6866dabaa89b 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -13047,7 +13047,7 @@ Value *CodeGenFunction::EmitBPFBuiltinExpr(unsigned BuiltinID,
const auto *DR = cast<DeclRefExpr>(CE->getSubExpr());
const auto *Enumerator = cast<EnumConstantDecl>(DR->getDecl());
- auto &InitVal = Enumerator->getInitVal();
+ auto InitVal = Enumerator->getInitVal();
std::string InitValStr;
if (InitVal.isNegative() || InitVal > uint64_t(INT64_MAX))
InitValStr = std::to_string(InitVal.getSExtValue());
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 722e2ac9e4ff8d..793bb90da74d28 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -20214,7 +20214,7 @@ void Sema::ActOnEnumBody(SourceLocation EnumLoc, SourceRange BraceRange,
// Adjust the APSInt value.
InitVal = InitVal.extOrTrunc(NewWidth);
InitVal.setIsSigned(NewSign);
- ECD->setInitVal(InitVal);
+ ECD->setInitVal(Context, InitVal);
// Adjust the Expr initializer and type.
if (ECD->getInitExpr() &&
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 547eb77930b4ee..b05efe4f76517e 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -910,7 +910,7 @@ void ASTDeclReader::VisitEnumConstantDecl(EnumConstantDecl *ECD) {
VisitValueDecl(ECD);
if (Record.readInt())
ECD->setInitExpr(Record.readExpr());
- ECD->setInitVal(Record.readAPSInt());
+ ECD->setInitVal(Reader.getContext(), Record.readAPSInt());
mergeMergeable(ECD);
}
>From 64cc337eb24953695fbc2b9551c292e418ebd052 Mon Sep 17 00:00:00 2001
From: Craig Topper <craig.topper at sifive.com>
Date: Tue, 16 Jan 2024 10:40:51 -0800
Subject: [PATCH 2/2] fixup! mention in release notes.
---
clang/docs/ReleaseNotes.rst | 2 ++
1 file changed, 2 insertions(+)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 174392da17551e..6632a817c07c20 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -675,6 +675,8 @@ Bug Fixes in This Version
Fixes (`#67317 <https://github.com/llvm/llvm-project/issues/67317>`_)
- Clang now properly diagnoses use of stand-alone OpenMP directives after a
label (including ``case`` or ``default`` labels).
+- Fix compiler memory leak for enums with underlying type larger than 64 bits.
+ Fixes (`#78311 <https://github.com/llvm/llvm-project/pull/78311>`_)
Before:
More information about the cfe-commits
mailing list