This is an archive of the discontinued LLVM Phabricator instance.

[AST] Add C++11 attribute 'msvc::no_unique_address'
AbandonedPublic

Authored by RIscRIpt on Sep 22 2022, 10:34 AM.

Details

Reviewers
aaron.ballman

Event Timeline

RIscRIpt created this revision.Sep 22 2022, 10:34 AM
Herald added a project: Restricted Project. · View Herald Transcript
RIscRIpt requested review of this revision.Sep 22 2022, 10:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2022, 10:34 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
RIscRIpt updated this revision to Diff 462234.Sep 22 2022, 10:37 AM
RIscRIpt retitled this revision from [AST] Add msvc-specific C++11 attribute 'msvc::no_unique_address' to [AST] Add msvc-specific C++11 attributes.
RIscRIpt edited the summary of this revision. (Show Details)

Add msvc::constexpr commit

Can you split this out into two separate reviews? That makes it easier for the reviewers so we don't mix up context as well as when we do code archeology for changes. (I also made some quick comments as I noticed things in the file, but I've not done a full review yet.)

clang/include/clang/AST/Decl.h
2974
clang/include/clang/Basic/Attr.td
3503

Type attribute? That seems surprising -- this is probably meant to be InheritableAttr?

3510

Hmmm, very odd that the msvc no unique address attribute is only supported on Itanium ABI targets. Should this be TargetMicrosoftCXXABI instead?

clang/include/clang/Basic/DiagnosticSemaKinds.td
3366–3368 ↗(On Diff #462234)

You shouldn't need to add this diagnostic -- if the Subjects list is correct in Attr.td, we automatically diagnose this sort of thing.

RIscRIpt updated this revision to Diff 462274.Sep 22 2022, 1:09 PM
RIscRIpt retitled this revision from [AST] Add msvc-specific C++11 attributes to [AST] Add msvc-specific C++11 attribute 'msvc::no_unique_address'.

Remove msvc::constexpr commit

Can you split this out into two separate reviews?

Sure, done: https://reviews.llvm.org/D134475

clang/include/clang/Basic/Attr.td
3510

My fault, I did blind copy-paste completely forgetting about ABI!

RIscRIpt planned changes to this revision.Sep 22 2022, 2:20 PM
RIscRIpt updated this revision to Diff 462297.Sep 22 2022, 2:43 PM
RIscRIpt retitled this revision from [AST] Add msvc-specific C++11 attribute 'msvc::no_unique_address' to [AST] Add C++11 attribute 'msvc::no_unique_address'.

Add naive implementation ABI of [[msvc::no_unique_address]]

RIscRIpt planned changes to this revision.Sep 22 2022, 2:44 PM

TODO: add proper ABI tests of [[msvc::no_unique_address]]

RIscRIpt edited the summary of this revision. (Show Details)Sep 23 2022, 1:56 AM
aaron.ballman added inline comments.Sep 26 2022, 2:31 PM
clang/include/clang/Basic/Attr.td
3505

Does the 201803 value match what MSVC returns from __has_cpp_attribute?

clang/test/AST/msvc-attrs.cpp
4

Thanks for the start to the tests! You should also add CodeGen tests as well as Sema tests. The CodeGen tests are to make sure we're codegenning the correct offsets to members, and the Sema tests are to ensure we're matching diagnostic behavior with MSVC rather than accepting code MSVC rejects.

RIscRIpt abandoned this revision.Nov 4 2023, 2:59 PM

Abandoning due to lack of time and expertise. I might come back and tackle it in several months.