[clang] fixes named return of variables with dependent alignment

[mizvekov]

Summary

Named return of a variable with aligned attribute would
trip an assert in case alignment was dependent.

Signed-off-by: Matheus Izvekov <[e-mail]>

[aaronpuchert]

clang/lib/Sema/SemaStmt.cpp

3401

Perhaps use std::any_of? Maybe there is even a range wrapper in llvm.

[mizvekov]

• Add tests.
• Refactor common users of this new utility.
• Use std::any_of.

[• Quuxplusone]

clang/lib/Sema/SemaDecl.cpp

13318

Tangent: It would be nice to rename isAlignmentDependent into hasDependentAlignment or simply isDependent. IIUC, the intended meaning is "Is this alignment attribute 'dependent'?" and not (as the name would naturally parse) "is this alignment attribute 'alignment-dependent'?"

Arguably, it could also work to say I->isDependentAlignment() in the same way that we say getType()->isDependentType().

Speaking of which, maybe I should be renamed to AA or something? Is I really the right abbreviation for an AlignedAttr?

[mizvekov]

• Rename predicate variable from I to AA.

[mizvekov]

clang/lib/Sema/SemaDecl.cpp

13318

I agree, but it is not so simple. This attribute class is autogenerated from clang/include/clang/Basic/Attr.td and it's not obvious to me where that method name comes from. Probably the tablegen backend. But that is another area that I am not familiar with yet.
And there are a few users, and also a method named isAlignmentErrorDependent which would benefit from the same kind of rename.
This DR is supposed to be a quick fix for a regression, I better leave that tangent for another time :)

The second rename, well it is very common to name a predicate variable as I, but I renamed to AA as that is also good.

[mizvekov]

format.

[aaronpuchert]

I'd leave it to @rsmith to decide whether this makes sense in Sema.h.

I tend to follow the rule that if it's just used once I keep it local, but you might have good reasons for making it public.

clang/lib/Sema/SemaDecl.cpp

13317

From llvm/include/llvm/ADT/STLExtras.h.

13318

Seems plausible that it comes from

let Args = [ExprArgument<"Alignment">];

in AlignValue. An expression can be dependent, and to distinguish expression arguments, it probably generates functions isXXXDependent for ExprArgument<"XXX">. But I'm just guessing.

But yeah, nothing we can easily change here.

[mizvekov]

In D105380#2858638, @aaronpuchert wrote:

I tend to follow the rule that if it's just used once I keep it local, but you might have good reasons for making it public.

Well it is used twice.
Notice how I replaced a pre-existing static function called hasDependentAlignment with a static class member with the same name.
Since I kept the name, and it was used from a Sema member function, I did not have to change the code in the call site, hence it did not show up in the diff :)

clang/lib/Sema/SemaDecl.cpp

13317

That looks much better, thanks!

[mizvekov]

• Use llvm::any_of.

[rsmith]

clang/lib/Sema/SemaDecl.cpp

13314

It doesn't look like this has any particular relationship to Sema. Consider making this a member of VarDecl instead.

[mizvekov]

• Move it to VarDecl.

[MForster]

Adrian has reverted this commit for me. Here is a reproduction case for the issue that is triggered by this change:

#include <initializer_list>

template<typename T>
struct S {};

template<typename T>  
S<T> foo() {
  for (auto status : { S<T>() }) {
      return status;
  }
}

void a() {
  foo<int>();
}

This is the segmentation fault:

instantiating function definition 'foo<int>'
 #0 0x000055807ba49925 SignalHandler(int) (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x6649925)
 #1 0x00007f18a9d509a0 __restore_rt (/usr/grte/v4/lib64/libpthread.so.0+0xf9a0)
 #2 0x000055807901a01d clang::ASTContext::getTypeInfoImpl(clang::Type const*) const (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x3c1a01d)
 #3 0x0000558078ffcf93 clang::ASTContext::getPreferredTypeAlign(clang::Type const*) const (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x3bfcf93)
 #4 0x0000558078ffc4ea clang::ASTContext::getDeclAlign(clang::Decl const*, bool) const (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x3bfc4ea)
 #5 0x0000558078029686 clang::Sema::getNamedReturnInfo(clang::VarDecl const*) (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x2c29686)
 #6 0x0000558078026e84 clang::TemplateDeclInstantiator::VisitVarDecl(clang::VarDecl*, bool, llvm::ArrayRef<clang::BindingDecl*>*) (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x2c26e84)
 #7 0x0000558079388ebc clang::Sema::SubstDecl(clang::Decl*, clang::DeclContext*, clang::MultiLevelTemplateArgumentList const&) (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x3f88ebc)
 #8 0x0000558077fd8a2d clang::TreeTransform<(anonymous namespace)::TemplateInstantiator>::TransformDeclStmt(clang::DeclStmt*) (.llvm.3943398439186356346) (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x2bd8a2d)
 #9 0x000055807823f79f clang::TreeTransform<(anonymous namespace)::TemplateInstantiator>::TransformCXXForRangeStmt(clang::CXXForRangeStmt*) (.llvm.3943398439186356346) (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x2e3f79f)
#10 0x00005580794bfa18 clang::TreeTransform<(anonymous namespace)::TemplateInstantiator>::TransformCompoundStmt(clang::CompoundStmt*) (.llvm.3943398439186356346) (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x40bfa18)
#11 0x000055807940149f clang::Sema::InstantiateFunctionDefinition(clang::SourceLocation, clang::FunctionDecl*, bool, bool, bool) (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x400149f)
#12 0x00005580794027a3 clang::Sema::PerformPendingInstantiations(bool) (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x40027a3)
#13 0x00005580780f5443 clang::Sema::ActOnEndOfTranslationUnitFragment(clang::Sema::TUFragmentKind) (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x2cf5443)
#14 0x00005580780f461c clang::Sema::ActOnEndOfTranslationUnit() (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x2cf461c)
#15 0x00005580785530e3 clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, bool) (.cold) (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x31530e3)
#16 0x00005580780f224f clang::ParseAST(clang::Sema&, bool, bool) (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x2cf224f)
#17 0x00005580781fff95 clang::FrontendAction::Execute() (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x2dfff95)
#18 0x00005580781ffdc4 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x2dffdc4)
#19 0x00005580781ffc98 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x2dffc98)
#20 0x000055807974f570 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x434f570)
#21 0x00005580782a04b9 ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x2ea04b9)
#22 0x000055807964f49b main (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x424f49b)
#23 0x00007f18a9bbebbd __libc_start_main (/usr/grte/v4/lib64/libc.so.6+0x38bbd)
#24 0x0000558077fc92c9 _start (third_party/crosstool/v18/llvm_unstable/toolchain/bin/clang+0x2bc92c9)

[mizvekov]

Thanks for reporting that one!
Yeah looking if the type is dependent is not enough, a non-deduced auto type is not considered dependent.
I will work on a patch for it later today.

[mizvekov]

• hasDependentAlignment now returns true for VarDecls with undeduced auto types.
• Add couple of tests that cover these kinds of VarDecls appearing:
  • Valid code with a range for loop in dependent context.
  • Error recovery on invalid initialization of auto variable in dependent context.

Remove isInvalidDecl check on FinalizeDeclaration for alignment of TLS variables,
as this is covered by the undeduced auto check above.

[mizvekov]

.

clang/include/clang/Sema/Sema.h

public:
 
   void DiagnoseShadowingLambdaDecls(const sema::LambdaScopeInfo *LSI);
 
 private:
   /// Map of current shadowing declarations to shadowed declarations. Warn if
   /// it looks like the user is trying to modify the shadowing declaration.
   llvm::DenseMap<const NamedDecl *, const NamedDecl *> ShadowingDecls;
 
+  /// Determines if a variable's alignment is dependent.
+  static bool hasDependentAlignment(const VarDecl *VD);
+
 public:
   void CheckCastAlign(Expr *Op, QualType T, SourceRange TRange);
   void handleTagNumbering(const TagDecl *Tag, Scope *TagScope);
   void setTagNameForLinkagePurposes(TagDecl *TagFromDeclSpec,
                                     TypedefNameDecl *NewTD);
   void CheckTypedefForVariablyModifiedType(Scope *S, TypedefNameDecl *D);
   NamedDecl* ActOnTypedefDeclarator(Scope* S, Declarator& D, DeclContext* DC,
                                     TypeSourceInfo *TInfo,

clang/lib/Sema/SemaDecl.cpp

 #include "clang/Sema/DelayedDiagnostic.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/ParsedTemplate.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/Triple.h"
 #include <algorithm>
 #include <cstring>
 #include <functional>
 #include <unordered_map>
 
 using namespace clang;
void Sema::CheckCompleteVariableDeclaration(VarDecl *var) {
   if (Context.DeclMustBeEmitted(var) && !ModuleScopes.empty())
     Context.addModuleInitializer(ModuleScopes.back().Module, var);
 
   // Build the bindings if this is a structured binding declaration.
   if (auto *DD = dyn_cast<DecompositionDecl>(var))
     CheckCompleteDecompositionDeclaration(DD);
 }
 
-/// Determines if a variable's alignment is dependent.
-static bool hasDependentAlignment(VarDecl *VD) {
-  if (VD->getType()->isDependentType())
-    return true;
-  for (auto *I : VD->specific_attrs<AlignedAttr>())
-    if (I->isAlignmentDependent())
-      return true;
-  return false;
+bool Sema::hasDependentAlignment(const VarDecl *VD) {
+  return VD->getType()->isDependentType() ||
+         llvm::any_of(
+             VD->specific_attrs<AlignedAttr>(),
+             [](const AlignedAttr *AA) { return AA->isAlignmentDependent(); });
 }
 
 /// Check if VD needs to be dllexport/dllimport due to being in a
 /// dllexport/import function.
 void Sema::CheckStaticLocalForDllExport(VarDecl *VD) {
   assert(VD->isStaticLocal());
 
   auto *FD = dyn_cast_or_null<FunctionDecl>(VD->getParentFunctionOrMethod());

clang/lib/Sema/SemaStmt.cpp

if (VDReferencedType.isVolatileQualified() ||
       return NamedReturnInfo();
     Info.S = NamedReturnInfo::MoveEligible;
   } else {
     return NamedReturnInfo();
   }
 
   // Variables with higher required alignment than their type's ABI
   // alignment cannot use NRVO.
-  if (!VDType->isDependentType() && VD->hasAttr<AlignedAttr>() &&
+  if (!hasDependentAlignment(VD) &&
       Context.getDeclAlign(VD) > Context.getTypeAlignInChars(VDType))
     Info.S = NamedReturnInfo::MoveEligible;
 
   return Info;
 }
 
 /// Updates given NamedReturnInfo's move-eligible and
 /// copy-elidable statuses, considering the function
 /// return type criteria as applicable to return statements.
 ///
 /// \param Info The NamedReturnInfo object to update.

clang/test/CodeGen/nrvo-tracking.cpp

 // RUN: %clang_cc1 -std=c++20 -fblocks -Wno-return-stack-address -triple x86_64-unknown-unknown-gnu -emit-llvm -O1 -fexperimental-new-pass-manager -o - %s | FileCheck %s
 
-struct X {
-    X();
-    X(const X&);
-    X(X&&);
+struct alignas(4) X {
+  X(const X &);
+  X(X &&);
 };
 
 #define L(A, B, C) void l##A() {    \
   auto t = []<class T = X>() -> C { \
     T t;                            \
     return B;                       \
   }();                              \
 }

 void b_attr() {
   auto t = []<class T = X>() {
     return ^X() [[clang::vectorcall]] {
       T t;
       return t;
     };
   }()();
 }
+
+namespace test_alignas {
+
+template <int A> X t1() {
+  X a [[gnu::aligned(A)]];
+  return a;
+}
+
+// CHECK-LABEL: define{{.*}} void @_ZN12test_alignas2t1ILi1EEE1Xv
+// CHECK:       call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  ret void
+template X t1<1>();
+
+// CHECK-LABEL: define{{.*}} void @_ZN12test_alignas2t1ILi4EEE1Xv
+// CHECK:       call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  ret void
+template X t1<4>();
+
+// CHECK-LABEL: define{{.*}} void @_ZN12test_alignas2t1ILi8EEE1Xv
+// CHECK:       call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
+// CHECK-NEXT:  call void @llvm.lifetime.end
+template X t1<8>();
+
+template <int A> X t2() {
+  X a [[gnu::aligned(1)]] [[gnu::aligned(A)]] [[gnu::aligned(2)]];
+  return a;
+}
+
+// CHECK-LABEL: define{{.*}} void @_ZN12test_alignas2t2ILi1EEE1Xv
+// CHECK:       call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  ret void
+template X t2<1>();
+
+// CHECK-LABEL: define{{.*}} void @_ZN12test_alignas2t2ILi4EEE1Xv
+// CHECK:       call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  ret void
+template X t2<4>();
+
+// CHECK-LABEL: define{{.*}} void @_ZN12test_alignas2t2ILi8EEE1Xv
+// CHECK:       call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
+// CHECK-NEXT:  call void @llvm.lifetime.end
+template X t2<8>();
+
+// CHECK-LABEL: define{{.*}} void @_ZN12test_alignas2t3Ev
+// CHECK:       call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  ret void
+X t3() {
+  X a [[gnu::aligned(1)]];
+  return a;
+}
+
+// CHECK-LABEL: define{{.*}} void @_ZN12test_alignas2t4Ev
+// CHECK:       call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
+// CHECK-NEXT:  call void @llvm.lifetime.end
+X t4() {
+  X a [[gnu::aligned(8)]];
+  return a;
+}
+
+// CHECK-LABEL: define{{.*}} void @_ZN12test_alignas2t5Ev
+// CHECK:       call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
+// CHECK-NEXT:  call void @llvm.lifetime.end
+X t5() {
+  X a [[gnu::aligned(1)]] [[gnu::aligned(8)]];
+  return a;
+}
+
+} // namespace test_alignas