[cfe-commits] Patch for "Throwing exceptions from temporary object's destructor" issue

[John McCall]

On Sep 12, 2012, at 4:40 AM, SENTHIL KUMAR THANGAVELU wrote:
> Hello all, John McCall,
>     I have attached a patch for the issue we discussed previously "http://clang-developers.42468.n3.nabble.com/Throwing-exceptions-from-temporary-object-s-destructor-td4026498.html" specifically targetting only init-temp1.C(from gcc test suite) . Tested the patch on X86-linux and ARM-linux ran clang regression, all test cases passed except CodeGenCXX/temporaries.cpp.
>    The issue was due to same landingpad being used for constructor and destructor of the same object and the parent auto var decl is not cleaned up(destructor called) when the destructor threw an exception. This patch creates a new landing pad containing a call to parent auto var decl's destructor. Once this destructor is called all other cleanup items are threaded through.
>    I met with regression failure for the case CodeGenCXX/temporaries.cpp. I have reduced the test case only to contain the failure portion attached in email(temporaries.test.cpp). There is some pattern which the test case expects and the fix changes the pattern so the test case fails. I have attached the .ll file generated with and without the patch. Can someone suggest how to modify the test case? also I would like to know the rational required to modify the test case. The 2 ".ll" files were generated with the patch on 3.1 branch.  Please let me know your comments.
>  
> Description of attachments:
> 1) patch.p0.txt -  patch based on svn revision 163683
> 2) temporaries.test.cpp - reduced test case from CodeGenCXX/temporaries.cpp containing only the snippet causing failure.  If this snippet is removed from original test case, test passes.
> 3) temporaries.base.ll -  clang output without my changes for temporaries.test.cpp
> 4) temporaries.withchanges.ll - clang output with my changes for temporaries.test.cpp
> 5) init-temp1.C - test case from gcc test suite

Index: lib/CodeGen/CGException.cpp
===================================================================
--- lib/CodeGen/CGException.cpp	(revision 163683)
+++ lib/CodeGen/CGException.cpp	(working copy)
@@ -745,6 +745,50 @@
 };
 const CleanupHackLevel_t CleanupHackLevel = CHL_MandatoryCleanup;
 
+// Create a new landing pad for tmp objects destructor's unwind
+// To be used only by auto var decl parent creating tmp objs
+llvm::BasicBlock *CodeGenFunction::EmitAutoVarDeclCleanupLpad() {

This function is unnecessary (and quite buggy, but fortunately being wrong is
adequate).  You should just push an EH cleanup during the emission of a
normal cleanup in which there's an auto var emission active.

+  AutoVarEmission CurAutoVarEmission; // tmp obj eh_cleanup will need

Comments on instance variables should be documentation comments
(i.e. should start with ///) and should actually document the variable.
For example, saying *why* EH cleanups need this information is a good
start (as a hint, they don't — it's *normal* cleanups that need it).

Also, please don't randomly copy the AutoVarEmission.  You need an
llvm::Value * (the address of the object) and a QualType.

   AutoVarEmission EmitAutoVarAlloca(const VarDecl &var);
+  bool ShouldConsiderParentVarDecCleanUp();

This method is just a buggier and less general version of
  Variable && needsEHCleanup(Variable->getType()->isDestructedType()).

John.