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

[Lang Hames]

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
>
>
> On 1/24/14 5:23 AM, Gasiunas, Vaidas wrote:
>
> Hi Andy,
>
> Here is a new version of the patch:
> - I made the computation optional by introducing a virtual method
> needsToReserveAllocationSpace in the memory manager, so that the allocation
> sizes are precomputed only if this method returns true. So now there
> shouldn't be any computational overhead for the users that do not need that
> functionality.
> - The memory to be allocated is classified into 3 categories, depending on
> required permissions (RX, RO, RW). So the memory manger receives now three
> numbers.
> - I fixed allocation type of the sections, emitted in the last loop of
> RuntimeDyld::loadObject. Only .text sections are allocated as code sections
> now.
>
> Regards,
> Vaidas
>
> -----Original Message-----
> From: Kaylor, Andrew [mailto:andrew.kaylor at intel.com]
> Sent: Mittwoch, 22. Januar 2014 18:22
> To: Gasiunas, Vaidas
> Subject: RE: [LLVMdev] Making LLVM safer in out-of-memory situations
>
> Yeah, that sounds like a bug.  I remember there being a place where we
> didn't check to see if a section was code or data.  I think at some time in
> the history of the code the information wasn't available where that
> happened.  That's probably not true now.
>
> -Andy
>
> -----Original Message-----
> From: Gasiunas, Vaidas [mailto:vaidas.gasiunas at sap.com]
> Sent: Wednesday, January 22, 2014 1:01 AM
> To: Kaylor, Andrew
> Subject: RE: [LLVMdev] Making LLVM safer in out-of-memory situations
>
> Hi Andy,
>
> OK, this is basically the way I implemented it, but the unit test fails
> because of the .eh_frame section, because it is loaded as a code section.
> This section is loaded in the last loop of RuntimeDyldImpl::loadObject,
> which loads all sections with relocations if there were not loaded yet. It
> loads all sections as code sections without differentiating. Should I fix
> that?
>
> Regards,
> Vaidas
>
>
> -----Original Message-----
> From: Kaylor, Andrew [mailto:andrew.kaylor at intel.com]
> Sent: Dienstag, 21. Januar 2014 18:29
> To: Gasiunas, Vaidas
> Subject: RE: [LLVMdev] Making LLVM safer in out-of-memory situations
>
> Hi Vaidas,
>
> For ELF files, there is a flag (SHF_EXECINSTR) that indicates whether or not
> the section needs to be executable.  For LLVM purposes, however, I think you
> should use ObjectFile::IsSectionText() (or SectionRef::IsText()).  Both of
> these use the SHF_EXECINSTR flag rather than the section name.  The MachO
> implementation uses a similar flag (S_ATTR_PURE_INSTRUCTIONS).
>
> -Andy
>
> -----Original Message-----
> From: Gasiunas, Vaidas [mailto:vaidas.gasiunas at sap.com]
> Sent: Tuesday, January 21, 2014 8:07 AM
> To: Kaylor, Andrew; llvm-commits at cs.uiuc.edu
> Subject: RE: [LLVMdev] Making LLVM safer in out-of-memory situations
>
> Hi Andy,
>
> I am trying to categorize the allocation size by memory permission types
> (RX, RW, RO). I have been assuming that .text sections are the only kind of
> sections that need to be executable, but in the unit test example
> (MCJITCAPITest.cpp) not only the .text section but also the .eh_frame
> section is loaded as a code section. Is that an intended behavior? If yes,
> what would be then the most reliable criteria to decide whether a section is
> a code section?
>
> Cheers,
> Vaidas
>
> -----Original Message-----
> From: Kaylor, Andrew [mailto:andrew.kaylor at intel.com]
> Sent: Dienstag, 14. Januar 2014 19:34
> To: Gasiunas, Vaidas; llvm-commits at cs.uiuc.edu
> Cc: LLVM Dev; Philip Reames
> Subject: RE: [LLVMdev] Making LLVM safer in out-of-memory situations
>
> Hi Vaidas,
>
> I've got a few of comments about this patch.
>
> First, I think the calculation needs to be optional and it shouldn't happen
> by default.  I think most clients are relatively flexible about the total
> size and wouldn't want the extra time hit needed to calculate the size in
> advance.
>
> Second, if the calculation is being done, it should probably track size by
> expected section permissions.  For instance, a host-based memory manager is
> likely to want to know the total size of RX, RW and RO memory it will need
> to allocate so that sections with common permissions can be grouped onto the
> same page(s).
>
> Third, it looks like there are some slow calculations that will be done
> twice with this patch.  In particular, the stub size calculation (which
> isn't even needed in many circumstances).  We should avoid that extra work.
>
> I understand that these issues aren't necessarily your concern, but I think
> these things would need to be addressed in order for this to be merged into
> LLVM trunk.  If you don't have the time to make these changes, maybe someone
> else will be able to pick up the patch and add these changes.
>
> Thanks for the patch!
>
> -Andy
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>