This is an archive of the discontinued LLVM Phabricator instance.

Fix bug 26547 - alignof should return ABI alignment, not preferred alignment
ClosedPublic

Authored by ubsan on Oct 12 2018, 10:08 AM.

Details

Summary
  • Add UETT_PreferredAlignOf to account for the difference between __alignof and alignof
  • AlignOfType now returns ABI alignment instead of preferred alignment iff clang-abi-compat > 7, and one uses _Alignof or alignof

bug link: https://bugs.llvm.org/show_bug.cgi?id=26547

Diff Detail

Repository
rL LLVM

Event Timeline

ubsan created this revision.Oct 12 2018, 10:08 AM

Thanks, this generally looks good, but it needs tests. For _Alignof, test/Sema/align-x86.c` would be a reasonable place for the test; for C++ alignof, I don't see a suitable existing test file, but you could add one to test/SemaCXX, perhaps based on test/Sema/align-x86.c.

include/clang/Basic/LangOptions.h
128 ↗(On Diff #169449)

Clang 7 was branched from SVN r338536.

131 ↗(On Diff #169449)

Nit: Capitalize "See", add period. But I think you can actually just drop this line: we don't really need to justify why we now follow the rules of the relevant language standards.

132 ↗(On Diff #169449)

You will also need to update the code in lib/Frontend/CompilerInvocation.cpp to use this new enumerator (search for Ver6 to find the code you need to change).

284 ↗(On Diff #169449)

Please avoid reflowing unrelated lines; it makes future "svn annotate"s less useful.

include/clang/Basic/TypeTraits.h
99–104 ↗(On Diff #169449)

Nit: comments should start with a capital letter and end with a period. Maybe also make these documentation comments (use ///)?

lib/AST/ExprConstant.cpp
5960 ↗(On Diff #169449)

LLVM / Clang convention is to start variable names with a capital letter.

ubsan updated this revision to Diff 169503.Oct 12 2018, 3:22 PM

add tests

fix nits

Please extend the tests to check that the -fclang-abi-compat=7 switch undoes your change here.

One other comment, and otherwise this looks good. Will you need someone to commit this for you?

test/SemaCXX/align-x86.cpp
4–5 ↗(On Diff #169503)

It's not OK to include standard library headers here -- there's no guarantee that anything sensible will be on the include path, and we want our tests to be self-contained. (Feel free to define your own mini-std::complex in this test.)

I'm having real difficulty with this - I get really odd errors in seemingly unrelated tests when I change things.

I'm having real difficulty with this - I get really odd errors in seemingly unrelated tests when I change things.

Is it possible you have LLVM and Clang checked out at different revisions? That's a common source of weird behavior, particularly in CodeGen tests.

Is it possible you have LLVM and Clang checked out at different revisions? That's a common source of weird behavior, particularly in CodeGen tests.

Unfortunately no - when I don't specifically don't set the type trait kind of __alignof to UETT_PreferredAlignOf, and remove the code in AlignOfType to differentiate between PreferredAlignOf and AlignOf, all the tests pass again.

when I don't specifically don't set the type trait kind of __alignof to UETT_PreferredAlignOf, and remove the code in AlignOfType to differentiate between PreferredAlignOf and AlignOf, all the tests pass again.

You've extended UnaryExprOrTypeTrait from having 4 enumerators to 5, which means it no longer fits in a 2-bit bitfield. Stmt::UnaryExprOrTypeTraitExprBitfields::Kind needs to be made wider.

... Yup, that'd do it. Thanks Richard!

Finish doing the thing! all tests are passing, at least on the revision I'm running on :)

@rsmith please check the thing!

rsmith accepted this revision.Oct 24 2018, 2:35 PM

Thanks! Can you also write something for the release notes (docs/ReleaseNotes.rst) describing the ABI change?

include/clang/Basic/DiagnosticSemaKinds.td
5478 ↗(On Diff #170814)

Please factor out this repeated %select and use a %sub instead.

lib/Sema/SemaExpr.cpp
3772 ↗(On Diff #170814)

Please reflow this to under 80 columns.

This revision is now accepted and ready to land.Oct 24 2018, 2:35 PM
ubsan updated this revision to Diff 170994.Oct 24 2018, 2:57 PM

Add ABI breakage information and reflow

I don't actually know how to send this upstream, since it's been accepted... How should I proceed?

I don't actually know how to send this upstream, since it's been accepted... How should I proceed?

If you don't have an SVN account, you can ask someone who does to commit it for you. (In this case, I'll do it.)

A few minor nit first, though :)

docs/ReleaseNotes.rst
80 ↗(On Diff #170994)

I don't think we need to say this. It could be set to 7 before, it just had no effect.

152 ↗(On Diff #170994)

which -> that

include/clang/Basic/DiagnosticSemaKinds.td
5478 ↗(On Diff #170814)

I think you may have missed this comment.

This revision was automatically updated to reflect the committed changes.

I'm surprised that nobody discussed the potential for ABI breakage of this change, which changes the result of the alignof operator. I'm trying to evaluate what to do in libc++ as a result of this change (now std::alignment_of does not match alignof) for PR39713, and I'm wondering whether this issue was considered here. I'm not saying this is a bad change, by the way, I'd simply like to understand what the implications are and find good arguments to convince me to follow the lead in libc++. Pinging @dexonsmith for awareness.

The ABI breakage was already there, in the difference between GCC and Clang

  • if one compiles against libc++ with gcc, it is not compatible with things

that are compiled with clang. I frankly think that being ABI compatible
with gcc (and correct according to the standard) is far more important than
being ABI compatible with previous versions of clang - if people need the
ABI compatibility, they can and should use -fclang-abi-compat