ahatanak created this revision.
ahatanak added reviewers: doug.gregor, rjmccall, vsk.
ahatanak added a subscriber: cfe-commits.
This patch fixes a bug in the code between line 117-126 of TypeLocBuilder.cpp
which was causing the assert in line 132.
The bug manifests itself when an element of size=4 and alignment=4 is inserted
first and then an element of size=28 and alignment=8 is inserted next. The code
inserts a 4-byte padding at the tail of the first element before the second
element is inserted, but that is incorrect and unnecessary since the second
element is correctly aligned without any paddings (it can start at Index=32
because it's size is 28-bytes).
http://reviews.llvm.org/D16843
Files:
lib/Sema/TypeLocBuilder.cpp
lib/Sema/TypeLocBuilder.h
test/SemaObjCXX/typeloc-data-alignment.mm
Index: test/SemaObjCXX/typeloc-data-alignment.mm
===================================================================
--- /dev/null
+++ test/SemaObjCXX/typeloc-data-alignment.mm
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+// Make sure this doesn't crash.
+
+@protocol P
+@end
+template <class T>
+id<P> foo(T) {
+ int i;
+ foo(i);
+}
Index: lib/Sema/TypeLocBuilder.h
===================================================================
--- lib/Sema/TypeLocBuilder.h
+++ lib/Sema/TypeLocBuilder.h
@@ -41,12 +41,13 @@
/// The inline buffer.
enum { BufferMaxAlignment = llvm::AlignOf<void*>::Alignment };
llvm::AlignedCharArray<BufferMaxAlignment, InlineCapacity> InlineBuffer;
- unsigned NumBytesAtAlign4, NumBytesAtAlign8;
+ unsigned NumBytesAtAlign4;
+ bool SeenAlign8;
public:
TypeLocBuilder()
: Buffer(InlineBuffer.buffer), Capacity(InlineCapacity),
- Index(InlineCapacity), NumBytesAtAlign4(0), NumBytesAtAlign8(0)
+ Index(InlineCapacity), NumBytesAtAlign4(0), SeenAlign8(false)
{
}
@@ -80,7 +81,8 @@
LastTy = QualType();
#endif
Index = Capacity;
- NumBytesAtAlign4 = NumBytesAtAlign8 = 0;
+ NumBytesAtAlign4 = 0;
+ SeenAlign8 = false;
}
/// \brief Tell the TypeLocBuilder that the type it is storing has been
Index: lib/Sema/TypeLocBuilder.cpp
===================================================================
--- lib/Sema/TypeLocBuilder.cpp
+++ lib/Sema/TypeLocBuilder.cpp
@@ -68,66 +68,49 @@
LastTy = T;
#endif
- assert(LocalAlignment <= BufferMaxAlignment && "Unexpected alignment");
+ assert(Capacity % 8 == 0 && "Capacity must be a multiple of 8");
+ assert(LocalSize % 4 == 0 && "LocalSize must be a multiple of 4");
+ assert(LocalAlignment <= BufferMaxAlignment &&
+ (LocalAlignment == 8 || LocalAlignment == 4 || LocalSize == 0) &&
+ "Unexpected alignment");
+
+ // Check whether there is padding at the tail of the 4-byte elements.
+ bool HasPadding = (Index + NumBytesAtAlign4) % 8;
+ bool RemoveOrInsertPadding = false;
+ unsigned RequiredCapacity = Capacity - Index + LocalSize;
+
+ // The element being pushed has to be aligned at an 8-byte boundary if its
+ // alignment is 8-byte or an 8-byte aligned element has already been pushed.
+ // In that case, make sure RequiredCapacity is a multiple of 8.
+ if ((SeenAlign8 || LocalAlignment == 8) && RequiredCapacity % 8) {
+ RequiredCapacity += (HasPadding ? -4 : 4);
+ RemoveOrInsertPadding = NumBytesAtAlign4 != 0;
+ }
+
+ unsigned NewCapacity = Capacity;
// If we need to grow, grow by a factor of 2.
- if (LocalSize > Index) {
- size_t RequiredCapacity = Capacity + (LocalSize - Index);
- size_t NewCapacity = Capacity * 2;
- while (RequiredCapacity > NewCapacity)
- NewCapacity *= 2;
+ while (RequiredCapacity > NewCapacity)
+ NewCapacity *= 2;
+
+ if (NewCapacity != Capacity)
grow(NewCapacity);
- }
- // Because we're adding elements to the TypeLoc backwards, we have to
- // do some extra work to keep everything aligned appropriately.
- // FIXME: This algorithm is a absolute mess because every TypeLoc returned
- // needs to be valid. Partial TypeLocs are a terrible idea.
- // FIXME: 4 and 8 are sufficient at the moment, but it's pretty ugly to
- // hardcode them.
- if (LocalAlignment == 4) {
- if (NumBytesAtAlign8 == 0) {
- NumBytesAtAlign4 += LocalSize;
- } else {
- unsigned Padding = NumBytesAtAlign4 % 8;
- if (Padding == 0) {
- if (LocalSize % 8 == 0) {
- // Everything is set: there's no padding and we don't need to add
- // any.
- } else {
- assert(LocalSize % 8 == 4);
- // No existing padding; add in 4 bytes padding
- memmove(&Buffer[Index - 4], &Buffer[Index], NumBytesAtAlign4);
- Index -= 4;
- }
- } else {
- assert(Padding == 4);
- if (LocalSize % 8 == 0) {
- // Everything is set: there's 4 bytes padding and we don't need
- // to add any.
- } else {
- assert(LocalSize % 8 == 4);
- // There are 4 bytes padding, but we don't need any; remove it.
- memmove(&Buffer[Index + 4], &Buffer[Index], NumBytesAtAlign4);
- Index += 4;
- }
- }
- NumBytesAtAlign4 += LocalSize;
- }
- } else if (LocalAlignment == 8) {
- if (!NumBytesAtAlign8 && NumBytesAtAlign4 % 8 != 0) {
- // No existing padding and misaligned members; add in 4 bytes padding
+ if (RemoveOrInsertPadding) {
+ if (HasPadding) // Remove padding.
+ memmove(&Buffer[Index + 4], &Buffer[Index], NumBytesAtAlign4);
+ else // Insert padding.
memmove(&Buffer[Index - 4], &Buffer[Index], NumBytesAtAlign4);
- Index -= 4;
- }
- // Forget about any padding.
+ }
+
+ if (LocalAlignment == 8) {
+ SeenAlign8 ||= LocalSize != 0;
NumBytesAtAlign4 = 0;
- NumBytesAtAlign8 += LocalSize;
- } else {
- assert(LocalSize == 0);
+ } else if (LocalAlignment == 4) {
+ NumBytesAtAlign4 += LocalSize;
}
- Index -= LocalSize;
+ Index = Capacity - RequiredCapacity;
assert(Capacity - Index == TypeLoc::getFullDataSizeForType(T) &&
"incorrect data size provided to CreateTypeSourceInfo!");
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
