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

[Lang Hames]

Excellent!

Thank you very much for all your work on this Vaidas. I've committed
it in r201259.

Cheers,
Lang.

On Wed, Feb 12, 2014 at 8:40 AM, Gasiunas, Vaidas
<vaidas.gasiunas at sap.com> wrote:
> 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