Richard Smith <rich...@metafoo.co.uk> writes:
> On Tue, Jun 30, 2015 at 4:57 PM, Justin Bogner <m...@justinbogner.com> wrote:
>
> ping
>
> Justin Bogner <m...@justinbogner.com> writes:
> > If we hit an error already, we may have set Name to nullptr, which
> > means calling isAcceptableTagRedeclaration hits UB. Avoid this like we
> > do elsewhere in the function by checking Name first.
> >
> > We could also fix this by passing OrigName instead, since the name is
> > only used for diagnostics anyway. Should I do that instead, or is this
> > good to commit?
> >
> > commit 203946970eafb48a120b29dfac1612b8762b9115
> > Author: Justin Bogner <m...@justinbogner.com>
> > Date: Mon Jun 22 00:05:05 2015 -0700
> >
> > Sema: Fix some undefined behaviour when acting on redeclarations
> >
> > If we hit an error already, we may have set Name to nullptr, which
> > means calling isAcceptableTagRedeclaration hits UB. Avoid this like
> we
> > do elsewhere in the function by checking Name first.
> >
> > diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
> > index ce89d99..8c94460 100644
> > --- a/lib/Sema/SemaDecl.cpp
> > +++ b/lib/Sema/SemaDecl.cpp
> > @@ -11747,9 +11747,9 @@ Decl *Sema::ActOnTag(Scope *S, unsigned TagSpec,
> TagUseKind TUK,
> > SS.isNotEmpty() || isExplicitSpecialization)) {
> > // Make sure that this wasn't declared as an enum and now used
> as a
> > // struct or something similar.
> > - if (!isAcceptableTagRedeclaration(PrevTagDecl, Kind,
> > - TUK == TUK_Definition, KWLoc,
> > - *Name)) {
> > + 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().
> Also, do you have a testcase? Is this already caught by our testsuite + ubsan?
Yep, this is caught by ubsan.
> > bool SafeToContinue
> > = (PrevTagDecl->getTagKind() != TTK_Enum &&
> > Kind != TTK_Enum);
> _______________________________________________
> cfe-commits mailing list
> cfe-commits@cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
commit 503397856508ec455300d84419f000ced801266b
Author: Justin Bogner <m...@justinbogner.com>
Date: Mon Jun 22 00:05:05 2015 -0700
Sema: Fix some undefined behaviour when acting on redeclarations
If we hit an error already, we may have set Name to nullptr, which
means calling isAcceptableTagRedeclaration hits UB. Since the name is
only used for diagnostics here, pass OrigName instead.
diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
index 1dec6fc..83dfb45 100644
--- a/lib/Sema/SemaDecl.cpp
+++ b/lib/Sema/SemaDecl.cpp
@@ -11763,7 +11763,7 @@ Decl *Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
// struct or something similar.
if (!isAcceptableTagRedeclaration(PrevTagDecl, Kind,
TUK == TUK_Definition, KWLoc,
- *Name)) {
+ *OrigName)) {
bool SafeToContinue
= (PrevTagDecl->getTagKind() != TTK_Enum &&
Kind != TTK_Enum);
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
