[Sema] Don't call isNonConstantStorage on incomplete variable types #161590
Conversation
The code that applies section attributes to global variables was crashing when it encountered a variable whose type was incomplete. This change adds a check to ensure the variable's type is complete before calling the function. Fixes llvm#120371 rdar://155577913
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Akira Hatanaka (ahatanak) ChangesThe code that applies section attributes to global variables was crashing when it encountered a variable whose type was incomplete. This change adds a check to ensure the variable's type is complete before calling the function. Fixes #120371 rdar://155577913 Full diff: https://github.com/llvm/llvm-project/pull/161590.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 0069b08f1991a..e6271745e7ac1 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -14918,52 +14918,57 @@ void Sema::CheckCompleteVariableDeclaration(VarDecl *var) {
}
// Apply section attributes and pragmas to global variables.
- if (GlobalStorage && var->isThisDeclarationADefinition() &&
- !inTemplateInstantiation()) {
- PragmaStack<StringLiteral *> *Stack = nullptr;
- int SectionFlags = ASTContext::PSF_Read;
- bool MSVCEnv =
- Context.getTargetInfo().getTriple().isWindowsMSVCEnvironment();
- std::optional<QualType::NonConstantStorageReason> Reason;
- if (HasConstInit &&
- !(Reason = var->getType().isNonConstantStorage(Context, true, false))) {
- Stack = &ConstSegStack;
- } else {
- SectionFlags |= ASTContext::PSF_Write;
- Stack = var->hasInit() && HasConstInit ? &DataSegStack : &BSSSegStack;
- }
- if (const SectionAttr *SA = var->getAttr<SectionAttr>()) {
- if (SA->getSyntax() == AttributeCommonInfo::AS_Declspec)
+ [&]() {
+ if (GlobalStorage && var->isThisDeclarationADefinition() &&
+ !inTemplateInstantiation()) {
+ PragmaStack<StringLiteral *> *Stack = nullptr;
+ int SectionFlags = ASTContext::PSF_Read;
+ bool MSVCEnv =
+ Context.getTargetInfo().getTriple().isWindowsMSVCEnvironment();
+ std::optional<QualType::NonConstantStorageReason> Reason;
+ if (HasConstInit && var->getType()->isIncompleteType())
+ return;
+ if (HasConstInit && !(Reason = var->getType().isNonConstantStorage(
+ Context, true, false))) {
+ Stack = &ConstSegStack;
+ } else {
+ SectionFlags |= ASTContext::PSF_Write;
+ Stack = var->hasInit() && HasConstInit ? &DataSegStack : &BSSSegStack;
+ }
+ if (const SectionAttr *SA = var->getAttr<SectionAttr>()) {
+ if (SA->getSyntax() == AttributeCommonInfo::AS_Declspec)
+ SectionFlags |= ASTContext::PSF_Implicit;
+ UnifySection(SA->getName(), SectionFlags, var);
+ } else if (Stack->CurrentValue) {
+ if (Stack != &ConstSegStack && MSVCEnv &&
+ ConstSegStack.CurrentValue != ConstSegStack.DefaultValue &&
+ var->getType().isConstQualified()) {
+ assert((!Reason || Reason != QualType::NonConstantStorageReason::
+ NonConstNonReferenceType) &&
+ "This case should've already been handled elsewhere");
+ Diag(var->getLocation(), diag::warn_section_msvc_compat)
+ << var << ConstSegStack.CurrentValue
+ << (int)(!HasConstInit
+ ? QualType::NonConstantStorageReason::NonTrivialCtor
+ : *Reason);
+ }
SectionFlags |= ASTContext::PSF_Implicit;
- UnifySection(SA->getName(), SectionFlags, var);
- } else if (Stack->CurrentValue) {
- if (Stack != &ConstSegStack && MSVCEnv &&
- ConstSegStack.CurrentValue != ConstSegStack.DefaultValue &&
- var->getType().isConstQualified()) {
- assert((!Reason || Reason != QualType::NonConstantStorageReason::
- NonConstNonReferenceType) &&
- "This case should've already been handled elsewhere");
- Diag(var->getLocation(), diag::warn_section_msvc_compat)
- << var << ConstSegStack.CurrentValue << (int)(!HasConstInit
- ? QualType::NonConstantStorageReason::NonTrivialCtor
- : *Reason);
- }
- SectionFlags |= ASTContext::PSF_Implicit;
- auto SectionName = Stack->CurrentValue->getString();
- var->addAttr(SectionAttr::CreateImplicit(Context, SectionName,
- Stack->CurrentPragmaLocation,
- SectionAttr::Declspec_allocate));
- if (UnifySection(SectionName, SectionFlags, var))
- var->dropAttr<SectionAttr>();
- }
-
- // Apply the init_seg attribute if this has an initializer. If the
- // initializer turns out to not be dynamic, we'll end up ignoring this
- // attribute.
- if (CurInitSeg && var->getInit())
- var->addAttr(InitSegAttr::CreateImplicit(Context, CurInitSeg->getString(),
- CurInitSegLoc));
- }
+ auto SectionName = Stack->CurrentValue->getString();
+ var->addAttr(SectionAttr::CreateImplicit(
+ Context, SectionName, Stack->CurrentPragmaLocation,
+ SectionAttr::Declspec_allocate));
+ if (UnifySection(SectionName, SectionFlags, var))
+ var->dropAttr<SectionAttr>();
+ }
+
+ // Apply the init_seg attribute if this has an initializer. If the
+ // initializer turns out to not be dynamic, we'll end up ignoring this
+ // attribute.
+ if (CurInitSeg && var->getInit())
+ var->addAttr(InitSegAttr::CreateImplicit(
+ Context, CurInitSeg->getString(), CurInitSegLoc));
+ }
+ }();
// All the following checks are C++ only.
if (!getLangOpts().CPlusPlus) {
diff --git a/clang/test/SemaCXX/attr-section.cpp b/clang/test/SemaCXX/attr-section.cpp
index 1c07e3dd8bba2..3ec455d860ff0 100644
--- a/clang/test/SemaCXX/attr-section.cpp
+++ b/clang/test/SemaCXX/attr-section.cpp
@@ -69,3 +69,11 @@ __attribute__((section("non_trivial_ctor"))) const t1 v1; // expected-note {{dec
extern const t1 v2;
__attribute__((section("non_trivial_ctor"))) const t1 v2{3}; // expected-error {{'v2' causes a section type conflict with 'v1'}}
} // namespace non_trivial_ctor
+
+namespace incomplete_type {
+template <class T>
+struct A {
+ struct B;
+ static constexpr B b{nullptr};
+};
+}
|
clang/lib/Sema/SemaDecl.cpp
Outdated
| Context.getTargetInfo().getTriple().isWindowsMSVCEnvironment(); | ||
| std::optional<QualType::NonConstantStorageReason> Reason; | ||
| if (HasConstInit && var->getType()->isIncompleteType()) | ||
| return; |
There was a problem hiding this comment.
This doesn't seem like it's quite the right thing to check; we shouldn't be be trying to compute the section at all for a definition inside a templated class/function, no matter what the type is.
There was a problem hiding this comment.
If we stop computing the section for definitions inside templated classes/functions altogether, clang will stop emitting error 'm' causes a section type conflict with 'v1' when the following code is compiled. Is that okay?
template <class T>
struct C {
__attribute__((section("non_trivial_ctor")))
static constexpr int m{123};
};
There was a problem hiding this comment.
Seems fine. It's not actually causing a section conflict unless you instantiate it.
| // Apply section attributes and pragmas to global variables. | ||
| if (GlobalStorage && var->isThisDeclarationADefinition() && | ||
| !inTemplateInstantiation()) { | ||
| !var->getDeclContext()->isDependentContext()) { |
There was a problem hiding this comment.
Did you try !type->isDependentType() - we do that elsewhere
There was a problem hiding this comment.
To fill in some details, it is more consistent and it is cheaper since isDependentContext() looks at all the parent contexts recursively.
There was a problem hiding this comment.
Checking JUST the actual var decl is actually going to be a 'smaller' thing, but I'm not sure it is actually going to work? @ahatanak : please try it though, it is quite a bit easier.
The difference here is that if we switch to ONLY a dependent type (instead of a dependent decl), we end up in a situation where we might try to do this for a non-dependently typed variable in a dependent context. So I don't think that'll fix it?
Consider the example:
template <class T>
struct A {
struct B;
static constexpr B b{nullptr}; // This used to crash.
};
IN this case, B is a dependent type, since it is inside the struct (so it is A<T>::B. However consider this slight modification (please add this test!).
struct B;
template <class T>
struct A {
static constexpr B b{nullptr}; // This used to crash.
};
Note I moved B out of the template. Now b is still going to go down this path (I think?) but is no longer a dependent type, despite being a dependent context. I think this is wrong?
There was a problem hiding this comment.
Yes, clang computes the section and emits the diagnostic for b without instantiation if !type->isDependentType() is called instead of !var->getDeclContext()->isDependentContext().
struct B1 { void *p; };
template <class T>
struct A1 {
__attribute__((section("non_trivial_ctor")))
static constexpr B1 b{nullptr};
};
// auto *p = &A1<int>::b;
|
This change needs a release note. |
6 participants
The code that applies section attributes to global variables was crashing when it encountered a variable whose type was incomplete. This change adds a check to ensure the variable's type is complete before calling the function.
Fixes #120371
rdar://155577913