[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

13317 ↗

(On Diff #356401)

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

13317 ↗

(On Diff #356401)

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

13316 ↗

(On Diff #356409)

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

13317 ↗

(On Diff #356401)

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

13316 ↗

(On Diff #356409)

That looks much better, thanks!

[mizvekov]

• Use llvm::any_of.

[rsmith]

clang/lib/Sema/SemaDecl.cpp

13314 ↗

(On Diff #356568)

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/lib/Sema/SemaStmt.cpp

if (VDType->isObjectType()) {
     if (VDReferencedType.isVolatileQualified() ||
         !VDReferencedType->isObjectType())
       return NamedReturnInfo();
     Info.S = NamedReturnInfo::MoveEligible;
   } else {
     return NamedReturnInfo();
   }
 
+  auto isAlignmentDependent = [](const VarDecl *VD) {
+    for (auto *I : VD->specific_attrs<AlignedAttr>())
+      if (I->isAlignmentDependent())
+        return true;
+    return false;
+  };
+
   // Variables with higher required alignment than their type's ABI
   // alignment cannot use NRVO.
   if (!VDType->isDependentType() && VD->hasAttr<AlignedAttr>() &&
+      !isAlignmentDependent(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