llvm / llvm-project

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

GNU extension `alignof(expression)` incorrectly returns preferred alignment #47017

Open jyknight opened 3 years ago

jyknight commented 3 years ago
Bugzilla Link 47673
Version trunk
OS All
CC @DougGregor,@efriedma-quic,@zygoloid

Extended Description

After adjusting for explicit alignment attributes on a decl, alignof(name) falls back to the preferred alignment of the decl's type. This is broken if the decl isn't actually allocated at preferred alignment.

An object will be aligned to the preferred alignment when creating complete objects on the stack or as a global. But, Decls aren't always referring to a newly-created complete object, and in some cases, like function parameters, the alignment can be ABI-mandated to be less than the preferred alignment.

To solve this, we could try to be "smart" and return the preferred alignment when we know this to be correct. But, I think it's probably better to simply make alignof(expr) be equivalent to alignof(decltype(var)) adjusted by the explicit alignment attributes on the decl.

Some examples of the problems:

clang -fsyntax-only -target powerpc-aix -xc++ - <<EOF
struct A { double x; };
static_assert(alignof(A) == 4, ""); // ABI alignment
static_assert(__alignof__(A) == 8, ""); // Preferred alignment

void f(A* a) {static_assert(alignof(*a) == 4, ""); } // correct
void f(A& a) {static_assert(alignof(a) == 8, ""); } // incorrect
void f(int x, A a) { static_assert(alignof(a) == 8, ""); } // incorrect
A f() {
    A a{1};
    static_assert(alignof(a) == 8, ""); // incorrect
    return a;
}
EOF

Some of the problems cannot happen anywhere except AIX, because that's the only target where aggregates have ABI alignment != preferred alignment. But elsewhere, references are still an issue:

clang -fsyntax-only -target i386-linux-gnu -xc++ - <<EOF
static_assert(alignof(double) == 4, ""); // ABI alignment
static_assert(__alignof__(double) == 8, ""); // Preferred alignment
void f(double* a) {static_assert(alignof(*a) == 4, ""); } // correct
void f(double& a) {static_assert(alignof(a) == 8, ""); } // incorrect
EOF

FWIW, it looks like GCC has a bug in alignof(expr) as well -- it seems to always return the preferred alignment, no matter what.

gcc -o /dev/null -c -m32 -xc++ - <<EOF
static_assert(alignof(double) == 4, ""); // ABI alignment
static_assert(__alignof__(double) == 8, ""); // Preferred alignment
void f(double* a) {static_assert(alignof(*a) == 8, ""); } // incorrect
void f(double& a) {static_assert(alignof(a) == 8, ""); } // incorrect
EOF
jyknight commented 3 years ago

Note: the GCC bug referred to below has been fixed by https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=61827d5d9a5a09a8c05d5e41f95b03ebc6c43f61

llvmbot commented 2 years ago

@llvm/issue-subscribers-c-1

hubert-reinterpretcast commented 2 years ago

@jyknight, just to clarify "adjusted by the explicit alignment attributes on the decl" includes, for a member of a struct, [[gnu::packed]] on the struct?

struct [[gnu::packed]] S {
  int x;
};
struct S s;

extern char x[_Alignof(s.x)];
extern char x[1];

https://godbolt.org/z/xfa4hG1Kd

jyknight commented 2 years ago

Yes, just like it does today.

I only meant to indicate that the output of alignof should be based unconditionally on ABI alignment plus modifications, but not try to use the preferred alignment (even if that may be applicable in some cases).

hubert-reinterpretcast commented 2 years ago

but not try to use the preferred alignment (even if that may be applicable in some cases

That would certainly be easier to make consistent this way, but I think this needs coordination with GCC (and maybe others). Is this being tracked on that end as well?

jyknight commented 2 years ago

See above. GCC was already fixed to work that way.

hubert-reinterpretcast commented 2 years ago

I read what I believe is the corresponding GCC defect report (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88115), and my impression was that it is about how they considered __alignof__ and alignof to be functionally equivalent when comparing template-dependent expressions in the signatures of template declarations.

Perhaps the case you mention (for lvalues not known to name a complete object) was also fixed; however, naming a global variable still produces the preferred alignment:

double d;
extern char x[_Alignof(double)];
extern char x[_Alignof(d)];

fails to compile with x86 gcc -fsyntax-only -m32 -xc -.

https://godbolt.org/z/qaa7bGWce

jyknight commented 2 years ago

Hm, I thought I'd looked at that back when I commented 2 years ago, but you're right. GCC is returning preferred alignment in the cases where it is actually guaranteed to have allocated the object at that greater alignment. (Which is to say: only for globals and function locals, and not references or function arguments.)

GCC's (new) behavior is entirely valid/correct, and if we want Clang to do the same, that's good by me. (There are complexities to watch out for, of course, e.g. we need to make sure NOT use the larger alignment for NRVO "locals", since they're actually disguised references to some other object.)

hubert-reinterpretcast commented 2 years ago

we need to make sure NOT use the larger alignment for NRVO "locals", since they're actually disguised references to some other object

To be consistent, it would need to be any type that the C++ ABI allows NRVO for (from the current function) instead of actually figuring out if NRVO actually would occur.

Trying to answer if NRVO would actually occur can run into contradictions:

struct A {
  A(double x) : x(x) {}
  A(const A &);
  ~A();
  double x;
};

void g(void *) noexcept;
A f(void **p) {
  A a(-1u);
  g(1 + &a);

  // `a` may be aligned to 4 if NRVO on AIX, thus:
  if constexpr(alignof(a) == 8) { // if not NRVO (given above context)
    return a; // make it NRVO
  } else {
    return A(0.);
  }
}
hubert-reinterpretcast commented 2 years ago

Aside: GCC only fixed C++ alignof and did not do the same for C _Alignof: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105588

Endilll commented 2 months ago

CC @AaronBallman