Fix for ExecutionEngine buglet

[Kaylor, Andrew]

I think the early return is preferable.  It doesn't look like the variables that get updated without the early return are used for anything other than statistic tracking, but they would be inaccurate in that case.

Thanks,
Andy

-----Original Message-----
From: Dale E. Martin [mailto:dale at the-martins.org] 
Sent: Wednesday, November 13, 2013 11:38 AM
To: llvm-commits at cs.uiuc.edu
Cc: Kaylor, Andrew
Subject: Re: Fix for ExecutionEngine buglet

> I assume from the description that you are using either the 
> interpreter or the old JIT execution engine.  If you are using the old 
> JIT, have you considered migrating to MCJIT?  That's not relevant to 
> the patch, but I think it's a good idea.  We're planning to deprecate 
> the old JIT engine in a future release.
 
My project is currently using LLVM 3.2 but I've been reading the blog entries about MCJIT and it's on my radar.

> As for the patch itself, it seems like it would be better to have the 
> function return immediately if GA is NULL after the call to 
> getMemoryForGV.  We definitely don't want to increment the number of 
> init bytes or the number of global variables in that case.

I can certainly update the patch to do an early return.  I don't know that in the big picture it's any different but it certainly looks cleaner.

> It would be nice if we had some way to let the caller know the method 
> failed.  Even better would be to figure out why the allocation is 
> failing and fix it.

I do believe that the issue is that we have generated a lot of code and data, and suddenly we're asking for a large global variable and the currently allocated buffer is basically at the end.  The memory allocator says "we don't have space for that" and returns NULL.  Since there is current no check for the allocation failing, when it tries to initialize this new global variable we try memcpy the initializer into address 0x0.

With this current patch, when that segv is avoided, that situation gets recognized in the loop one level higher in the execution engine, so the code generation fails, more memory gets allocated, and the code generation succeeds on the next pass through.

> I'm assuming based on the fact that total program failure is avoidable 
> that we aren't really out of memory.

Agreed.  I'm pretty new to LLVM but my reading of the comments in JITCodeEmiter.h led me to believe that this is the strategy - allocate a certain amount of memory, try to generate code, if it's not enough, fail, allocate more, and try again.  I was assume that I had found a small hole where the "did we run of out of memory?" check wasn't happening in time.

> That said, I wouldn't object to a patch that simply avoids 
> catastrophic failure at this point if that's all you have time for and 
> it is sufficient for your current needs.

It does seem to fix my current issue, but I'm willing to update the patch to do a "full" early return if that's preferable.

Thanks,
  Dale
--
Dale E. Martin - dale at the-martins.org
http://the-martins.org/~dmartin