Page MenuHomePhabricator

[ADT] Change the `IntervalMap` alignment assert for x86 MSVC
ClosedPublic

Authored by aleksandr.urakov on Sep 27 2018, 7:46 AM.

Details

Summary

This patch adds the different alignment assert for the x86 MSVC case. It is because x86 MSVC doesn't apply automatically (without __declspec(align(...))) alignments more than 4 bytes, even if alignof has returned so. Consider the example:

https://godbolt.org/z/zIPa_G

Here alignof for both S0 and S1 returns 8, but only S1 is really aligned on x86. The explanation of this behavior is here:

https://docs.microsoft.com/en-us/cpp/build/conflicts-with-the-x86-compiler

Diff Detail

Repository
rL LLVM

Event Timeline

Eugene.Zelenko edited reviewers, added: hans; removed: Eugene.Zelenko.Sep 27 2018, 12:46 PM
hans added a reviewer: rnk.Sep 28 2018, 5:16 AM

Hmm, it's strange that we haven't hit this assert before, then. What is causing you to hit it?

At the very least this would need the comment explaining exactly what the ifdef is doing and why.

I think checking for _WIN32 is also not correct. clang-cl will for example 8-byte align both S0 and S1 from your example and maybe MinGW does too (I haven't checked).

Yes, you are right, both clang and MinGW align both S0 and S1 (x86 MinGW aligns the stack on 16 byte in the main function, so foo0 and foo1 become identical). I've updated the diff, thanks.

This one was hit during testing https://reviews.llvm.org/D52618, when running lldb-test ir-memory-map. I build LLVM with MSVC compiler, so the assert hits for IntervalMap<addr_t, unsigned, 8, IntervalMapHalfOpenInfo<addr_t>>.

rnk added a comment.Oct 1 2018, 1:44 PM

I think it would be easier to just slap LLVM_ALIGNAS(RootLeaf) on the member here to work around the bug / limitation of AlignedCharArrayUnion:

private:
  // The root data is either a RootLeaf or a RootBranchData instance.
  AlignedCharArrayUnion<RootLeaf, RootBranchData> data;

Yes, you are right, it's more simple! I've updated the patch.

rnk accepted this revision.Oct 8 2018, 10:51 AM

lgtm, thanks!

This revision is now accepted and ready to land.Oct 8 2018, 10:51 AM
This revision was automatically updated to reflect the committed changes.