<div dir="ltr">Thanks Adrian!</div><br><div class="gmail_quote"><div dir="ltr">On Tue, Feb 23, 2016 at 9:19 AM Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">--> r261657.<br>
<br>
Author: adrian <adrian@91177308-0d34-0410-b5e6-96231b3b80d8><br>
Date: Tue Feb 23 17:13:47 2016 +0000<br>
<br>
Remove an unnecessary workaround introduced in r259975. (NFC)<br>
<br>
Now that LLVM r259973 allows replacing a temporary type with another<br>
temporary we can rely on the original implementation.<br>
<br>
It is possible for enums to be created as part of<br>
their own declcontext. In this case a FwdDecl will be created<br>
twice. This doesn't cause a problem because both FwdDecls are<br>
entered into the ReplaceMap: finalize() will replace the first<br>
FwdDecl with the second and then replace the second with<br>
complete type.<br>
<br>
Thanks to echristo for pointing this out.<br>
<br>
git-svn-id: <a href="https://llvm.org/svn/llvm-project/cfe/trunk@261657" rel="noreferrer" target="_blank">https://llvm.org/svn/llvm-project/cfe/trunk@261657</a><br>
<br>
-- adrian<br>
<br>
> On Feb 22, 2016, at 5:26 PM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> wrote:<br>
><br>
> Hi Adrian,<br>
><br>
> On Sat, Feb 20, 2016 at 1:18 PM Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br>
> This had me puzzled for a second, but then I figured out what happened :-)<br>
> The “crash” I quoted in the commit message really was an assertion failure, to be precise, the very assertion I relaxed in LLVM r259973 in order to be able to defer the building of the DeclContext implemented by this commit. Even with the assertion removed, I still believe that implementing it this way preferable, as it avoids creating the enum a second time.<br>
><br>
> > On Feb 19, 2016, at 7:05 PM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> wrote:<br>
> ><br>
> > Hi Adrian,<br>
> ><br>
> > I'm taking a look at this and can't duplicate using the testcase you gave without your patch(es) applied. It's also causing asserts in other code as you can have the forward decl left around and the CU isn't a valid context.<br>
><br>
> Do you happen to have an example handy? I don’t quite understand the problem from the description — shouldn’t the temporary fwddecl be RAUWed unconditionally after the DeclContext is created?<br>
><br>
><br>
> I'd think so. It appears to get through to code generation unreplaced though and...<br>
><br>
> If you’re blocked on this we can definitely revert the change in CGDebugInfo (but not the testcase), I just would like to understand what’s going on.<br>
><br>
><br>
> I am a bit. I don't have a reduction at the moment small enough to debug what's wrong with this and the current patch doesn't appear necessary so I'm unable to debug the original problem at this point to try to help more.<br>
><br>
> -eric<br>
><br>
> -- adrian<br>
><br>
> > Can you take a look/revert until you've got a different testcase? There's not enough information in the commit to construct one up for you.<br>
> ><br>
> > Thanks!<br>
> ><br>
> > -eric<br>
> ><br>
> > On Fri, Feb 5, 2016 at 6:03 PM Adrian Prantl via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
> > Author: adrian<br>
> > Date: Fri Feb 5 19:59:09 2016<br>
> > New Revision: 259975<br>
> ><br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=259975&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=259975&view=rev</a><br>
> > Log:<br>
> > Fix a crash when emitting dbeug info for forward-declared scoped enums.<br>
> > It is possible for enums to be created as part of their own<br>
> > declcontext. We need to cache a placeholder to avoid the type being<br>
> > created twice before hitting the cache.<br>
> ><br>
> > <rdar://problem/24493203><br>
> ><br>
> > Added:<br>
> > cfe/trunk/test/CodeGenCXX/debug-info-scoped-class.cpp<br>
> > Modified:<br>
> > cfe/trunk/lib/CodeGen/CGDebugInfo.cpp<br>
> ><br>
> > Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp<br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=259975&r1=259974&r2=259975&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=259975&r1=259974&r2=259975&view=diff</a><br>
> > ==============================================================================<br>
> > --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)<br>
> > +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Fri Feb 5 19:59:09 2016<br>
> > @@ -2051,13 +2051,25 @@ llvm::DIType *CGDebugInfo::CreateEnumTyp<br>
> > // If this is just a forward declaration, construct an appropriately<br>
> > // marked node and just return it.<br>
> > if (isImportedFromModule || !ED->getDefinition()) {<br>
> > - llvm::DIScope *EDContext = getDeclContextDescriptor(ED);<br>
> > llvm::DIFile *DefUnit = getOrCreateFile(ED->getLocation());<br>
> > +<br>
> > + // It is possible for enums to be created as part of their own<br>
> > + // declcontext. We need to cache a placeholder to avoid the type being<br>
> > + // created twice before hitting the cache.<br>
> > + llvm::DIScope *EDContext = DBuilder.createReplaceableCompositeType(<br>
> > + llvm::dwarf::DW_TAG_enumeration_type, "", TheCU, DefUnit, 0);<br>
> > +<br>
> > unsigned Line = getLineNumber(ED->getLocation());<br>
> > StringRef EDName = ED->getName();<br>
> > llvm::DIType *RetTy = DBuilder.createReplaceableCompositeType(<br>
> > llvm::dwarf::DW_TAG_enumeration_type, EDName, EDContext, DefUnit, Line,<br>
> > 0, Size, Align, llvm::DINode::FlagFwdDecl, FullName);<br>
> > +<br>
> > + // Cache the enum type so it is available when building the declcontext<br>
> > + // and replace the declcontect with the real thing.<br>
> > + TypeCache[Ty].reset(RetTy);<br>
> > + EDContext->replaceAllUsesWith(getDeclContextDescriptor(ED));<br>
> > +<br>
> > ReplaceMap.emplace_back(<br>
> > std::piecewise_construct, std::make_tuple(Ty),<br>
> > std::make_tuple(static_cast<llvm::Metadata *>(RetTy)));<br>
> ><br>
> > Added: cfe/trunk/test/CodeGenCXX/debug-info-scoped-class.cpp<br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/debug-info-scoped-class.cpp?rev=259975&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/debug-info-scoped-class.cpp?rev=259975&view=auto</a><br>
> > ==============================================================================<br>
> > --- cfe/trunk/test/CodeGenCXX/debug-info-scoped-class.cpp (added)<br>
> > +++ cfe/trunk/test/CodeGenCXX/debug-info-scoped-class.cpp Fri Feb 5 19:59:09 2016<br>
> > @@ -0,0 +1,15 @@<br>
> > +// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -std=c++11 \<br>
> > +// RUN: -triple thumbv7-apple-ios %s -o - | FileCheck %s<br>
> > +<br>
> > +// This forward-declared scoped enum will be created while building its own<br>
> > +// declcontext. Make sure it is only emitted once.<br>
> > +<br>
> > +struct A {<br>
> > + enum class Return;<br>
> > + Return f1();<br>
> > +};<br>
> > +A::Return* f2() {}<br>
> > +<br>
> > +// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "Return",<br>
> > +// CHECK-SAME: flags: DIFlagFwdDecl,<br>
> > +// CHECK-NOT: tag: DW_TAG_enumeration_type, name: "Return"<br>
> ><br>
> ><br>
> > _______________________________________________<br>
> > cfe-commits mailing list<br>
> > <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
> > <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
<br>
</blockquote></div>