Justin Bogner <m...@justinbogner.com> writes:
> Justin Bogner <m...@justinbogner.com> writes:
>> Richard Smith <rich...@metafoo.co.uk> writes:
>>> On Tue, Jun 30, 2015 at 5:32 PM, Justin Bogner <m...@justinbogner.com>
>>> wrote:
>>>> Richard Smith <rich...@metafoo.co.uk> writes:
>>>>> On Tue, Jun 30, 2015 at 4:57 PM, Justin Bogner
>>>>> <m...@justinbogner.com> wrote:
>>>>>> + if (Name && !isAcceptableTagRedeclaration(PrevTagDecl, Kind,
>>>>>> + TUK == TUK_Definition, KWLoc,
>>>>>> + *Name)) {
>>>>>
>>>>> So, this is tricky: if SafeToContinue is false, we don't want to
>>>>> carry on because we'd make an EnumDecl a redeclaration of a
>>>>> RecordDecl or vicer versa, which would violate the AST
>>>>> invariants. I'm not sure how we get here from error recovery:
>>>>> both places that set Name to nullptr also jump past this code.
>>>>> I think the only way to reach this is if SkipBody->Previous is
>>>>> non-null, in which case this is not error recovery and we should
>>>>> perform this check.
>>>>>
>>>>> Can you instead change isAcceptableTagRedeclaration to take a
>>>>> const IdentifierInfo*?
>>>>
>>>> The IdentifierInfo is only used in a diagnostic - I think it makes sense
>>>> to pass `OrigName` here if we want the check. This is what we do a
>>>> little below for diagnoseQualifiedDeclaration().
>>>
>>> But OrigName is null too, isn't it? The only places that make Name and
>>> OrigName differ also jump past this code.
>>
>> You're right - I misread the assert where OrigName is assigned and was
>> confused.
>>
>> I don't think passing const IdentifierInfo* is correct -
>> isAcceptableTagRedeclaration implements [dcl.type.elab]p3, which is
>> about elaborated-type-specifiers with the same name agreeing on kind.
>> It doesn't make much sense to do this check if there isn't a name at
>> all, and it seems suspicious to me that we're getting here at all.
>>
>> What does !Previous.empty() mean if we don't have a Name?
>>
>> This behaviour only triggers on one test in the test suite,
>> test/Modules/submodules-merge-defs.cpp. You can reproduce with
>> assert(Name) before the call to isAcceptableTagRedeclaration() if you're
>> curious.
>
> FWIW, we're no longer hitting the UB here as of r241763 - I believe
> r241743 is the relevant commit. It's not totally clear to me whether
> that fixed or hid the bug though.
Sorry, I was wrong about this, but now I understand how we get here.
This fires when we're merging anonymous enum definitions (as per
r236690). We're merging an enum like the following with a definition
we've already seen:
enum { f, g, h };
In this case, it's perfectly reasonable to not have a name and we do
want to check that the redecl is compatible, so I've gone with the
"const IdentifierInfo *" solution, attached. Okay to commit?
On a side note, passing null pointers to `operator<<(const
DiagnosticBuilder, const IdentifierInfo)` eventually leads to this code:
case DiagnosticsEngine::ak_identifierinfo: {
const IdentifierInfo *II = getArgIdentifier(ArgNo);
assert(ModifierLen == 0 && "No modifiers for strings yet");
// Don't crash if get passed a null pointer by accident.
if (!II) {
const char *S = "(null)";
OutStr.append(S, S + strlen(S));
continue;
}
llvm::raw_svector_ostream(OutStr) << '\'' << II->getName() << '\'';
break;
}
Here, the comment claims that passing a null pointer is an accident, but
we seem to do it intentionally (at least, a few tests fail if I change
it to an assert). Is the comment wrong, or should we look into fixing
that?
commit 700b866ff0b5d65007a836dd0cbcf800d326f4b2
Author: Justin Bogner <m...@justinbogner.com>
Date: Fri Jul 10 14:41:48 2015 -0700
Sema: Allow null names to be passed in to isAcceptableTagRedeclaration
It's possible for TagRedeclarations to involve decls without a name,
ie, anonymous enums. We hit some undefined behaviour if we bind these
null names to the reference here.
We never dereference the name, so it's harmless if it's null - make it
a pointer to allow that.
Fixes the Modules/submodules-merge-defs.cpp test under ubsan.
diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
index b5a1b0b..ca06b21 100644
--- a/include/clang/Sema/Sema.h
+++ b/include/clang/Sema/Sema.h
@@ -1849,7 +1849,7 @@ public:
bool isAcceptableTagRedeclaration(const TagDecl *Previous,
TagTypeKind NewTag, bool isDefinition,
SourceLocation NewTagLoc,
- const IdentifierInfo &Name);
+ const IdentifierInfo *Name);
enum TagUseKind {
TUK_Reference, // Reference to a tag: 'struct foo *X;'
diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
index 9db14bf..2887721 100644
--- a/lib/Sema/SemaDecl.cpp
+++ b/lib/Sema/SemaDecl.cpp
@@ -11294,7 +11294,7 @@ static bool isClassCompatTagKind(TagTypeKind Tag)
bool Sema::isAcceptableTagRedeclaration(const TagDecl *Previous,
TagTypeKind NewTag, bool isDefinition,
SourceLocation NewTagLoc,
- const IdentifierInfo &Name) {
+ const IdentifierInfo *Name) {
// C++ [dcl.type.elab]p3:
// The class-key or enum keyword present in the
// elaborated-type-specifier shall agree in kind with the
@@ -11323,7 +11323,7 @@ bool Sema::isAcceptableTagRedeclaration(const TagDecl *Previous,
// In a template instantiation, do not offer fix-its for tag mismatches
// since they usually mess up the template instead of fixing the problem.
Diag(NewTagLoc, diag::warn_struct_class_tag_mismatch)
- << getRedeclDiagFromTagKind(NewTag) << isTemplate << &Name
+ << getRedeclDiagFromTagKind(NewTag) << isTemplate << Name
<< getRedeclDiagFromTagKind(OldTag);
return true;
}
@@ -11342,7 +11342,7 @@ bool Sema::isAcceptableTagRedeclaration(const TagDecl *Previous,
if (!previousMismatch) {
previousMismatch = true;
Diag(NewTagLoc, diag::warn_struct_class_previous_tag_mismatch)
- << getRedeclDiagFromTagKind(NewTag) << isTemplate << &Name
+ << getRedeclDiagFromTagKind(NewTag) << isTemplate << Name
<< getRedeclDiagFromTagKind(I->getTagKind());
}
Diag(I->getInnerLocStart(), diag::note_struct_class_suggestion)
@@ -11364,7 +11364,7 @@ bool Sema::isAcceptableTagRedeclaration(const TagDecl *Previous,
}
Diag(NewTagLoc, diag::warn_struct_class_tag_mismatch)
- << getRedeclDiagFromTagKind(NewTag) << isTemplate << &Name
+ << getRedeclDiagFromTagKind(NewTag) << isTemplate << Name
<< getRedeclDiagFromTagKind(OldTag);
Diag(Redecl->getLocation(), diag::note_previous_use);
@@ -11847,7 +11847,7 @@ Decl *Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
// struct or something similar.
if (!isAcceptableTagRedeclaration(PrevTagDecl, Kind,
TUK == TUK_Definition, KWLoc,
- *Name)) {
+ Name)) {
bool SafeToContinue
= (PrevTagDecl->getTagKind() != TTK_Enum &&
Kind != TTK_Enum);
diff --git a/lib/Sema/SemaTemplate.cpp b/lib/Sema/SemaTemplate.cpp
index 97904ce..035c37c 100644
--- a/lib/Sema/SemaTemplate.cpp
+++ b/lib/Sema/SemaTemplate.cpp
@@ -1008,7 +1008,7 @@ Sema::CheckClassTemplate(Scope *S, unsigned TagSpec, TagUseKind TUK,
// template declaration (7.1.5.3).
RecordDecl *PrevRecordDecl = PrevClassTemplate->getTemplatedDecl();
if (!isAcceptableTagRedeclaration(PrevRecordDecl, Kind,
- TUK == TUK_Definition, KWLoc, *Name)) {
+ TUK == TUK_Definition, KWLoc, Name)) {
Diag(KWLoc, diag::err_use_with_wrong_tag)
<< Name
<< FixItHint::CreateReplacement(KWLoc, PrevRecordDecl->getKindName());
@@ -2310,7 +2310,7 @@ TypeResult Sema::ActOnTagTemplateIdType(TagUseKind TUK,
assert(Id && "templated class must have an identifier");
if (!isAcceptableTagRedeclaration(D, TagKind, TUK == TUK_Definition,
- TagLoc, *Id)) {
+ TagLoc, Id)) {
Diag(TagLoc, diag::err_use_with_wrong_tag)
<< Result
<< FixItHint::CreateReplacement(SourceRange(TagLoc), D->getKindName());
@@ -6199,7 +6199,7 @@ Sema::ActOnClassTemplateSpecialization(Scope *S, unsigned TagSpec,
assert(Kind != TTK_Enum && "Invalid enum tag in class template spec!");
if (!isAcceptableTagRedeclaration(ClassTemplate->getTemplatedDecl(),
Kind, TUK == TUK_Definition, KWLoc,
- *ClassTemplate->getIdentifier())) {
+ ClassTemplate->getIdentifier())) {
Diag(KWLoc, diag::err_use_with_wrong_tag)
<< ClassTemplate
<< FixItHint::CreateReplacement(KWLoc,
@@ -7235,7 +7235,7 @@ Sema::ActOnExplicitInstantiation(Scope *S,
if (!isAcceptableTagRedeclaration(ClassTemplate->getTemplatedDecl(),
Kind, /*isDefinition*/false, KWLoc,
- *ClassTemplate->getIdentifier())) {
+ ClassTemplate->getIdentifier())) {
Diag(KWLoc, diag::err_use_with_wrong_tag)
<< ClassTemplate
<< FixItHint::CreateReplacement(KWLoc,
diff --git a/lib/Sema/SemaTemplateInstantiate.cpp b/lib/Sema/SemaTemplateInstantiate.cpp
index 7d58017..c1961e5 100644
--- a/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/lib/Sema/SemaTemplateInstantiate.cpp
@@ -989,7 +989,7 @@ TemplateInstantiator::RebuildElaboratedType(SourceLocation KeywordLoc,
if (Id && Keyword != ETK_None && Keyword != ETK_Typename) {
TagTypeKind Kind = TypeWithKeyword::getTagTypeKindForKeyword(Keyword);
if (!SemaRef.isAcceptableTagRedeclaration(TD, Kind, /*isDefinition*/false,
- TagLocation, *Id)) {
+ TagLocation, Id)) {
SemaRef.Diag(TagLocation, diag::err_use_with_wrong_tag)
<< Id
<< FixItHint::CreateReplacement(SourceRange(TagLocation),
diff --git a/lib/Sema/TreeTransform.h b/lib/Sema/TreeTransform.h
index 8b150c3..6e193a3 100644
--- a/lib/Sema/TreeTransform.h
+++ b/lib/Sema/TreeTransform.h
@@ -1010,7 +1010,7 @@ public:
}
if (!SemaRef.isAcceptableTagRedeclaration(Tag, Kind, /*isDefinition*/false,
- IdLoc, *Id)) {
+ IdLoc, Id)) {
SemaRef.Diag(KeywordLoc, diag::err_use_with_wrong_tag) << Id;
SemaRef.Diag(Tag->getLocation(), diag::note_previous_use);
return QualType();
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
