Skip to content

Commit c7f5024

Browse files
marsupialmarsupial
committed
Fix AllocatedValue for cases when constructor or cling::printValue has thrown/failed.
Previous implementation was both non-conforming and could easily lead to a crash. By delivering a memory as a char buffer, and allowing the client to write into the -1 slot, ValuePrinterSynthesizer can mark that construction has succeeded and the destructor can be called. Additionally this change hides most implementation details of AllocatedValue from cling::Value.
1 parent bf59a74 commit c7f5024

File tree

2 files changed

+205
-52
lines changed

2 files changed

+205
-52
lines changed

‎lib/Interpreter/Value.cpp‎

Lines changed: 80 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,9 @@
2727

2828
#include "llvm/IR/GlobalValue.h"
2929
#include "llvm/IR/Module.h"
30+
#include "llvm/Support/Host.h"
3031
#include "llvm/Support/raw_os_ostream.h"
3132

32-
#include <stddef.h> // offsetof
33-
3433
namespace {
3534

3635
///\brief The allocation starts with this layout; it is followed by the
@@ -53,51 +52,78 @@ namespace {
5352
size_t m_NElements;
5453

5554
///\brief The reference count - once 0, this object will be deallocated.
56-
size_t m_RefCnt;
55+
/// Hopefully 2^55 - 1 references should be enough as the last byte is
56+
/// used for flag storage.
57+
enum { SizeBytes = sizeof(size_t), ConstructedByte = SizeBytes - 1 };
58+
union {
59+
size_t m_Count;
60+
char m_Bytes[SizeBytes];
61+
};
62+
63+
size_t UpdateRefCount(int Amount) {
64+
// Bit shift the bytes used in m_Bytes for representing an integer
65+
// respecting endian-ness and which of those bytes are significant.
66+
assert((Amount == 1 || Amount == -1) && "Invalid amount");
67+
union { size_t m_Count; char m_Bytes[SizeBytes]; } RC = { 0 };
68+
const size_t NBytes = SizeBytes - sizeof(char);
69+
const size_t Endian = llvm::sys::IsBigEndianHost;
70+
::memcpy(&RC.m_Bytes[Endian], &m_Bytes[0], NBytes);
71+
RC.m_Count += Amount;
72+
::memcpy(&m_Bytes[0], &RC.m_Bytes[Endian], NBytes);
73+
return RC.m_Count;
74+
}
75+
76+
static AllocatedValue* FromPtr(void* Ptr) {
77+
return reinterpret_cast<AllocatedValue*>(reinterpret_cast<char*>(Ptr) -
78+
sizeof(AllocatedValue));
79+
}
5780

58-
public:
5981
///\brief Initialize the storage management part of the allocated object.
6082
/// The allocator is referencing it, thus initialize m_RefCnt with 1.
6183
///\param [in] dtorFunc - the function to be called before deallocation.
62-
AllocatedValue(void* dtorFunc, size_t allocSize, size_t nElements) :
63-
m_DtorFunc(cling::utils::VoidToFunctionPtr<DtorFunc_t>(dtorFunc)),
64-
m_AllocSize(allocSize), m_NElements(nElements), m_RefCnt(1)
65-
{}
66-
67-
static constexpr unsigned getPayloadOffset() {
68-
static_assert(std::is_standard_layout<AllocatedValue>::value,
69-
"Cannot use offsetof");
70-
static_assert(((offsetof(AllocatedValue, m_RefCnt) + sizeof(m_RefCnt)) %
71-
sizeof(size_t)) == 0,
72-
"Buffer may not be machine aligned");
73-
return offsetof(AllocatedValue, m_RefCnt) + sizeof(m_RefCnt);
84+
AllocatedValue(size_t Size, size_t NElem, DtorFunc_t Dtor) :
85+
m_DtorFunc(Dtor), m_AllocSize(Size), m_NElements(NElem) {
86+
m_Count = 0;
87+
UpdateRefCount(1);
7488
}
7589

76-
char* getPayload() {
77-
return reinterpret_cast<char*>(&m_RefCnt) + sizeof(m_RefCnt);
78-
}
90+
public:
91+
///\brief Create an AllocatedValue whose lifetime is reference counted.
92+
/// \returns The address of the writeable client data.
93+
static void* Create(size_t Size, size_t NElem, DtorFunc_t Dtor) {
94+
char* Alloc = new char[sizeof(AllocatedValue) + Size];
95+
AllocatedValue* AV = new (Alloc) AllocatedValue(Size, NElem, Dtor);
96+
97+
static_assert(std::is_standard_layout<AllocatedValue>::value, "padding");
98+
static_assert(sizeof(m_Count) == sizeof(m_Bytes), "union padding");
99+
static_assert(((offsetof(AllocatedValue, m_Count) + sizeof(m_Count)) %
100+
SizeBytes) == 0,
101+
"Buffer may not be machine aligned");
102+
assert(&Alloc[sizeof(AllocatedValue) - 1] == &AV->m_Bytes[SizeBytes - 1]
103+
&& "Padded AllocatedValue");
79104

80-
static AllocatedValue* getFromPayload(void* payload) {
81-
return
82-
reinterpret_cast<AllocatedValue*>((char*)payload - getPayloadOffset());
105+
// Give back the first client writable byte.
106+
return AV->m_Bytes + SizeBytes;
83107
}
84108

85-
void Retain() { ++m_RefCnt; }
109+
static void Retain(void* Ptr) {
110+
FromPtr(Ptr)->UpdateRefCount(1);
111+
}
86112

87113
///\brief This object must be allocated as a char array. Deallocate it as
88114
/// such.
89-
void Release() {
90-
assert (m_RefCnt > 0 && "Reference count is already zero.");
91-
if (--m_RefCnt == 0) {
92-
if (m_DtorFunc) {
93-
assert(m_NElements && "No elements!");
94-
char* Payload = getPayload();
95-
const auto Skip = m_AllocSize / m_NElements;
96-
while (m_NElements-- != 0)
97-
(*m_DtorFunc)(Payload + m_NElements * Skip);
115+
static void Release(void* Ptr) {
116+
AllocatedValue* AV = FromPtr(Ptr);
117+
if (AV->UpdateRefCount(-1) == 0) {
118+
if (AV->m_DtorFunc && AV->m_Bytes[ConstructedByte]) {
119+
assert(AV->m_NElements && "No elements!");
120+
char* Payload = reinterpret_cast<char*>(Ptr);
121+
const auto Skip = AV->m_AllocSize / AV->m_NElements;
122+
while (AV->m_NElements-- != 0)
123+
(*AV->m_DtorFunc)(Payload + AV->m_NElements * Skip);
98124
}
99125
this->~AllocatedValue();
100-
delete [] (char*)this;
126+
delete [] reinterpret_cast<char*>(AV);
101127
}
102128
}
103129
};
@@ -109,7 +135,7 @@ namespace cling {
109135
m_Storage(other.m_Storage), m_StorageType(other.m_StorageType),
110136
m_Type(other.m_Type), m_Interpreter(other.m_Interpreter) {
111137
if (other.needsManagedAllocation())
112-
AllocatedValue::getFromPayload(m_Storage.m_Ptr)->Retain();
138+
AllocatedValue::Retain(m_Storage.m_Ptr);
113139
}
114140

115141
Value::Value(clang::QualType clangTy, Interpreter& Interp):
@@ -123,22 +149,22 @@ namespace cling {
123149
Value& Value::operator =(const Value& other) {
124150
// Release old value.
125151
if (needsManagedAllocation())
126-
AllocatedValue::getFromPayload(m_Storage.m_Ptr)->Release();
152+
AllocatedValue::Release(m_Storage.m_Ptr);
127153

128154
// Retain new one.
129155
m_Type = other.m_Type;
130156
m_Storage = other.m_Storage;
131157
m_StorageType = other.m_StorageType;
132158
m_Interpreter = other.m_Interpreter;
133159
if (needsManagedAllocation())
134-
AllocatedValue::getFromPayload(m_Storage.m_Ptr)->Retain();
160+
AllocatedValue::Retain(m_Storage.m_Ptr);
135161
return *this;
136162
}
137163

138164
Value& Value::operator =(Value&& other) {
139165
// Release old value.
140166
if (needsManagedAllocation())
141-
AllocatedValue::getFromPayload(m_Storage.m_Ptr)->Release();
167+
AllocatedValue::Release(m_Storage.m_Ptr);
142168

143169
// Move new one.
144170
m_Type = other.m_Type;
@@ -153,7 +179,7 @@ namespace cling {
153179

154180
Value::~Value() {
155181
if (needsManagedAllocation())
156-
AllocatedValue::getFromPayload(m_Storage.m_Ptr)->Release();
182+
AllocatedValue::Release(m_Storage.m_Ptr);
157183
}
158184

159185
clang::QualType Value::getType() const {
@@ -211,22 +237,24 @@ namespace cling {
211237

212238
void Value::ManagedAllocate() {
213239
assert(needsManagedAllocation() && "Does not need managed allocation");
214-
void* dtorFunc = 0;
215-
clang::QualType DtorType = getType();
240+
const clang::QualType Ty = getType();
241+
clang::QualType DtorTy = Ty;
242+
216243
// For arrays we destruct the elements.
217-
if (const clang::ConstantArrayType* ArrTy
218-
= llvm::dyn_cast<clang::ConstantArrayType>(DtorType.getTypePtr())) {
219-
DtorType = ArrTy->getElementType();
244+
if (const clang::ConstantArrayType* ArrTy =
245+
llvm::dyn_cast<clang::ConstantArrayType>(Ty.getTypePtr())) {
246+
DtorTy = ArrTy->getElementType();
220247
}
221-
if (const clang::RecordType* RTy = DtorType->getAs<clang::RecordType>())
222-
dtorFunc = m_Interpreter->compileDtorCallFor(RTy->getDecl());
223-
224-
const clang::ASTContext& ctx = getASTContext();
225-
const auto payloadSize = ctx.getTypeSizeInChars(getType()).getQuantity();
226-
char* alloc = new char[AllocatedValue::getPayloadOffset() + payloadSize];
227-
AllocatedValue* allocVal = new (alloc) AllocatedValue(dtorFunc, payloadSize,
228-
GetNumberOfElements());
229-
m_Storage.m_Ptr = allocVal->getPayload();
248+
249+
AllocatedValue::DtorFunc_t DtorFunc = nullptr;
250+
if (const clang::RecordType* RTy = DtorTy->getAs<clang::RecordType>()) {
251+
DtorFunc = cling::utils::VoidToFunctionPtr<AllocatedValue::DtorFunc_t>(
252+
m_Interpreter->compileDtorCallFor(RTy->getDecl()));
253+
}
254+
255+
m_Storage.m_Ptr = AllocatedValue::Create(
256+
getASTContext().getTypeSizeInChars(Ty).getQuantity(),
257+
GetNumberOfElements(), DtorFunc);
230258
}
231259

232260
void Value::AssertOnUnsupportedTypeCast() const {

‎test/Prompt/ValuePrinter/Error.C‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
//------------------------------------------------------------------------------
2+
// CLING - the C++ LLVM-based InterpreterG :)
3+
//
4+
// This file is dual-licensed: you can choose to license it under the University
5+
// of Illinois Open Source License or the GNU Lesser General Public License. See
6+
// LICENSE.TXT for details.
7+
//------------------------------------------------------------------------------
8+
9+
// RUN: cat %s | %cling -Xclang -verify 2>&1 | FileCheck %s
10+
11+
#include "cling/Interpreter/Interpreter.h"
12+
#include "cling/Interpreter/Value.h"
13+
#include <string>
14+
#include <stdexcept>
15+
#include <stdio.h>
16+
17+
class Thrower {
18+
int m_Private = 0;
19+
public:
20+
Thrower(int I = 1) : m_Private(I) {
21+
if (I) throw std::runtime_error("Thrower");
22+
}
23+
~Thrower() { printf("~Thrower-%d\n", m_Private); }
24+
};
25+
26+
void barrier() {
27+
static int N = 0;
28+
printf("%d -------------\n", N++);
29+
}
30+
31+
namespace cling {
32+
std::string printValue(const Thrower* T) {
33+
throw std::runtime_error("cling::printValue");
34+
return "";
35+
}
36+
}
37+
38+
barrier();
39+
// CHECK: 0 -------------
40+
41+
42+
// Un-named, so it's not a module static which would trigger std::terminate.
43+
Thrower()
44+
// CHECK-NEXT: >>> Caught a std::exception: 'Thrower'.
45+
// CHECK-NOT: ~Thrower-1
46+
47+
barrier();
48+
// CHECK-NEXT: 1 -------------
49+
50+
Thrower& fstatic() {
51+
static Thrower sThrower;
52+
return sThrower;
53+
}
54+
fstatic()
55+
// CHECK-NEXT: >>> Caught a std::exception: 'Thrower'.
56+
// CHECK-NOT: ~Thrower-1
57+
58+
barrier();
59+
// CHECK-NEXT: 2 -------------
60+
61+
// Must be -new-, throwing from a constructor of a static calls std::terminate!
62+
new Thrower
63+
// CHECK-NEXT: >>> Caught a std::exception: 'Thrower'.
64+
// CHECK-NOT: ~Thrower-1
65+
66+
barrier();
67+
// CHECK-NEXT: 3 -------------
68+
69+
cling::Value V;
70+
gCling->evaluate("Thrower T1(1)", V);
71+
// CHECK-NEXT: >>> Caught a std::exception: 'Thrower'.
72+
V = cling::Value();
73+
// CHECK-NOT: ~Thrower-1
74+
75+
barrier();
76+
// CHECK-NEXT: 4 -------------
77+
78+
gCling->evaluate("Thrower()", V);
79+
// CHECK-NEXT: >>> Caught a std::exception: 'Thrower'.
80+
V = cling::Value();
81+
// CHECK-NOT: ~Thrower-1
82+
83+
barrier();
84+
// CHECK-NEXT: 5 -------------
85+
86+
87+
gCling->evaluate("Thrower T1(1)", V);
88+
// CHECK-NEXT: >>> Caught a std::exception: 'Thrower'.
89+
V = cling::Value();
90+
// CHECK-NOT: ~Thrower-1
91+
92+
barrier();
93+
// CHECK-NEXT: 6 -------------
94+
95+
96+
// Check throwing from cling::printValue doesn't crash.
97+
Thrower T0(0)
98+
// CHECK-NEXT: >>> Caught a std::exception: 'cling::printValue'.
99+
100+
barrier();
101+
// CHECK-NEXT: 7 -------------
102+
103+
gCling->echo("Thrower T0a(0)");
104+
// CHECK-NEXT: ~Thrower-0
105+
// CHECK-NEXT: >>> Caught a std::exception: 'cling::printValue'.
106+
107+
108+
barrier();
109+
// CHECK-NEXT: 8 -------------
110+
111+
gCling->echo("Thrower(0)");
112+
// CHECK-NEXT: ~Thrower-0
113+
// CHECK-NEXT: >>> Caught a std::exception: 'cling::printValue'.
114+
115+
barrier();
116+
// CHECK-NEXT: 9 -------------
117+
118+
119+
// T0 is a valid object and destruction should occur when out of scope.
120+
// CHECK-NOT: ~Thrower-1
121+
// CHECK-NEXT: ~Thrower-0
122+
// CHECK-NOT: ~Thrower-1
123+
124+
// expected-no-diagnostics
125+
.q

0 commit comments

Comments
 (0)