7396 – Indicate default alignment with 0.

Created attachment 1071 [details] 
align.patch - Patch file
The patch modifies DMD to use the value 0 when default alignment should be
used. It allows implementations to easily deviate from how DMD handles field
alignment and keep the requirement that `align` will restore default alignment.
Below is an attempt to describe why this is useful.
---
DMD and GDC different in how they interpret the align keyword. GDC will align
all fields to the specified size. DMD appears to still take into consideration
the required alignment for a given type.
The issue arises when one wishes to return to default alignment.
"align by itself sets it to the default, which matches the default member
alignment of the companion C compiler."
GDC uses the value passed to AlignDeclaration to force the alignment of field
members. The front end currently treats default alignment as system alignment
generally 8. This will indicate to GDC all field members should be aligned on
8 bytes instead of restoring default alignment.
The following test shows the unlikely situation when this becomes an issue. 
This situation in unlikely until conditional complication comes into play since
a trivial workaround is to use braces.
// For GDC.
struct A 
{
 align(2):
 byte a; // 2 bytes
 byte b; // 2 bytes
 byte c; // 2 bytes
 align: // restore default alignment. Translates to align(8)
 byte d; // 8 bytes. 1 is expected
 byte e; // 8 bytes. 1 is expected
 byte f; // 8 bytes. 1 is expected
}
align struct A {} is remade into align(8) struct A {}.

I'm really not understanding this.
"align" by itself means default alignment. How that would differ from align(0) escapes me.

(In reply to comment #1)
> I'm really not understanding this.
> 
> "align" by itself means default alignment. How that would differ from align(0)
> escapes me.
The problem is how the front end treats "align". 
---
struct B
{
 align:
 char a;
}
$ dmd -o- align.d -H 
// D import file generated from 'align.d'
// Notice align became align (8)
struct B
{
 align (8) char a;
}
---
That treatment of "align" conflicts with what's in the specification.
"align by itself sets it to the default, which matches the default member alignment of the companion C compiler."
"Integer specifies the alignment which matches the behavior of the companion C compiler when non-default alignments are used."
The front end has no way to indicate default alignment is what the user wants and is effectively rewriting the default alignment expression to be an Integer alignment expression.
By setting "n = 0" instead of "n = global.structalign" if clearly indicates to the compiler default alignment is desired. This is beneficial for when the default compiler is not DMD. 
A description of GCC's aligned attribute which GDC attempted to mimic.
http://gcc.gnu.org/onlinedocs/gcc/Variable-Attributes.html#Variable-Attributes 

The default alignment of 8 does not align byte values to 8 byte boundaries, as your message suggests. I think this is where the issue is - I think it is just a misunderstanding.
The default alignment should match what the C compiler does as the default. I believe this does behave this way currently - dmd for Windows matches the dmc compiler, and dmd for Linux matchs gcc.
If this is not what gdc is doing, then there's an issue with gdc.
In any case,
 align:
is *required* to match what the C compiler does, whatever that may be.

The idea was that, ie:
struct foo
{
 align(8) int bar;
}
be equivalent to:
struct foo
{
 int bar __attribute__((aligned(8)));
};
Currently this is not the case in DMDFE, as the user defined alignment is pretty much ignored (unless the value 4 or less?).
Slightly off note, but there is also no error if the aligned value given is not a power of 2, eg: align(3).

Going with what Iain added. The reason for this is that the front end treats default alignment and align(8) as equal. This leaves no way for GDC to know that default alignment is being requested.
That brings me to my original suggestion that 0 should be used to indicate default alignment. This gives a very clear indication that default alignment is what is desired.

I would suggest the problem is with the way gdc is doing alignment.
 align:
means the default alignment that matches the C compiler. dmd and gdc need to do whatever it takes to make that happen. Adding another align directive just confuses things.

(In reply to comment #6)
> I would suggest the problem is with the way gdc is doing alignment.
gdc is attempting to treat field alignment in the same manner as gcc's aligned attribute does. From the wording of the specification this seems reasonable.
> align:
> 
> means the default alignment that matches the C compiler. 
`align:` is translated into `align(8)` by the parser @ https://github.com/D-Programming-Language/dmd/blob/master/src/parse.c#L503 
> dmd and gdc need to do whatever it takes to make that happen. 
After the align attribute is parsed, neither dmd nor gdc can determine default alignment is wanted.
> Adding another align directive just confuses things.
I'm not proposing another align directive. I'm suggesting a modification to the way `align:` is handled by the parser so that dmd or gdc can determine if default alignment is needed.

(In reply to comment #6)
> I would suggest the problem is with the way gdc is doing alignment.
> 
> align:
> 
> means the default alignment that matches the C compiler. dmd and gdc need to do
> whatever it takes to make that happen. Adding another align directive just
> confuses things.
From the spec:
---
Align Attribute specifies the alignment of struct members. align by itself sets it to the default, which matches the default member alignment of the companion C compiler.
---
GDC matches the companion GCC compiler, in that we have a callback to get the field alignment for the type, which may not necessarily the same as the type alignment, as some architectures (i.e. i386) limit struct field alignment to a lower boundary than alignment of some types of variables.
From the spec:
---
Integer specifies the alignment which matches the behavior of the companion C compiler when non-default alignments are used. 
---
GDC matches the companion GCC compiler here as well, in that:
struct S {
 align(4) byte a; // placed at offset 0
 align(4) byte b; // placed at offset 4
}
This is achieved by adding a declalign field in VarDeclaration that takes precedence over the type align.
This I think is different from how DMC++ treats the align attribute, which is where the conflict of interest arises.

>This I think is different from how DMC++ treats the align attribute, which is
where the conflict of interest arises. 
Which means that dmd should change to match gcc for gcc platforms. The addition of an align(0) is not the right solution.

(In reply to comment #9)
> >This I think is different from how DMC++ treats the align attribute, which is
> where the conflict of interest arises. 
> 
> Which means that dmd should change to match gcc for gcc platforms. The addition
> of an align(0) is not the right solution.
It's not an addition of align(0) - although currently I think the parser will accept any non-negative integer, which needs to be addressed. The point Daniel raised in question is that it would be helpful if there was information available so we could tell the difference between whether the user specified align: or align(n), so we can do the right thing accordingly.

(In reply to comment #9)
> The addition of an align(0) is not the right solution.
Currently,
 `align:` becomes`align(8)`. Ambiguous. default alignment or 8 byte? 
Using 0,
 `align:` becomes `align(0)`. Now if 0, default alignment is requested.
I'm not suggesting adding align(0). I'm suggesting setting the internal variable to 0 when default alignment is wanted. The reason for this is the knowledge that default alignment is required is only available to the parser and not saved.
The biggest issue with this method is that if new code is added which uses the structalign field of a scope block. It must also check for 0. This could be alleviated by removing direct access to structalign and adding 2 member functions.
Scope::alignsize() - return (structalign ? structalign : global.structalign);
Scope::isDefaultAlignment() - return !structalign
---
The original patch is designed to allow GDC to do what it's been doing without breaking DMD or diverging the source more than necessary. Since you have suggested DMD should change to match gcc on gcc platforms. It should be considered invalid as any solution for DMD will solve the problem for GDC as well.

(In reply to comment #11)
> Currently,
> `align:` becomes`align(8)`. Ambiguous. default alignment or 8 byte? 
This is the misunderstanding.
 align:
means align to the C compiler default. IT DOES NOT MEAN align(8). How it is implemented under the hood is IRRELEVANT.

(In reply to comment #12)
> (In reply to comment #11)
> > Currently,
> > `align:` becomes`align(8)`. Ambiguous. default alignment or 8 byte? 
> 
> This is the misunderstanding.
> 
> align:
> 
> means align to the C compiler default. IT DOES NOT MEAN align(8). How it is
> implemented under the hood is IRRELEVANT.
It is RELEVANT because that information is THROWN AWAY during parsing. ONLY the PARSER knows that C compiler default alignment is required and DOES NOT convey that information in any form.

The statement:
 `align:` becomes `align(8)`
in based entirely on how the parser handles the align attribute.
// excerpt from parse.c line 503.
case TOKalign:
{ unsigned n;
 s = NULL;
 nextToken();
 if (token.value == TOKlparen)
 {
 nextToken();
 if (token.value == TOKint32v && token.uns64value > 0)
 n = (unsigned)token.uns64value;
 else
 { error("positive integer expected, not %s", token.toChars());
 n = 1;
 }
 nextToken();
 check(TOKrparen);
 }
 else
 n = global.structalign; // default
 a = parseBlock();
 s = new AlignDeclaration(n, a);
 break;
}
When `align:` is encountered n is set to global.structalign. This makes the statement equal to `align(8)`.
It's not that I don't understand that `align:` means default alignment and should be treated differently than `align(8)`. It's that only the parser knows that and currently doesn't convey that information.

Right, it's a compiler issue. Not a language issue, and no new language features or syntax are required.

(In reply to comment #15)
> Right, it's a compiler issue. Not a language issue, and no new language
> features or syntax are required.
Yes, I agree. My proposal was the following.
// excerpt from parse.c line 503.
case TOKalign:
{ unsigned n;
 s = NULL;
 nextToken();
 if (token.value == TOKlparen)
 {
 nextToken();
 if (token.value == TOKint32v && token.uns64value > 0)
 n = (unsigned)token.uns64value;
 else
 { error("positive integer expected, not %s", token.toChars());
 n = 1;
 }
 nextToken();
 check(TOKrparen);
 }
 else
- n = global.structalign; // default
+ n = 0; // default
 a = parseBlock();
 s = new AlignDeclaration(n, a);
 break;
}
Now the compiler can test for 0 and know that default alignment is required. This removes the ambiguity with the current implementation.

https://github.com/D-Programming-Language/dmd/pull/657
The pull request additionally solves the broken header generation
for default align. Also note that adding this won't change the meaning of
align(0), as 0 is already an error.
PS:
We should definitely check at some point that alignment is a power of two.
There is already code relying on this (AggregateDeclaration::alignmember).

(In reply to comment #17)
> PS:
> We should definitely check at some point that alignment is a power of two.
> There is already code relying on this (AggregateDeclaration::alignmember).
I think it would also make sense to disallow any align(n) value greater than align(16) for 32bit, and possibly align(32) for 64bit platforms.

I do too.

I can't seem to get git working at work. :)
(In reply to comment #16)
> 
> Yes, I agree. My proposal was the following.
> 
> // excerpt from parse.c line 503.
> case TOKalign:
> { unsigned n;
> 
> s = NULL;
> nextToken();
> if (token.value == TOKlparen)
> {
> nextToken();
> if (token.value == TOKint32v && token.uns64value > 0)
> n = (unsigned)token.uns64value;
> else
> { error("positive integer expected, not %s", token.toChars());
> n = 1;
> }
> nextToken();
> check(TOKrparen);
> }
> else
> - n = global.structalign; // default
> + n = 0; // default
> 
> a = parseBlock();
> s = new AlignDeclaration(n, a);
> break;
> }
> 
> Now the compiler can test for 0 and know that default alignment is required. 
> This removes the ambiguity with the current implementation.
Daniel, to add to that:
 if (token.value == TOKlparen)
 {
 nextToken();
 if (token.value == TOKint32v && token.uns64value > 0)
+ {
+ if (token.value & (token.value - 1))
+ error("align must be a power of 2, not %s", token.toChars());
 n = (unsigned)token.uns64value;
+ }
 else
 { error("positive integer expected, not %s", token.toChars());
 n = 1;
 }
 nextToken();
 check(TOKrparen);
 }
And in attrib.h:
 struct AlignDeclaration : AttribDeclaration
 {
- unsigned salign;
+ unsigned salign; // alignment in effect, must be power of 2.
+ // 0 if using default align for target.
 
 AlignDeclaration(unsigned sa, Dsymbols *decl);

>I think it would also make sense to disallow any align(n) value greater than
>align(16) for 32bit, and possibly align(32) for 64bit platforms.
Don't do that. GCC can provide arbitrary alignment even for stack values.
Also your number is already too low, xsave needs 64-byte alignment.

Commit pushed to master at https://github.com/D-Programming-Language/dmd
https://github.com/D-Programming-Language/dmd/commit/4a5a8352e91dd361a96644fb3aaa1aece0c9d0d8
fix Issue 7396 - Indicate default alignment with 0.

Addressed using a #define rather than a magic value, and a typedef.

Commit pushed to dmd-1.x at https://github.com/D-Programming-Language/dmd
https://github.com/D-Programming-Language/dmd/commit/81fc676f9ae108fd673a77019d29b4aaa91aa8e6
fix Issue 7396 - Indicate default alignment with 0.

Thanks, I'll be merging this in tonight. Does the frontend error if the alignment given is not a power of 2?
ie: using align(3) should not ICE or compile.

Currently, it does not. I regard that as a separate issue, however.