This is an archive of the discontinued LLVM Phabricator instance.

[clang] fixes named return of variables with dependent alignment
ClosedPublic

Authored by mizvekov on Jul 2 2021, 3:59 PM.

Details

Summary

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

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Diff Detail

Event Timeline

mizvekov requested review of this revision.Jul 2 2021, 3:59 PM
mizvekov created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2021, 3:59 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaronpuchert added inline comments.
clang/lib/Sema/SemaStmt.cpp
3401

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

mizvekov updated this revision to Diff 356401.Jul 4 2021, 2:35 PM
  • Add tests.
  • Refactor common users of this new utility.
  • Use std::any_of.
mizvekov retitled this revision from DRAFT: [clang] fixes named return of variables with dependent alignment to [clang] fixes named return of variables with dependent alignment.Jul 4 2021, 2:36 PM
mizvekov edited the summary of this revision. (Show Details)
Quuxplusone added inline comments.
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 updated this revision to Diff 356403.Jul 4 2021, 3:09 PM
  • Rename predicate variable from I to AA.
mizvekov marked 2 inline comments as done.Jul 4 2021, 3:14 PM
mizvekov added inline comments.
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 updated this revision to Diff 356409.Jul 4 2021, 5:24 PM
mizvekov marked an inline comment as done.

format.

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.

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 updated this revision to Diff 356568.Jul 5 2021, 2:56 PM
  • Use llvm::any_of.
mizvekov marked an inline comment as done.Jul 5 2021, 2:58 PM
rsmith accepted this revision.Jul 5 2021, 3:47 PM
rsmith added inline comments.
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.

This revision is now accepted and ready to land.Jul 5 2021, 3:47 PM
mizvekov updated this revision to Diff 356576.Jul 5 2021, 4:16 PM
  • Move it to VarDecl.
mizvekov marked an inline comment as done.Jul 5 2021, 4:17 PM

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 reopened this revision.Jul 6 2021, 4:14 AM

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.

This revision is now accepted and ready to land.Jul 6 2021, 4:14 AM
mizvekov updated this revision to Diff 356833.Jul 6 2021, 4:15 PM
  • 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.