<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jun 23, 2015, at 9:59 AM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" class="">dexonsmith@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class="Apple-interchange-newline">On 2015-Jun-22, at 17:53, Pete Cooper <<a href="mailto:peter_cooper@apple.com" class="">peter_cooper@apple.com</a>> wrote:<br class=""><br class="">So back to 0002.<br class=""><br class="">You were worried about the lack of protection to ensure that a subclass of Constant actually implements destroyConstant. Unfortunately I couldn’t find a good way to check for this statically, but I did go ahead and implement the alternative you suggested. For context, what follows (indented) was the proposal which i’ve basically just implemented exactly as described. It seems to be a very good way to handle this, and isn’t much more code churn.<br class=""><br class="">However, I realize there's already something called<br class="">`Constant::destroyConstantImpl()`, which has a `delete this` inside it<br class="">(ironic, given that the context for this patch is removing the vtable<br class="">that the `delete` call relies on).<br class=""><br class="">I haven't looked at all the patches in this series yet, but I feel like<br class="">there ought to be some way of clarifying `Constant` destruction<br class="">immediately. My shot-from-the-hip is something like the following:<br class=""><br class=""> class Constant {<br class=""> public:<br class=""> void destroyConstant();<br class=""> };<br class=""><br class=""> class SomeConstant : public Constant {<br class=""> friend class Base; // For fooImpl().<br class=""><br class=""> /// Destroy and delete the constant.<br class=""> void destroyConstantImpl();<br class=""> ~SomeConstant();<br class=""><br class=""> // Don't provide destroyConstant().<br class=""> };<br class=""><br class=""> void Constant::destroyConstant() {<br class=""> // Remove lingering references from the constant pool (move from<br class=""> // old `Constant::destroyConstantImpl()`).<br class=""> while (!use_empty()) {<br class=""> // ...<br class=""> }<br class=""><br class=""> // Dispatch to subclass to cleanup and delete.<br class=""> switch (...) {<br class=""> default:<br class=""> llvm_unreachable(...);<br class=""> // Compile error if there's an unhandled case instead of<br class=""> // infinite recursion.<br class=""> #define HANDLE_CONSTANT(NAME) \<br class=""> case NAME ## Kind: \<br class=""> cast<NAME>(this)->destroyConstantImpl(); \<br class=""> break;<br class=""> }<br class=""><br class=""> // When we drop virtual dispatch for the destructor, move the<br class=""> // delete call inside the switch statement above.<br class=""> delete this;<br class=""> }<br class=""><br class=""> void SomeConstant::destroyConstantImpl() {<br class=""> assert(use_empty() && ...);<br class=""> getContext()->SomeConstantPool.erase(this);<br class=""> }<br class=""><br class="">This inverts the destroyConstant/Impl relationship.<br class=""><br class="">Maybe this leaves out some case, or doesn't quite fit with the end goal<br class="">(you've thought about this more than I have). My main point is, with<br class="">static dispatch we can easily catch the "missing destroyConstant()<br class="">implementation" at compile-time, and we should.<br class=""><br class="">Cheers,<br class="">Pete<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">LGTM with a little nitpicking.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">commit faa9ec17ac16b8b0c4f22c16d353e0ee13590253<br class="">Author: Peter Cooper <<a href="mailto:peter_cooper@apple.com" class="">peter_cooper@apple.com</a>><br class="">Date: Mon Jun 15 13:04:29 2015 -0700<br class=""><br class=""> Devirtualize Constant::destroyConstant.<br class=""><br class=""> This reorganizes destroyConstant and destroyConstantImpl.<br class=""><br class=""> Now there is only destroyConstant in Constant itself, while<br class=""> subclasses are required to implement destroyConstantImpl.<br class=""><br class=""> destroyConstantImpl no longer calls delete but is instead only<br class=""> responsible for removing the constant from any maps in which it<br class=""> is contained.<br class=""><br class="">diff --git a/lib/IR/Constants.cpp b/lib/IR/Constants.cpp<br class="">index 76c55b6..773c829 100644<br class="">--- a/lib/IR/Constants.cpp<br class="">+++ b/lib/IR/Constants.cpp<br class="">@@ -276,35 +276,6 @@ Constant *Constant::getAggregateElement(Constant *Elt) const {<br class=""> return nullptr;<br class="">}<br class=""><br class="">-<br class="">-void Constant::destroyConstantImpl() {<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Can you move `Constant::destroyConstant()` up to here to minimize the</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">diff? Alternatively, if there's some reason to move the code, please do</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">it in a separate NFC commit.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote>Moved it up there. Thanks for pointing this one out, was no reason to move it.<br class=""><blockquote type="cite" class=""><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">- // When a Constant is destroyed, there may be lingering<br class="">- // references to the constant by other constants in the constant pool. These<br class="">- // constants are implicitly dependent on the module that is being deleted,<br class="">- // but they don't know that. Because we only find out when the CPV is<br class="">- // deleted, we must now notify all of our users (that should only be<br class="">- // Constants) that they are, in fact, invalid now and should be deleted.<br class="">- //<br class="">- while (!use_empty()) {<br class="">- Value *V = user_back();<br class="">-#ifndef NDEBUG // Only in -g mode...<br class="">- if (!isa<Constant>(V)) {<br class="">- dbgs() << "While deleting: " << *this<br class="">- << "\n\nUse still stuck around after Def is destroyed: "<br class="">- << *V << "\n\n";<br class="">- }<br class="">-#endif<br class="">- assert(isa<Constant>(V) && "References remain to Constant being destroyed");<br class="">- cast<Constant>(V)->destroyConstant();<br class="">-<br class="">- // The constant should remove itself from our use list...<br class="">- assert((use_empty() || user_back() != V) && "Constant not removed!");<br class="">- }<br class="">-<br class="">- // Value has no outstanding references it is safe to delete it now...<br class="">- delete this;<br class="">-}<br class="">-<br class="">static bool canTrapImpl(const Constant *C,<br class=""> SmallPtrSetImpl<const ConstantExpr *> &NonTrappingOps) {<br class=""> assert(C->getType()->isFirstClassType() && "Cannot evaluate aggregate vals!");<br class="">@@ -1432,6 +1409,45 @@ const APInt &Constant::getUniqueInteger() const {<br class=""> return cast<ConstantInt>(C)->getValue();<br class="">}<br class=""><br class="">+void Constant::destroyConstant() {<br class="">+<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">This newline is strange.</span></div></blockquote>Yes, yes it is, and now gone.</div><div><br class=""></div><div>Thanks for the review. Committed as r240471.</div><div><br class=""></div><div>Pete<br class=""><blockquote type="cite" class=""><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">+ /// First call destroyConstantImpl on the subclass. This gives the subclass<br class="">+ /// a chance to remove the constant from any maps/pools it's contained in.<br class="">+ switch (getValueID()) {<br class="">+ default:<br class="">+ llvm_unreachable("Not a constant!");<br class="">+#define HANDLE_CONSTANT(Name) \<br class="">+ case Value::Name##Val: \<br class="">+ return cast<Name>(this)->destroyConstantImpl();<br class="">+#include "llvm/IR/Value.def"<br class="">+ }<br class="">+<br class="">+ // When a Constant is destroyed, there may be lingering<br class="">+ // references to the constant by other constants in the constant pool. These<br class="">+ // constants are implicitly dependent on the module that is being deleted,<br class="">+ // but they don't know that. Because we only find out when the CPV is<br class="">+ // deleted, we must now notify all of our users (that should only be<br class="">+ // Constants) that they are, in fact, invalid now and should be deleted.<br class="">+ //<br class="">+ while (!use_empty()) {<br class="">+ Value *V = user_back();<br class="">+#ifndef NDEBUG // Only in -g mode...<br class="">+ if (!isa<Constant>(V)) {<br class="">+ dbgs() << "While deleting: " << *this<br class="">+ << "\n\nUse still stuck around after Def is destroyed: " << *V<br class="">+ << "\n\n";<br class="">+ }<br class="">+#endif<br class="">+ assert(isa<Constant>(V) && "References remain to Constant being destroyed");<br class="">+ cast<Constant>(V)->destroyConstant();<br class="">+<br class="">+ // The constant should remove itself from our use list...<br class="">+ assert((use_empty() || user_back() != V) && "Constant not removed!");<br class="">+ }<br class="">+<br class="">+ // Value has no outstanding references it is safe to delete it now...<br class="">+ delete this;<br class="">+}<br class=""><br class="">//---- ConstantPointerNull::get() implementation.<br class="">//</blockquote></div></blockquote></div><br class=""></body></html>