ahatanak updated this revision to Diff 48241.
ahatanak added a comment.
Address some of Manman's review comments.
- Add comments that explain what the code is doing (I should add some
high-level comments too later).
- Simplify the if-else statement.
I haven't implemented the other changes you've suggested, because I wasn't sure
whether imitating the code in "LocalAlignment == 4" would improve readability
of the code. For example, I can declare variable "unsigned Padding =
NumBytesAtAlign4 %8", but the variable alone doesn't tell you whether there is
padding or not (if we haven't inserted any 8-byte elements, we won't have any
paddings, but NumBytesAtAlign4 % 8 can still be non-zero).
Maybe I didn't understand what you were suggesting. Could you elaborate a bit
more on which part of this code is perhaps hard to understand and how we can
improve it?
http://reviews.llvm.org/D16843
Files:
lib/Sema/TypeLocBuilder.cpp
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.cpp
===================================================================
--- lib/Sema/TypeLocBuilder.cpp
+++ lib/Sema/TypeLocBuilder.cpp
@@ -115,11 +115,32 @@
NumBytesAtAlign4 += LocalSize;
}
} else if (LocalAlignment == 8) {
- if (!NumBytesAtAlign8 && NumBytesAtAlign4 % 8 != 0) {
- // No existing padding and misaligned members; add in 4 bytes padding
- memmove(&Buffer[Index - 4], &Buffer[Index], NumBytesAtAlign4);
- Index -= 4;
+ // If the new index is not aligned on an 8-byte boundary, adjust the
+ // current index.
+ bool AdjustIndex = (Index - LocalSize) % 8;
+
+ if (AdjustIndex) {
+ // We know there is a 4-byte padding if an 8-byte aligned element has
been
+ // inserted and the total size of the 4-byte aligned elements that have
+ // been inserted since the last 8-byte aligned element was inserted is
not
+ // a multiple of 4.
+ bool CurrentlyHasPadding = NumBytesAtAlign8 && NumBytesAtAlign4 % 8;
+
+ if (CurrentlyHasPadding) {
+ // There is a 4-byte padding that starts at Index+NumBytesAtAlign4, so
+ // we can remove it and shift the 4-byte elements backwards to make
sure
+ // the new element is 8-byte aligned.
+ memmove(&Buffer[Index + 4], &Buffer[Index], NumBytesAtAlign4);
+ Index += 4;
+ } else {
+ // There is no 4-byte padding we can remove, so we have to insert a
+ // 4-byte padding and shift the 4-byte elements forward to make sure
+ // the new element is 8-byte aligned.
+ memmove(&Buffer[Index - 4], &Buffer[Index], NumBytesAtAlign4);
+ Index -= 4;
+ }
}
+
// Forget about any padding.
NumBytesAtAlign4 = 0;
NumBytesAtAlign8 += LocalSize;
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.cpp
===================================================================
--- lib/Sema/TypeLocBuilder.cpp
+++ lib/Sema/TypeLocBuilder.cpp
@@ -115,11 +115,32 @@
NumBytesAtAlign4 += LocalSize;
}
} else if (LocalAlignment == 8) {
- if (!NumBytesAtAlign8 && NumBytesAtAlign4 % 8 != 0) {
- // No existing padding and misaligned members; add in 4 bytes padding
- memmove(&Buffer[Index - 4], &Buffer[Index], NumBytesAtAlign4);
- Index -= 4;
+ // If the new index is not aligned on an 8-byte boundary, adjust the
+ // current index.
+ bool AdjustIndex = (Index - LocalSize) % 8;
+
+ if (AdjustIndex) {
+ // We know there is a 4-byte padding if an 8-byte aligned element has been
+ // inserted and the total size of the 4-byte aligned elements that have
+ // been inserted since the last 8-byte aligned element was inserted is not
+ // a multiple of 4.
+ bool CurrentlyHasPadding = NumBytesAtAlign8 && NumBytesAtAlign4 % 8;
+
+ if (CurrentlyHasPadding) {
+ // There is a 4-byte padding that starts at Index+NumBytesAtAlign4, so
+ // we can remove it and shift the 4-byte elements backwards to make sure
+ // the new element is 8-byte aligned.
+ memmove(&Buffer[Index + 4], &Buffer[Index], NumBytesAtAlign4);
+ Index += 4;
+ } else {
+ // There is no 4-byte padding we can remove, so we have to insert a
+ // 4-byte padding and shift the 4-byte elements forward to make sure
+ // the new element is 8-byte aligned.
+ memmove(&Buffer[Index - 4], &Buffer[Index], NumBytesAtAlign4);
+ Index -= 4;
+ }
}
+
// Forget about any padding.
NumBytesAtAlign4 = 0;
NumBytesAtAlign8 += LocalSize;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
