Bug 16508 – Alignment of class members is not respected. Affects new, scoped and classInstanceAlignment.

Illustration: class A { align(8192) ubyte[0x400000] i; } class B : A { import core.simd; void16 giveMeMyAlignment; } void main() { import std.stdio, std.traits; writeln(classInstanceAlignment!B); writeln(cast(void*)new B); } Class B's alignment is 8192 (0x2000), but the program output is: 8 7FEAFA8DF000 This indicates that the alignment directive is silently ignored for `new` and that we have no access to the actual member alignment through compile-time reflection. I had to pick a fantasy alignment here, as otherwise the pointers are always rounded to a value larger than the alignment. We need a way to get at the actual effective class instance alignment at compile time and possibly feed that as a compile-time constant into the `align()` directive for efficient `scoped` implementations. Before anyone asks, the `scope` keyword to place objects on the stack is also affected. But that's a general problem with current dmd, that doesn't support stack alignments > 16 bytes. See: https://issues.dlang.org/show_bug.cgi?id=16098

These are actually two separate issues. One is for Phobos: it would seem that classInstanceAlignment doesn't even look at the base. That one has nothing to do with druntime. Second is allocating on a (ludicrous) alignment requirement. If you're still around and willing to split 'em up yourself, please do so.

std.traits.classInstanceAlignment is broken because it uses "alignof" to calculate the maximum alignment of the class fields, and "alignof" on class/struct fields is broken because it returns the alignment of field types instead of the alignment of fields. __traits(classInstanceSize) is broken because it returns a useless value that doesn't account for the trailing padding required for proper alignment of object arrays. GC allocations are also broken as they return misaligned class objects.

> __traits(classInstanceSize) is broken because it returns a useless value that doesn't account for the trailing padding required for proper alignment of object arrays. There's no such thing as class instance arrays, only arrays of class references, so that's a non-issue.

(In reply to kinke from comment #3) > > There's no such thing as class instance arrays, only arrays of class > references, so that's a non-issue. I'm curious why you chose to nitpick - it's unlikely you didn't understand what I meant. Class instance types exist implicitly and are commonly represented by structs. The return type of std.typecons.scoped is an example, as you know. It is reasonable to expect arrays (and, in general, sequentially stored objects) of such structs to be usable. __traits(classInstanceSize) and std.trait.classInstanceAlignment are used almost exclusively to calculate the size/alignment for such structs. Neither produces results usable for that purpose.

(In reply to Max Samukha from comment #4) > I'm curious why you chose to nitpick - it's unlikely you didn't understand > what I meant. Just stumbled upon this issue and wanted to correct this - `classInstanceSize` doesn't need a fix. > Class instance types exist implicitly and are commonly represented by > structs. The return type of std.typecons.scoped is an example, as you know. > It is reasonable to expect arrays (and, in general, sequentially stored > objects) of such structs to be usable. A field like `align(C.alignof) void[__traits(classInstanceSize, C)] buffer` should make sure there's appropriate padding, solely based on the align declaration, which is required anyway and apparently does need fixing.

(In reply to kinke from comment #5) > (In reply to Max Samukha from comment #4) > > I'm curious why you chose to nitpick - it's unlikely you didn't understand > > what I meant. > > Just stumbled upon this issue and wanted to correct this - > `classInstanceSize` doesn't need a fix. > > > Class instance types exist implicitly and are commonly represented by > > structs. The return type of std.typecons.scoped is an example, as you know. > > It is reasonable to expect arrays (and, in general, sequentially stored > > objects) of such structs to be usable. > > A field like `align(C.alignof) void[__traits(classInstanceSize, C)] buffer` > should make sure there's appropriate padding, solely based on the align > declaration, which is required anyway and apparently does need fixing. (I assume you meant align(classInstanceAlignment!C), which currently produces wrong results due to this bug) I'm sorry to disagree. __traits(classInstanceSize) should return the stride of an array of class instances, similarly to what .sizeof does for structs.

(In reply to Max Samukha from comment #6) > (I assume you meant align(classInstanceAlignment!C), which currently > produces wrong results due to this bug) Yeah sorry, of course. We'll probably need a __traits(classInstanceAlignment) I guess, as `align(64) class C { int x; }` won't be inferrable by looking at the field alignments. > I'm sorry to disagree. No problem there. But it's the size the compiler uses (for the init symbol, the TypeInfo.initializer() etc.), not some library bug, and it's working.

(In reply to kinke from comment #7) > (In reply to Max Samukha from comment #6) > > (I assume you meant align(classInstanceAlignment!C), which currently > > produces wrong results due to this bug) > > Yeah sorry, of course. We'll probably need a > __traits(classInstanceAlignment) I guess, as `align(64) class C { int x; }` > won't be inferrable by looking at the field alignments. Yeah, I forgot about the external align. It seems the trait is necessary. > > > I'm sorry to disagree. > > No problem there. But it's the size the compiler uses (for the init symbol, > the TypeInfo.initializer() etc.), not some library bug, and it's working. Bugs will most likely be revealed in those when/if the align attribute is fixed and people start using it.

(In reply to Max Samukha from comment #8) > > No problem there. But it's the size the compiler uses (for the init symbol, > > the TypeInfo.initializer() etc.), not some library bug, and it's working. > > Bugs will most likely be revealed in those when/if the align attribute is > fixed and people start using it. Nope - again, class instances are never stored consecutively. Only wrapper structs can be, and as long as they align the backing buffer as required, all is/will be working fine.

(In reply to kinke from comment #9) > (In reply to Max Samukha from comment #8) > > > No problem there. But it's the size the compiler uses (for the init symbol, > > > the TypeInfo.initializer() etc.), not some library bug, and it's working. > > > > Bugs will most likely be revealed in those when/if the align attribute is > > fixed and people start using it. > > Nope - again, class instances are never stored consecutively. Only wrapper > structs can be, and as long as they align the backing buffer as required, > all is/will be working fine. By that argument, a struct object is likewise an ephemeral entity with a backing buffer of bytes, and therefore 'sizeof' of a struct should not include the padding. We'd better stop at that to avoid a philosophical debate. A template returning the desired result will be easy to define after the bugs are fixed.

@kinke updated dlang/dmd pull request #14063 "Add __traits(classInstanceAlignment)" mentioning this issue: - Add __traits(classInstanceAlignment) Tackling the compiler part of issue 16508. https://github.com/dlang/dmd/pull/14063

dlang/dmd pull request #14063 "Add __traits(classInstanceAlignment)" was merged into master: - 8e61fbfb89065a08e339cfb76bdaca10467236ed by Martin Kinkelin: Add __traits(classInstanceAlignment) Tackling the compiler part of issue 16508. https://github.com/dlang/dmd/pull/14063

@kinke updated dlang/phobos pull request #8459 "Use new __traits(classInstanceAlignment)" mentioning this issue: - Use new __traits(classInstanceAlignment) Tackling the Phobos part of issue 16508. https://github.com/dlang/phobos/pull/8459

@kinke updated dlang/druntime pull request #3820 "Use new __traits(classInstanceAlignment)" mentioning this issue: - Use new __traits(classInstanceAlignment) Tackling the druntime part of issue 16508. https://github.com/dlang/druntime/pull/3820

dlang/phobos pull request #8459 "Use new __traits(classInstanceAlignment)" was merged into master: - 781a1a403778cc693aa478a1c1df9c2a0106034c by Martin Kinkelin: Use new __traits(classInstanceAlignment) Tackling the Phobos part of issue 16508. https://github.com/dlang/phobos/pull/8459

dlang/druntime pull request #3820 "Use new __traits(classInstanceAlignment)" was merged into master: - 34676459ab0a58fc5fb533b318e272ee0ae1dd60 by Martin Kinkelin: Use new __traits(classInstanceAlignment) Tackling the druntime part of issue 16508. https://github.com/dlang/druntime/pull/3820

@tchaloupka updated dlang/phobos pull request #8467 "Fix 23132 - Avoid ranges copy when compared for equality" mentioning this issue: - Use new __traits(classInstanceAlignment) Tackling the Phobos part of issue 16508. https://github.com/dlang/phobos/pull/8467

@pbackus updated dlang/phobos pull request #8507 "Fix 23245, 23246 - std.format regressions" mentioning this issue: - Use new __traits(classInstanceAlignment) Tackling the Phobos part of issue 16508. https://github.com/dlang/phobos/pull/8507

@RubyTheRoobster updated dlang/phobos pull request #8525 "fix issue 23140" mentioning this issue: - Use new __traits(classInstanceAlignment) Tackling the Phobos part of issue 16508. https://github.com/dlang/phobos/pull/8525

THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/dmd/issues/17157 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB