This is an archive of the discontinued LLVM Phabricator instance.

Fix alignment issues in LLVM.
ClosedPublic

Authored by jyknight on Jun 4 2015, 10:57 PM.

Details

Summary

Adds static_asserts to ensure alignment of concatenated objects is
correct, and fixes them where they are not.

Also changes the definition of AlignOf to use constexpr, except on
MSVC, to avoid enum comparison warnings from GCC.

(There's not too much of this in llvm itself, most of the fun is in
clang).

This seems to make LLVM actually work without Bus Error on 32bit
sparc.

Diff Detail

Event Timeline

jyknight updated this revision to Diff 27178.Jun 4 2015, 10:57 PM
jyknight retitled this revision from to Fix alignment issues in LLVM..
jyknight updated this object.
jyknight edited the test plan for this revision. (Show Details)
jyknight added a subscriber: Unknown Object (MLST).
majnemer added inline comments.
include/llvm/Support/AlignOf.h
56–57

Why not just do enum : unsigned { Alignment = ... ; ?

jyknight added inline comments.Jun 5 2015, 6:52 AM
include/llvm/Support/AlignOf.h
56–57

I hadn't tried until you suggested, but the answer is: because GCC still warns about it.

Ping? If there's no objection, I'd like to submit this.

rnk added a reviewer: rnk.Jun 16 2015, 9:32 AM
rnk edited edge metadata.
rnk added a subscriber: dexonsmith.

BTW, Duncan recently rewrote this so I'll add him.

include/llvm/Support/AlignOf.h
56–57

A static constexpr int still sometimes requires an out of class definition if you accidentally take the address of the global by passing it to a reference-taking function like std::max, which is a pretty common operation for alignments.

I'd rather add casts in the assertions or disable gcc's warning. Seem reasonable?

lib/IR/Metadata.cpp
393–396

I feel like a more intuitive way to phrase this computation is to overallocate like so:

size_t OpSize = NumOps * sizeof(MDOperand);
OpSize = RoundUpToAlignment(OpSize, llvm::alignOf<uint64_t>());

The placement new operations can use the same negative indexing approach as operator delete, and we never have to consider AlignOffset as a separate quantity.

411–412

What was wrong with the old code here? A strict aliasing warning?

jyknight added inline comments.Jun 16 2015, 10:54 AM
include/llvm/Support/AlignOf.h
56–57

Adding casts everywhere would be pretty annoying. Adding -Wno-enum-compare to the compile flags seems okay, but somewhat unfortunate, as it can usually expose real bugs.

How about just adding a couple lines to the header:

template <typename T>
constexpr unsigned AlignOf<T>::Alignment;

?

lib/IR/Metadata.cpp
393–396

SGTM, will change it.

411–412

It's fine back the way it was before; I think I must've added some arithmetic in there in a prior revision.

jyknight updated this revision to Diff 27790.Jun 16 2015, 3:48 PM
jyknight edited edge metadata.

Updated per review comments from Duncan and Reid.

rnk accepted this revision.Jun 16 2015, 5:48 PM
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.Jun 16 2015, 5:48 PM
This revision was automatically updated to reflect the committed changes.