[clang] Handle __declspec() attributes in using

[thieta]

Summary

This patch fixes so that declspec attributes are forwarded
to the alias declaration.

Before this patch this would assert:

class Test { int a; };
using AlignedTest = __declspec(align(16)) const Test;
static_assert(alignof(AlignedTest) == 16, "error");

But afterwards it behaves the same as MSVC does and doesn't
assert.

Fixes: llvm/llvm-project#60513

[aaron.ballman]

Please be sure to add a release note for the fix.

clang/lib/Parse/ParseDecl.cpp

61–63

How about using an algorithm for this (would need to be reformatted for 80 col)?

65–66

Similar here, but this one might be less of a win in terms of readability.

clang/test/SemaCXX/using-declspec.cpp

5

Because we're touching ParseTypeName() and that is called in a lot of contexts, I think we should have extra testing in non-alias declaration cases and verify that MSVC behaves the same way. e.g., (search for ParseTypeName(), see where we parse type names, devise some test cases)

// I don't think the align attribute has a declaration to move onto in this case...
auto func() -> __declspec(align(16)) int;
// So I *think* the alignment of the return type isn't changed?
static_assert(alignof(decltype(func())) == alignof(int));

// Same here, no declaration to shift to
int i = (__declspec(align(16))int)12;

// But there is a declaration here!
typedef __declspec(align(16)) int Foo;
static_assert(alignof(Foo) == 16);

(There are possibly other interesting tests to consider.)

I suspect we should be issuing some "attribute ignored" diagnostics for the cases where the attribute has no effect (even if MSVC does not warn).

20

Please add a newline to the end of the file

[thieta]

• Expand on tests
• Fix crash when Attrs was null
• Added release note

[thieta]

Thanks for your comments - feel free to comment on the release note, I was struggling with describing the fix well for a short release note paragraph.

clang/lib/Parse/ParseDecl.cpp

61–63

I spent a while on trying to figure this out. I am not that great with the algorithms in the standard library since we don't really use that at work. But from my testing I couldn't get it to work:

• DS.getAttributes() returns a ParsedAttr&, but ToBeMoved needs a pointer to ParsedAttr. So your example code above fails because it can't convert ParsedAttr& to ParsedAttr* in the back_inserter()
• I tried to change ToBeMoved to be a list of references instead, but ParsedAttr disallows copying (I think, the error message was a bit confusing).
• I could do transform() to a list of pointers and then copy_if() but that seemed to really make it harder to understand.
• A transform_if() would solve the problem or I guess replace back_inserter() with some out iterator that could transform. But I couldn't find anything used like this browsing the LLVM source code.

So yeah - I might totally be missing something obvious here, feel free to point it out.

65–66

yeah I can do this - I'll wait to see what we do with the block above first.

clang/test/SemaCXX/using-declspec.cpp

5

Thanks for the expanded tests here! I tried all these tests on MSVC and they behave exactly as you described them above and they found bug in my implementation - Attrs in ParseTypeName() could be a nullptr.

We already emit warnings where the attribute is not used as you can see in the test case below.

[aaron.ballman]

LGTM aside from some minor nits that you can fix when landing, thank you for the fix!

clang/docs/ReleaseNotes.rst

112–114

clang/lib/Parse/ParseDecl.cpp

60–61

Formatting is off here.

61–63

So yeah - I might totally be missing something obvious here, feel free to point it out.

Thank you for trying it out, I'd say let's leave it as-is (but fix the formatting issues).

[thieta]

Thanks for the review and the explanations! I fixed your nits and landed this.

clang/docs/ReleaseNotes.rst

 Attribute Changes in Clang
 --------------------------
 
 Introduced a new function attribute ``__attribute__((unsafe_buffer_usage))``
 to be worn by functions containing buffer operations that could cause out of
 bounds memory accesses. It emits warnings at call sites to such functions when
 the flag ``-Wunsafe-buffer-usage`` is enabled.
 
+``__declspec`` attributes can now be used together with the using keyword. Before
+the attributes on ``__declspec`` was ignored, while now it will be forwarded to the
+point where the alias is used.
+
 Windows Support
 ---------------
 
 AIX Support
 -----------
 
 C Language Changes in Clang
 ---------------------------

clang/lib/Parse/ParseDecl.cpp

TypeResult Parser::ParseTypeName(SourceRange *Range, DeclaratorContext Context,
   // Parse the common declaration-specifiers piece.
   DeclSpec DS(AttrFactory);
   if (Attrs)
     DS.addAttributes(*Attrs);
   ParseSpecifierQualifierList(DS, AS, DSC);
   if (OwnedType)
     *OwnedType = DS.isTypeSpecOwned() ? DS.getRepAsDecl() : nullptr;
 
+  // Move declspec attributes to ParsedAttributes
+  if (Attrs) {
+    llvm::SmallVector<ParsedAttr *, 1> ToBeMoved;
+    for (ParsedAttr &AL : DS.getAttributes()) {
+      if (AL.isDeclspecAttribute())
+        ToBeMoved.push_back(&AL);
+    }
+
+    for (ParsedAttr *AL : ToBeMoved)
+      Attrs->takeOneFrom(DS.getAttributes(), AL);
+  }
+
   // Parse the abstract-declarator, if present.
   Declarator DeclaratorInfo(DS, ParsedAttributesView::none(), Context);
   ParseDeclarator(DeclaratorInfo);
   if (Range)
     *Range = DeclaratorInfo.getSourceRange();
 
   if (DeclaratorInfo.isInvalidType())
     return true;

clang/test/SemaCXX/using-declspec.cpp

+// RUN: %clang_cc1 -fms-compatibility -fsyntax-only -verify %s
+
+// This should ignore the alignment and issue a warning about
+// align not being used
+auto func() -> __declspec(align(16)) int; // expected-warning{{attribute ignored when parsing type}}
+static_assert(alignof(decltype(func())) == alignof(int));
+
+// The following should NOT assert since alignment should
+// follow the type
+struct Test { int a; };
+using AlignedTest = __declspec(align(16)) const Test;
+static_assert(alignof(AlignedTest) == 16, "error");
+
+// Same here, no declaration to shift to
+int i = (__declspec(align(16))int)12; // expected-warning{{attribute ignored when parsing type}}
+
+// But there is a declaration here!
+typedef __declspec(align(16)) int Foo;
+static_assert(alignof(Foo) == 16);