[LLVMdev] Making LLVM safer in out-of-memory situations

[Gasiunas, Vaidas]

Hi Lang, Philip,

Thank you for comments! I tried to address them in the new version of the patch.

1) This line is indeed garbage. I removed it.

2) I extracted a reusable method computeSectionStubBufSize, which contains most of the repetition. In my code I also had 3 similar loops. I introduced the static function computeAllocationSizeForSections to reuse them.

3) I renamed all variables in my code to camel case. I also fixed the naming convention inconsistencies in RuntimeDyldImpl::loadObject. 

I also extended the comment for reserveAllocationSpace.

Best regards,
Vaidas

-----Original Message-----
From: Lang Hames [mailto:lhames at gmail.com] 
Sent: Dienstag, 11. Februar 2014 01:40
To: Philip Reames
Cc: Gasiunas, Vaidas; Kaylor, Andrew; llvm-commits at cs.uiuc.edu
Subject: Re: [LLVMdev] Making LLVM safer in out-of-memory situations

Hi Vaidas,

This looks good to me, but there are a couple of issues that would be
good to clear up before it goes in to the mainline:

1) On line 224 of the patch, the variable you have introduced is
unused. What was it intended for?

+        ObjSectionToIDMap::iterator i =
LocalSections.find(*RelocatedSection);

2) The new method you're introducing,
RuntimeDyldImpl::computeTotalAllocSize, contains a lot of copy-pasted
code from RuntimeDyldImpl::emitSection. I think there are some
opportunities to factor out common code there. In particular the stub
buffer size calculations. Is there a way you can pull that out into a
private convenience method/function?

3) Please update RuntimeDyldImpl::computeTotalAllocSize to use the
LLVM variable naming conventions. Variable names should be in camel
case, starting with a capital letter (see e.g. the variable names in
the emitSection method). I know there's a lot of code around that
doesn't  follow this standard, but we should try to make sure new code
does.

Apart from that, this looks good to me.

Cheers,
Lang.

On Fri, Jan 24, 2014 at 10:21 AM, Philip Reames
<listmail at philipreames.com> wrote:
> Minor comment:
> - The comment for reserveAllocationSpace should reference the enable
> required by needsToReserveAllocationSpace.
>
> Otherwise, looks fine to me.  (Pending Andy's OK.)
>
> Philip
-------------- next part --------------
A non-text attachment was scrubbed...
Name: computeTotalAllocSize3.patch
Type: application/octet-stream
Size: 22621 bytes
Desc: computeTotalAllocSize3.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140212/e5ae5398/attachment.obj>