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

[Michael Muller]

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
=============================================================================
-------------- 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;