<div dir="ltr">That's no problem. Just thought I'd ask, since it was a small fix for a segfault in LLVM. ;) As an aside, I think at least part of why it doesn't apply cleanly is due to the micro-optimization that was made to isSizedDerivedType (r196604) shortly before I committed my fix (I had to resolve a few conflicts when updating my local tree as I was about to commit).</div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Sat, Dec 7, 2013 at 1:19 PM, Bill Wendling <span dir="ltr"><<a href="mailto:isanbard@gmail.com" target="_blank">isanbard@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word">I’m hesitant to do that, because it’s not a regression from 3.3 and we’re deep into the final stages of the release. Also the patch doesn’t apply cleanly...<span class="HOEnZb"><font color="#888888"><div>
<br></div><div>-bw</div></font></span><div><div class="h5"><div><br><div><div>On Dec 6, 2013, at 4:24 PM, Kaelyn Takata <<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>> wrote:</div><br><blockquote type="cite">
<div dir="ltr">Do we want to pull this into the release branch? The segfault has apparently existed since LLVM 3.0.<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Dec 6, 2013 at 4:13 PM, Kaelyn Uhrain <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: rikka<br>
Date: Fri Dec 6 18:13:34 2013<br>
New Revision: 196626<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=196626&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=196626&view=rev</a><br>
Log:<br>
Fix the segfault reported in PR 11990.<br>
<br>
The sefault occurs due to an infinite loop when the verifier tries to<br>
determine the size of a type of the form "%rt = type { %rt }" while<br>
checking an alloca of the type.<br>
<br>
Added:<br>
llvm/trunk/test/Verifier/recursive-type-1.ll<br>
llvm/trunk/test/Verifier/recursive-type-2.ll<br>
llvm/trunk/test/Verifier/recursive-type-3.ll<br>
Modified:<br>
llvm/trunk/include/llvm/IR/DerivedTypes.h<br>
llvm/trunk/include/llvm/IR/Type.h<br>
llvm/trunk/lib/IR/Type.cpp<br>
llvm/trunk/lib/IR/Verifier.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/IR/DerivedTypes.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DerivedTypes.h?rev=196626&r1=196625&r2=196626&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DerivedTypes.h?rev=196626&r1=196625&r2=196626&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/IR/DerivedTypes.h (original)<br>
+++ llvm/trunk/include/llvm/IR/DerivedTypes.h Fri Dec 6 18:13:34 2013<br>
@@ -249,7 +249,7 @@ public:<br>
bool isOpaque() const { return (getSubclassData() & SCDB_HasBody) == 0; }<br>
<br>
/// isSized - Return true if this is a sized type.<br>
- bool isSized() const;<br>
+ bool isSized(SmallPtrSet<const Type*, 4> *Visited = 0) const;<br>
<br>
/// hasName - Return true if this is a named struct that has a non-empty name.<br>
bool hasName() const { return SymbolTableEntry != 0; }<br>
<br>
Modified: llvm/trunk/include/llvm/IR/Type.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Type.h?rev=196626&r1=196625&r2=196626&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Type.h?rev=196626&r1=196625&r2=196626&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/IR/Type.h (original)<br>
+++ llvm/trunk/include/llvm/IR/Type.h Fri Dec 6 18:13:34 2013<br>
@@ -16,6 +16,7 @@<br>
#define LLVM_IR_TYPE_H<br>
<br>
#include "llvm/ADT/APFloat.h"<br>
+#include "llvm/ADT/SmallPtrSet.h"<br>
#include "llvm/Support/Casting.h"<br>
#include "llvm/Support/CBindingWrapping.h"<br>
#include "llvm/Support/DataTypes.h"<br>
@@ -275,7 +276,7 @@ public:<br>
/// get the actual size for a particular target, it is reasonable to use the<br>
/// DataLayout subsystem to do this.<br>
///<br>
- bool isSized() const {<br>
+ bool isSized(SmallPtrSet<const Type*, 4> *Visited = 0) const {<br>
// If it's a primitive, it is always sized.<br>
if (getTypeID() == IntegerTyID || isFloatingPointTy() ||<br>
getTypeID() == PointerTyID ||<br>
@@ -287,7 +288,7 @@ public:<br>
getTypeID() != VectorTyID)<br>
return false;<br>
// Otherwise we have to try harder to decide.<br>
- return isSizedDerivedType();<br>
+ return isSizedDerivedType(Visited);<br>
}<br>
<br>
/// getPrimitiveSizeInBits - Return the basic size of this type if it is a<br>
@@ -429,7 +430,7 @@ private:<br>
/// isSizedDerivedType - Derived types like structures and arrays are sized<br>
/// iff all of the members of the type are sized as well. Since asking for<br>
/// their size is relatively uncommon, move this operation out of line.<br>
- bool isSizedDerivedType() const;<br>
+ bool isSizedDerivedType(SmallPtrSet<const Type*, 4> *Visited = 0) const;<br>
};<br>
<br>
// Printing of types.<br>
<br>
Modified: llvm/trunk/lib/IR/Type.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Type.cpp?rev=196626&r1=196625&r2=196626&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Type.cpp?rev=196626&r1=196625&r2=196626&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/IR/Type.cpp (original)<br>
+++ llvm/trunk/lib/IR/Type.cpp Fri Dec 6 18:13:34 2013<br>
@@ -155,14 +155,14 @@ int Type::getFPMantissaWidth() const {<br>
/// isSizedDerivedType - Derived types like structures and arrays are sized<br>
/// iff all of the members of the type are sized as well. Since asking for<br>
/// their size is relatively uncommon, move this operation out of line.<br>
-bool Type::isSizedDerivedType() const {<br>
+bool Type::isSizedDerivedType(SmallPtrSet<const Type*, 4> *Visited) const {<br>
if (const ArrayType *ATy = dyn_cast<ArrayType>(this))<br>
- return ATy->getElementType()->isSized();<br>
+ return ATy->getElementType()->isSized(Visited);<br>
<br>
if (const VectorType *VTy = dyn_cast<VectorType>(this))<br>
- return VTy->getElementType()->isSized();<br>
+ return VTy->getElementType()->isSized(Visited);<br>
<br>
- return cast<StructType>(this)->isSized();<br>
+ return cast<StructType>(this)->isSized(Visited);<br>
}<br>
<br>
//===----------------------------------------------------------------------===//<br>
@@ -550,17 +550,20 @@ StructType *StructType::create(StringRef<br>
return llvm::StructType::create(Ctx, StructFields, Name);<br>
}<br>
<br>
-bool StructType::isSized() const {<br>
+bool StructType::isSized(SmallPtrSet<const Type*, 4> *Visited) const {<br>
if ((getSubclassData() & SCDB_IsSized) != 0)<br>
return true;<br>
if (isOpaque())<br>
return false;<br>
<br>
+ if (Visited && !Visited->insert(this))<br>
+ return false;<br>
+<br>
// Okay, our struct is sized if all of the elements are, but if one of the<br>
// elements is opaque, the struct isn't sized *yet*, but may become sized in<br>
// the future, so just bail out without caching.<br>
for (element_iterator I = element_begin(), E = element_end(); I != E; ++I)<br>
- if (!(*I)->isSized())<br>
+ if (!(*I)->isSized(Visited))<br>
return false;<br>
<br>
// Here we cheat a bit and cast away const-ness. The goal is to memoize when<br>
<br>
Modified: llvm/trunk/lib/IR/Verifier.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Verifier.cpp?rev=196626&r1=196625&r2=196626&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Verifier.cpp?rev=196626&r1=196625&r2=196626&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/IR/Verifier.cpp (original)<br>
+++ llvm/trunk/lib/IR/Verifier.cpp Fri Dec 6 18:13:34 2013<br>
@@ -1861,11 +1861,12 @@ void Verifier::visitStoreInst(StoreInst<br>
}<br>
<br>
void Verifier::visitAllocaInst(AllocaInst &AI) {<br>
+ SmallPtrSet<const Type*, 4> Visited;<br>
PointerType *PTy = AI.getType();<br>
Assert1(PTy->getAddressSpace() == 0,<br>
"Allocation instruction pointer not in the generic address space!",<br>
&AI);<br>
- Assert1(PTy->getElementType()->isSized(), "Cannot allocate unsized type",<br>
+ Assert1(PTy->getElementType()->isSized(&Visited), "Cannot allocate unsized type",<br>
&AI);<br>
Assert1(AI.getArraySize()->getType()->isIntegerTy(),<br>
"Alloca array size must have integer type", &AI);<br>
<br>
Added: llvm/trunk/test/Verifier/recursive-type-1.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Verifier/recursive-type-1.ll?rev=196626&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Verifier/recursive-type-1.ll?rev=196626&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/test/Verifier/recursive-type-1.ll (added)<br>
+++ llvm/trunk/test/Verifier/recursive-type-1.ll Fri Dec 6 18:13:34 2013<br>
@@ -0,0 +1,12 @@<br>
+; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s<br>
+<br>
+%rt2 = type { i32, { i8, %rt2, i8 }, i32 }<br>
+<br>
+define i32 @main() nounwind {<br>
+entry:<br>
+ ; Check that recursive types trigger an error instead of segfaulting, when<br>
+ ; the recursion isn't through a pointer to the type.<br>
+ ; CHECK: Cannot allocate unsized type<br>
+ %0 = alloca %rt2<br>
+ ret i32 0<br>
+}<br>
<br>
Added: llvm/trunk/test/Verifier/recursive-type-2.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Verifier/recursive-type-2.ll?rev=196626&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Verifier/recursive-type-2.ll?rev=196626&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/test/Verifier/recursive-type-2.ll (added)<br>
+++ llvm/trunk/test/Verifier/recursive-type-2.ll Fri Dec 6 18:13:34 2013<br>
@@ -0,0 +1,14 @@<br>
+; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s<br>
+<br>
+%rt1 = type { i32, { i8, %rt2, i8 }, i32 }<br>
+%rt2 = type { i64, { i6, %rt3 } }<br>
+%rt3 = type { %rt1 }<br>
+<br>
+define i32 @main() nounwind {<br>
+entry:<br>
+ ; Check that mutually recursive types trigger an error instead of segfaulting,<br>
+ ; when the recursion isn't through a pointer to the type.<br>
+ ; CHECK: Cannot allocate unsized type<br>
+ %0 = alloca %rt2<br>
+ ret i32 0<br>
+}<br>
<br>
Added: llvm/trunk/test/Verifier/recursive-type-3.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Verifier/recursive-type-3.ll?rev=196626&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Verifier/recursive-type-3.ll?rev=196626&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/test/Verifier/recursive-type-3.ll (added)<br>
+++ llvm/trunk/test/Verifier/recursive-type-3.ll Fri Dec 6 18:13:34 2013<br>
@@ -0,0 +1,11 @@<br>
+; RUN: llvm-as %s -o /dev/null 2>&1<br>
+<br>
+%rt2 = type { i32, { i8, %rt2*, i8 }, i32 }<br>
+<br>
+define i32 @main() nounwind {<br>
+entry:<br>
+ ; Check that linked-list-style recursive types where the recursion is through<br>
+ ; a pointer of the type is valid for an alloca.<br>
+ %0 = alloca %rt2<br>
+ ret i32 0<br>
+}<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div>
</blockquote></div><br></div></div></div></div></blockquote></div><br></div>