hubert.reinterpretcast added inline comments.
================ Comment at: clang/test/Layout/aix-double-struct-member.cpp:1 +// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \ +// RUN: -fdump-record-layouts -fsyntax-only %s 2>/dev/null | \ ---------------- Xiangling_L wrote: > hubert.reinterpretcast wrote: > > Xiangling_L wrote: > > > hubert.reinterpretcast wrote: > > > > I am concerned that none of the tests actually create an instance of > > > > the classes under test and check the alignment (or related adjustments) > > > > in the IR. That is, we set up the preferred alignment value but don't > > > > check that we use it where we should. > > > > > > > > As it is, it seems array new/delete has problems: > > > > ``` > > > > #include <assert.h> > > > > extern "C" void *calloc(decltype(sizeof 0), decltype(sizeof 0)); > > > > extern "C" void free(void *); > > > > extern "C" int printf(const char *, ...); > > > > > > > > extern void *allocated_ptr; > > > > extern decltype(sizeof 0) allocated_size; > > > > struct B { > > > > double d; > > > > ~B() {} > > > > static void *operator new[](decltype(sizeof 0) sz); > > > > static void operator delete[](void *p, decltype(sizeof 0) sz); > > > > }; > > > > B *allocBp(); > > > > > > > > #ifdef ALLOCBP > > > > void *allocated_ptr; > > > > decltype(sizeof 0) allocated_size; > > > > void *B::operator new[](decltype(sizeof 0) sz) { > > > > void *alloc = calloc(1u, allocated_size = sz); > > > > printf("%p: %s\n", alloc, __PRETTY_FUNCTION__); > > > > printf("%zu\n", sz); > > > > return allocated_ptr = alloc; > > > > } > > > > void B::operator delete[](void *p, decltype(sizeof 0) sz) { > > > > printf("%p: %s\n", p, __PRETTY_FUNCTION__); > > > > printf("%zu\n", sz); > > > > assert(sz == allocated_size); > > > > assert(p == allocated_ptr); > > > > free(p); > > > > } > > > > B *allocBp() { return new B[2]; } > > > > #endif > > > > > > > > #ifdef MAIN > > > > int main(void) { delete[] allocBp(); } > > > > #endif > > > > ``` > > > > > > > > The `xlclang++` invocation from XL C/C++ generates padding before the > > > > 32-bit `new[]` cookie. I'm not seeing that padding with this patch. > > > Thank. I will create more practical testcases as you mentioned in your > > > concern. And regarding to `padding before the 32-bit new[] cookie` issue, > > > I am wondering is that part of `power` alignment rule or what rules do we > > > follow to generate this kind of padding? > > The padding has to do with the alignment. The allocation function returns > > 8-byte aligned memory. The 32-bit cookie takes 4 of the first 8 bytes. The > > type's preferred alignment is 8, so there are 4 bytes of padding. > Regarding with checking the alignment where we use them, AFAIK the > problematic cases include not only the `cookie padding` issue you mentioned > here, but also the alignment of argument type, return type etc. > > So I am wondering does it make sense to have them handled in a separate patch > since this is already a big one? We can use this patch to implement the > correct value of `__alignof` and `alignof` and use a second patch to handle > the places where `we use them where we should`? > Yes, we can scope the patch that way somewhat; however, some cases of "[using the `__alignof__` value] where we should" that is missing is within the determination of the base and field offsets. We should keep those within the scope of this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79719/new/ https://reviews.llvm.org/D79719 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits