Skip to content

Conversation

@perry-ca
Copy link
Contributor

The code in getLocalDataSize() returns the sum of the size of the LocalData plus the size of the extra data. The start of the extra data is padded so it starts on a multiple of it's alignment. We also need to be adding tail padding so the final size is a multiple of the alignment of the LocalData. On most systems the alignment of the extra data is the same or greater than the alignment of the LocalData so you don't need the tail padding. However, on z/OS, the alignment of the extra data is less than the alignment of the LocalData and thus you do need the tail padding to make the final size a multiple of the LocalData alignment.

The extra data is the WrittenBuiltinSpecs struct. This struct is just a struct of bitfields. On most systems the alignment of the struct is determined by the type of the bitfields (eg. unsigned int -> align of 4). On z/OS, all bitfields are 1 byte aligned. Thus on z/OS WrittenBuiltinSpecs is only size 2 with alignment of 1 (verses 4 & 4).

@perry-ca perry-ca self-assigned this Jan 13, 2025
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 13, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 13, 2025

@llvm/pr-subscribers-clang

Author: Sean Perry (perry-ca)

Changes

The code in getLocalDataSize() returns the sum of the size of the LocalData plus the size of the extra data. The start of the extra data is padded so it starts on a multiple of it's alignment. We also need to be adding tail padding so the final size is a multiple of the alignment of the LocalData. On most systems the alignment of the extra data is the same or greater than the alignment of the LocalData so you don't need the tail padding. However, on z/OS, the alignment of the extra data is less than the alignment of the LocalData and thus you do need the tail padding to make the final size a multiple of the LocalData alignment.

The extra data is the WrittenBuiltinSpecs struct. This struct is just a struct of bitfields. On most systems the alignment of the struct is determined by the type of the bitfields (eg. unsigned int -> align of 4). On z/OS, all bitfields are 1 byte aligned. Thus on z/OS WrittenBuiltinSpecs is only size 2 with alignment of 1 (verses 4 & 4).


Full diff: https://github.com/llvm/llvm-project/pull/122761.diff

1 Files Affected:

  • (modified) clang/include/clang/AST/TypeLoc.h (+1)
diff --git a/clang/include/clang/AST/TypeLoc.h b/clang/include/clang/AST/TypeLoc.h
index 62ca52e508ba20..a55a38335ef6a6 100644
--- a/clang/include/clang/AST/TypeLoc.h
+++ b/clang/include/clang/AST/TypeLoc.h
@@ -397,6 +397,7 @@ class ConcreteTypeLoc : public Base {
     unsigned extraAlign = asDerived()->getExtraLocalDataAlignment();
     size = llvm::alignTo(size, extraAlign);
     size += asDerived()->getExtraLocalDataSize();
+    size = llvm::alignTo(size, asDerived()->getLocalDataAlignment());
     return size;
   }
 

@perry-ca perry-ca requested a review from redstar January 13, 2025 18:28
@redstar
Copy link
Member

redstar commented Jan 17, 2025

Change LGTM, however I think there should be a test checking the tail padding.

@perry-ca
Copy link
Contributor Author

This is an internal data structure so hard to write a test case. The clang code does have assertions that fail if the size isn't a multiple of the alignment.

Copy link
Member

@redstar redstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@perry-ca perry-ca merged commit 89305c3 into llvm:main Jan 17, 2025
11 checks passed
@perry-ca perry-ca deleted the perry/zos-typeloc-size branch January 17, 2025 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants