Jump to page: 1 2
Thread overview
[Issue 16508] Alignment of class members is not respected. Affects new, scoped and classInstanceAlignment.
Nov 23, 2018
Stanislav Blinov
Dec 17, 2019
Max Samukha
Aug 07, 2021
kinke
Aug 08, 2021
Max Samukha
Aug 08, 2021
kinke
Aug 08, 2021
Max Samukha
Aug 08, 2021
kinke
Aug 09, 2021
Max Samukha
Aug 09, 2021
kinke
Aug 09, 2021
Max Samukha
May 02
Dlang Bot
May 13
Dlang Bot
May 13
Dlang Bot
May 13
Dlang Bot
May 13
Dlang Bot
May 14
Dlang Bot
May 23
Dlang Bot
Jul 13
Dlang Bot
Aug 03
Dlang Bot
November 23, 2018
https://issues.dlang.org/show_bug.cgi?id=16508

Stanislav Blinov <stanislav.blinov@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |stanislav.blinov@gmail.com

--- Comment #1 from Stanislav Blinov <stanislav.blinov@gmail.com> ---
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.

--
December 17, 2019
https://issues.dlang.org/show_bug.cgi?id=16508

Max Samukha <maxsamukha@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |maxsamukha@gmail.com

--- Comment #2 from Max Samukha <maxsamukha@gmail.com> ---
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.

--
August 07, 2021
https://issues.dlang.org/show_bug.cgi?id=16508

kinke <kinke@gmx.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kinke@gmx.net

--- Comment #3 from kinke <kinke@gmx.net> ---
> __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.

--
August 08, 2021
https://issues.dlang.org/show_bug.cgi?id=16508

--- Comment #4 from Max Samukha <maxsamukha@gmail.com> ---
(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.

--
August 08, 2021
https://issues.dlang.org/show_bug.cgi?id=16508

--- Comment #5 from kinke <kinke@gmx.net> ---
(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.

--
August 08, 2021
https://issues.dlang.org/show_bug.cgi?id=16508

--- Comment #6 from Max Samukha <maxsamukha@gmail.com> ---
(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.

--
August 08, 2021
https://issues.dlang.org/show_bug.cgi?id=16508

--- Comment #7 from kinke <kinke@gmx.net> ---
(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.

--
August 09, 2021
https://issues.dlang.org/show_bug.cgi?id=16508

--- Comment #8 from Max Samukha <maxsamukha@gmail.com> ---
(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.

--
August 09, 2021
https://issues.dlang.org/show_bug.cgi?id=16508

--- Comment #9 from kinke <kinke@gmx.net> ---
(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.

--
August 09, 2021
https://issues.dlang.org/show_bug.cgi?id=16508

--- Comment #10 from Max Samukha <maxsamukha@gmail.com> ---
(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.

--
« First   ‹ Prev
1 2