llvm dev - Sep 2012 - [LLVMdev] [llvm-commits] Fwd: Re: [PATCH] Fix for bug in JIT exception table allocation

Pinging again, more loudly :-)

Michael Muller wrote:
> 
> Ping.  (looking for someone to review or commit this, thanks)
> 
> Michael Muller wrote:
> > 
> > Thanks, Duncan -
> > 
> > > Hi Michael,
> > > 
> > > > --- lib/ExecutionEngine/JIT/JITEmitter.cpp  (revision
163478)
> > > > +++ lib/ExecutionEngine/JIT/JITEmitter.cpp  (working copy)
> > > > @@ -974,14 +974,24 @@
> > > >      SavedBufferBegin = BufferBegin;
> > > >      SavedBufferEnd = BufferEnd;
> > > >      SavedCurBufferPtr = CurBufferPtr;
> > > > +    uint8_t *FrameRegister;
> > > >
> > > > -    BufferBegin = CurBufferPtr > > > >
MemMgr->startExceptionTable(F.getFunction(),
> > > > -                                                           
ActualSize);
> > > > -    BufferEnd = BufferBegin+ActualSize;
> > > > -    EmittedFunctions[F.getFunction()].ExceptionTable =
BufferBegin;
> > > > -    uint8_t *EhStart;
> > > > -    uint8_t *FrameRegister = DE->EmitDwarfTable(F,
*this, FnStart, FnEnd,
> > > > -                                                EhStart);
> > > > +    while (true) {
> > > > +      BufferBegin = CurBufferPtr > > > >
MemMgr->startExceptionTable(F.getFunction(),
> > > > +
> > > > ActualSize);
> > > > +      BufferEnd = BufferBegin+ActualSize;
> > > > +      EmittedFunctions[F.getFunction()].ExceptionTable =
BufferBegin;
> > > > +      uint8_t *EhStart;
> > > > +      FrameRegister = DE->EmitDwarfTable(F, *this,
FnStart, FnEnd,
> > > > EhStart);
> > > > +
> > > > +      // If we've run out of memory, try again with
twice as much space.
> > > > +      if (CurBufferPtr == BufferEnd) {
> > > > +        ActualSize = (CurBufferPtr-BufferBegin)*2;
> > > > +        MemMgr->deallocateExceptionTable(BufferBegin);
> > > > +      } else {
> > > > +        break;
> > > > +      }
> > > 
> > > here I think it would be more conformant with LLVM style to do:
> > > 
> > >         // If the buffer was large enough to hold the table then
we are
> > >         done.
> > >         if (CurBufferPtr != BufferEnd)
> > >           break;
> > > 
> > >         // Try again with twice as much space.
> > >         ActualSize = (CurBufferPtr-BufferBegin)*2;
> > >         MemMgr->deallocateExceptionTable(BufferBegin);
> > 
> > Done.  New patch attached.
> > 
> > > 
> > > Otherwise looks good to me, though that's not saying much
since I don't know
> > > this part of LLVM.
> > 
> > 
> > It looks like that code segment has been touched by a number of
people:
> > 
> > 
> >   147615     rafael   if (JITExceptionHandling) {
> >    49924   geoffray     uintptr_t ActualSize = 0;
> >    91464   jyasskin     SavedBufferBegin = BufferBegin;
> >    91464   jyasskin     SavedBufferEnd = BufferEnd;
> >    91464   jyasskin     SavedCurBufferPtr = CurBufferPtr;
> >    82418        rnk 
> >    47079   geoffray     BufferBegin = CurBufferPtr > >  
MemMgr->startExceptionTable(F.getFunction(),
> >    47079   geoffray
> >   ActualSize);
> >    47079   geoffray     BufferEnd = BufferBegin+ActualSize;
> >    84651   jyasskin    
EmittedFunctions[F.getFunction()].ExceptionTable > >   BufferBegin;
> >    82418        rnk     uint8_t *EhStart;
> >    82418        rnk     uint8_t *FrameRegister =
DE->EmitDwarfTable(F, *this,
> >   FnStart, FnEnd,
> >    82418        rnk                                                
EhStart);
> >    48019    lattner     MemMgr->endExceptionTable(F.getFunction(),
BufferBegin,
> >   CurBufferPtr,
> >    48019    lattner                               FrameRegister);
> >    91464   jyasskin     BufferBegin = SavedBufferBegin;
> >    91464   jyasskin     BufferEnd = SavedBufferEnd;
> >    91464   jyasskin     CurBufferPtr = SavedCurBufferPtr;
> >    47079   geoffray 
> >   102865   baldrick     if (JITExceptionHandling) {
> >   127047   echristo       TheJIT->RegisterTable(F.getFunction(),
FrameRegister);
> >    82418        rnk     }
> >    47079   geoffray   }
> > 
> > Any suggestions as to who would be the best qualified to look at it?
> > 
> > 
> > > 
> > > Ciao, Duncan.
> > > 
> > > > +    }
> > > >      MemMgr->endExceptionTable(F.getFunction(),
BufferBegin, CurBufferPtr,
> > > >                                FrameRegister);
> > > >      BufferBegin = SavedBufferBegin;
> > 
> > 
> >
============================================================================>
> michaelMuller = mmuller at enduden.com | http://www.mindhog.net/~mmuller
> >
-----------------------------------------------------------------------------
> > Government is not reason, it is not eloquence; it is force.  Like
fire, it
> > is a dangerous servant and a fearsome master.  - George Washington
> >
============================================================================>
> 
>
============================================================================>
michaelMuller = mmuller at enduden.com | http://www.mindhog.net/~mmuller
>
-----------------------------------------------------------------------------
> Government is not reason, it is not eloquence; it is force.  Like fire, it
> is a dangerous servant and a fearsome master.  - George Washington
>
============================================================================>

============================================================================michaelMuller
= mmuller at enduden.com | http://www.mindhog.net/~mmuller
-----------------------------------------------------------------------------
Lokah Samasta Sukhino Bhavantu - May all beings everywhere be happy and free.
And may my own thoughts and actions contribute to that happiness and freedom.
============================================================================--------------
next part --------------
Index: unittests/ExecutionEngine/JIT/JITTest.cpp
==================================================================---
unittests/ExecutionEngine/JIT/JITTest.cpp	(revision 163809)
+++ unittests/ExecutionEngine/JIT/JITTest.cpp	(working copy)
@@ -203,14 +203,21 @@
 
 class JITTest : public testing::Test {
  protected:
+  virtual RecordingJITMemoryManager* CreateMemoryManager() {
+    return new RecordingJITMemoryManager;
+  }
+
   virtual void SetUp() {
     M = new Module("<main>", Context);
-    RJMM = new RecordingJITMemoryManager;
+    RJMM = CreateMemoryManager();
     RJMM->setPoisonMemory(true);
     std::string Error;
+    TargetOptions Options;
+    Options.JITExceptionHandling = true;
     TheJIT.reset(EngineBuilder(M).setEngineKind(EngineKind::JIT)
                  .setJITMemoryManager(RJMM)
-                 .setErrorStr(&Error).create());
+                 .setErrorStr(&Error)
+                 .setTargetOptions(Options).create());
     ASSERT_TRUE(TheJIT.get() != NULL) << Error;
   }
 
@@ -292,6 +299,46 @@
   EXPECT_EQ(3, *GPtr);
 }
 
+// Regression test for a bug.  The JITEmitter wasn't checking to verify
that
+// it hadn't run out of space while generating the DWARF exception
information
+// for an emitted function.
+
+class ExceptionMemoryManagerMock : public RecordingJITMemoryManager {
+ public:
+  virtual uint8_t* startExceptionTable(const Function* F,
+                                       uintptr_t &ActualSize) {
+    // force an insufficient size the first time through.
+    bool ChangeActualSize = false;
+    if (ActualSize == 0)
+      ChangeActualSize = true;;
+    uint8_t* result +      RecordingJITMemoryManager::startExceptionTable(F,
ActualSize);
+    if (ChangeActualSize)
+      ActualSize = 1;
+    return result;
+  }
+};
+
+class JITExceptionMemoryTest : public JITTest {
+ protected:
+  virtual RecordingJITMemoryManager* CreateMemoryManager() {
+    return new ExceptionMemoryManagerMock;
+  }
+};
+
+TEST_F(JITExceptionMemoryTest, ExceptionTableOverflow) {
+  Function *F = Function::Create(TypeBuilder<void(void),
false>::get(Context),
+                                 Function::ExternalLinkage,
+                                 "func1", M);
+  BasicBlock *Block = BasicBlock::Create(Context, "block", F);
+  IRBuilder<> Builder(Block);
+  Builder.CreateRetVoid();
+  TheJIT->getPointerToFunction(F);
+  ASSERT_TRUE(RJMM->startExceptionTableCalls.size() == 2);
+  ASSERT_TRUE(RJMM->deallocateExceptionTableCalls.size() == 1);
+  ASSERT_TRUE(RJMM->endExceptionTableCalls.size() == 1);
+}
+
 int PlusOne(int arg) {
   return arg + 1;
 }
Index: lib/ExecutionEngine/JIT/JITEmitter.cpp
==================================================================---
lib/ExecutionEngine/JIT/JITEmitter.cpp	(revision 163809)
+++ lib/ExecutionEngine/JIT/JITEmitter.cpp	(working copy)
@@ -974,14 +974,24 @@
     SavedBufferBegin = BufferBegin;
     SavedBufferEnd = BufferEnd;
     SavedCurBufferPtr = CurBufferPtr;
+    uint8_t *FrameRegister;
 
-    BufferBegin = CurBufferPtr =
MemMgr->startExceptionTable(F.getFunction(),
-                                                             ActualSize);
-    BufferEnd = BufferBegin+ActualSize;
-    EmittedFunctions[F.getFunction()].ExceptionTable = BufferBegin;
-    uint8_t *EhStart;
-    uint8_t *FrameRegister = DE->EmitDwarfTable(F, *this, FnStart, FnEnd,
-                                                EhStart);
+    while (true) {
+      BufferBegin = CurBufferPtr =
MemMgr->startExceptionTable(F.getFunction(),
+                                                              ActualSize);
+      BufferEnd = BufferBegin+ActualSize;
+      EmittedFunctions[F.getFunction()].ExceptionTable = BufferBegin;
+      uint8_t *EhStart;
+      FrameRegister = DE->EmitDwarfTable(F, *this, FnStart, FnEnd, EhStart);
+
+      // If the buffer was large enough to hold the table then we are done.
+      if (CurBufferPtr != BufferEnd)
+        break;
+
+      // Try again with twice as much space.
+      ActualSize = (CurBufferPtr-BufferBegin)*2;
+      MemMgr->deallocateExceptionTable(BufferBegin);
+    }
     MemMgr->endExceptionTable(F.getFunction(), BufferBegin, CurBufferPtr,
                               FrameRegister);
     BufferBegin = SavedBufferBegin;