www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 7396] New: Indicate default alignment with 0.

reply d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396

           Summary: Indicate default alignment with 0.
           Product: D
           Version: D2
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: DMD
        AssignedTo: nobody puremagic.com
        ReportedBy: venix1 gmail.com



Created an attachment (id=1071)
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 {}.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 29 2012
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396


Walter Bright <bugzilla digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugzilla digitalmars.com



12:01:39 PST ---
I'm really not understanding this.

"align" by itself means default alignment. How that would differ from align(0)
escapes me.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396






 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

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396




16:40:01 PST ---
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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396


Iain Buclaw <ibuclaw ubuntu.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ibuclaw ubuntu.com



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).

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396




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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396




22:20:51 PST ---
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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396






 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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396






 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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 30 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396




03:19:50 PST ---

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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 30 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396




---


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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 30 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396






 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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 30 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396




11:21:01 PST ---


 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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 30 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396








 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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 30 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396




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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 30 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396




12:20:04 PST ---
Right, it's a compiler issue. Not a language issue, and no new language
features or syntax are required.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 30 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396






 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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 30 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396


dawg dawgfoto.de changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dawg dawgfoto.de



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).

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 30 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396




---


 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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 31 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396




01:17:43 PST ---
I do too.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 31 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396




---
I can't seem to get git working at work. :)





 
 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);

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jan 31 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396





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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Feb 01 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396




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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jun 28 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396


Walter Bright <bugzilla digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED



21:43:46 PDT ---
Addressed using a #define rather than a magic value, and a typedef.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jun 28 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396




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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jun 28 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396




---
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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jun 29 2012
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=7396




15:18:16 PDT ---
Currently, it does not. I regard that as a separate issue, however.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jun 29 2012