llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.69k stars 11.87k forks source link

alignment specifier ignored on enum definitions #22653

Open ec04fc15-fa35-46f2-80e1-5d271f2ef708 opened 9 years ago

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 9 years ago
Bugzilla Link 22279
Version trunk
OS Linux
CC @majnemer,@DougGregor,@gregbedwell,@wjristow

Extended Description

Consider:

  enum alignas(64) X : int {} x;
  static_assert(alignof(X) == 64, "");

This fails: the assert fires, and if you remove it, we generate code that has an underaligned global x.

Same thing happens for __attribute__((aligned(N))) (but in that case, we're at least being GCC-compatible).

Conversely, consider:

  typedef __attribute__((aligned(64))) N;
  enum X : N {} x;

Here, we do apply the alignment to X. It's not obvious whether we should. GCC does. EDG does not.

wjristow commented 9 years ago

Great. Thanks for explanation.

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 9 years ago

My question is what about 'e1b' with new Clang? Is an alignment of 4 what we expect/want?

Yes. See the "Type attributes" section in https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html

For enum1, the attribute appertains to the variable e1a, whereas for enum2, the attribute appertains to the enum type itself.

For additional confirmation, consider:

  struct struct1 { int n; } s1a __attribute__((aligned(16)));
  struct1 s1b;
  struct struct2 { int n; }     __attribute__((aligned(16)));
  struct2 s2;
``
Here, we have:

variable alignment s1a 16 s1b 4 s2 16


with all of {GCC, old Clang, new Clang}.
wjristow commented 9 years ago

Just to add to the confusion, I see I have some typos in comment 1. The correct summary of alignments for that example is:

  "old" Clang (and gcc):
  variable    alignment
    e1a           16
    e1b           4
    e2            4

  "new" Clang:
  variable    alignment
    e1a           16
    e1b           4
    e2            16

The old behaviour is just a bug (in both GCC and Clang) in my opinion.

I can see an argument for it being a GCC bug. And in fact, the GCC docs suggest that e2 ought to have alignment of 16 (although I think somewhat ambiguously, hence the "suggest").

We should probably issue a -Wgcc-compat warning if attribute((aligned)) is applied to an enum, so that people attempting to write portable code are at least informed that their attribute will be silently ignored by GCC.

That sounds good.


One question remains for me. Expanding the example of comment 1 to use the alignas(N) approach:

  enum enum1 { red, green, blue } e1a __attribute__((aligned(16)));
  enum1 e1b;
  enum enum2 { val1, val2, val3 }     __attribute__((aligned(16)));
  enum2 e2;

  enum alignas(16) enum3 { v1, v2, v3} e3a;
  enum3 e3b;

  enum alignas(16) enum4 { V1, V2, V3};
  enum4 e4;

With this, we have:

  "old" Clang (and gcc):
  variable    alignment
    e1a           16
    e1b           4
    e2            4
    e3a           4
    e3b           4
    e4            4

  "new" Clang:
  variable    alignment
    e1a           16
    e1b           4
    e2            16
    e3a           16
    e3b           16
    e4            16

The fact that e3a has alignment 4 in GCC (and old Clang) looks clearly to be a bug, irrespective of any ambiguities in GCC docs. And I think that supports your view of the old behavior of my first test-case being a bug in GCC.

My question is what about 'e1b' with new Clang? Is an alignment of 4 what we expect/want? It's compatible with GCC, but is that another aspect of this GCC bug?

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 9 years ago

being GCC-compatible is generally desired for GCC extensions. So it seems the old clang behavior is desired in this case.

The old behaviour is just a bug (in both GCC and Clang) in my opinion. (Alternatively, you can view this as a Clang extension of the GCC behavior.)

We should probably issue a -Wgcc-compat warning if attribute((aligned)) is applied to an enum, so that people attempting to write portable code are at least informed that their attribute will be silently ignored by GCC.

GCC bug filed as gcc.gnu.org/#64987 .

wjristow commented 9 years ago

A change to address this PR was made recently (2015-01-21), at r226653. That modification is causing a GCC-incompatibility, and also an object layout incompatibility pre- and post- r226653 with clang.

For example, given the following enums:

  enum enum1 { red, green, blue } e1a __attribute__((aligned(16)));
  enum1 e1b;
  enum enum2 { val1, val2, val3 }     __attribute__((aligned(16)));
  enum2 e2;

Prior to r226653 (say "old" clang), testing with -target x86_64-pc-linux-gnu, the alignment of e1a was 16, but that didn't make the underlying type enum1 have an alignment of 16. enum1 had an alignment of 4, and therefore e1b also had an alignment of 4. Somewhat consistent with that, e2 also had alignment of 4, and so enum2 had alignment of 4. That is, e1a had an alignment of 16, but every other variable, and the underlying enum types, only had alignments of 4.

In summary, with old clang:

  variable    alignment
    e1a           64
    e1b           4
    e2            4

This is the same behavior as GCC (tested with GCC 4.8.2 and 4.9.2)

But beginning with r226653 (say "new" clang), the behavior became:

  variable    alignment
    e1a           64
    e1b           4
    e2            16

So with new clang, the underlying enum type enum1 continues to only have alignment of 4, but for enum2 (the situation when no variable is declared), the underlying type now gets the alignment from the attribute (i.e., it's 16).

Given that this attribute is a GCC-extension, there isn't a precise specification like there is in the C or C++ Standard, so the desired behavior isn't crystal clear. But by the same token, being GCC-compatible is generally desired for GCC extensions. So it seems the old clang behavior is desired in this case.

If these enumerations are members of structs, then the size of the struct changes from old clang to new clang, causing object-file incompatibilities due to the change in layout. For example:

  // size and alignment are 4 with both old and new clang
  struct thing1 {
    enum1 color;
  };

  // size and alignment are 4 with old and, both are 16 with new clang
  struct thing2 {
    enum2 value;
  };
Endilll commented 3 months ago

As far as the C++ Standard is concerned, this issue is moot, because alignas was disallowed on enums by CWG2354, and we support that since Clang 15.

Of course __attribute__((aligned(64))) part of this issue is not affected by the Standard.

llvmbot commented 3 months ago

@llvm/issue-subscribers-clang-frontend

Author: None (ec04fc15-fa35-46f2-80e1-5d271f2ef708)

| | | | --- | --- | | Bugzilla Link | [22279](https://llvm.org/bz22279) | | Version | trunk | | OS | Linux | | CC | @majnemer,@DougGregor,@gregbedwell,@wjristow | ## Extended Description Consider: ```cpp enum alignas(64) X : int {} x; static_assert(alignof(X) == 64, ""); ``` This fails: the assert fires, and if you remove it, we generate code that has an underaligned global `x`. Same thing happens for `__attribute__((aligned(N)))` (but in that case, we're at least being GCC-compatible). Conversely, consider: ```cpp typedef __attribute__((aligned(64))) N; enum X : N {} x; ``` Here, we *do* apply the alignment to `X`. It's not obvious whether we should. GCC does. EDG does not.