[PATCH] D10272: Fix alignment issues in Clang.

[James Y Knight]

So, a couple things:

Firstly:

I think it's pretty sad that this idiom of manually dealing with memory layout is used all over the place, and has basically no abstraction. In addition to having to coordinate carefully between allocation and access, as it stands, it's almost impossible to tell from the header what the layout of the object actually is -- for example, in Decl.h, there's no hint that ImportDecl has a SourceLocation array appended to it. This is somewhat unfortunate.

I understand that it's done this way for speed and memory efficiency, but I can't help but think there *must* be a better way to accomplish it than to splatter manual object size arithmetic and reinterpret casts so widely over the codebase.

It'd be great if the GCC zero-length-array extension could be used, or if C++ had adopted C11's flexible array members. That would help in the perhaps 75% of cases where it's just a single optional trailing object, or array of trailing objects of the same type.

That makes the allocation size arithmetic easier -- `offsetof(Type, arrayfield[num_elements])`. It makes the access easier (just reference the field). And it makes the alignment simply handled by the compiler. I believe GCC's extension is supported by GCC, Clang, and VC++. I guess there's a desire to not use extensions like that even if they are supported by the compilers we care about?

And besides being not standard C++, that also wouldn't work for the ~25% of cases which have multiple kinds of appended objects, that come one after another. As an alternative to the GCC zero-length-array option, I had thought of perhaps making a template base class that handles these sorts of layouts, somehow, but hadn't really thought it through terribly far how it might work. (Or if it'd be too complicated to even be worth it.)

Secondly:

There are more cases where the suggestion doesn't work than just forward declarations. "this + 1" or "X + 1" is only used some of the time, there's many other ways that code accesses appended objects.

E.g. DeclTemplate.h DependentFunctionTemplateSpecializationInfo has:

  FunctionTemplateDecl * const *getTemplates() const {
    return reinterpret_cast<FunctionTemplateDecl*const*>(&getTemplateArgs()[NumArgs]);
  }

Or then there's DeclRefExpr

  return reinterpret_cast<ASTTemplateKWAndArgsInfo *>(
      llvm::alignAddr(&getInternalFoundDecl() + 1,
                      llvm::alignOf<ASTTemplateKWAndArgsInfo>()));

which re-aligns, and thus doesn't require that getInternalFoundDecl() is aligned properly for the ASTTemplateKWAndArgsInfo object -- but the base DeclRefExpr itself really ought to be aligned enough for ASTTemplateKWAndArgsInfo, or else the layout of the object would change depending on its address!

Annnnnnnnnnnnnnnnnyways:

I had originally started to put these asserts in the constructors, but that seemed far from the accesses and was hidden off in a random cpp file, and sometimes there are multiple of them, spread around. So I tried the accesses, but sometimes those were spread around multiple places too. So eventually I just settled on the class definition, because it's always right there and there's one of it. :)

I can make the requested change, in the subset of cases where it works, and will certainly do so if still requested. But I'm not really sure it's worth it, because it removes what is often the only documentation of Weirdness going on, at the class definition site.

And, I think the Right Thing is to address the deeper problem of what really seems to me like borderline unreadable code -- somehow. If that can be done, it will naturally allow alignment asserts to be removed at the same time as the casts and sizeof arithmetic, because it'd be unnecessary if the compiler or some common library code was ensuring that allocation and sizing and offsets and alignment are all done consistently and correctly.


http://reviews.llvm.org/D10272