[llvm-commits] [patch] Identifying code sections in MCJIT

[Kaylor, Andrew]

Hi Tim,

I'm not sure about this one.  I like your change, but I'm not sure it will work everywhere right now.

The MachO implementation of 'isText' (actually isSectionText by the time it gets resolved) checks to see if the section is named "__text" which probably isn't sufficient for the way you're using it here.  The ELF implementation checks section flags to see if the section is executable.

I'm not familiar enough with MachO to know how big the risk is here or how difficult it would be to correctly implement the isText function there.  I would expect that in most cases your patch would work for MachO.  It might not work in all cases, but apparently the existing code doesn't work in all cases either.

I also notice that there are some cases where findOrEmitSection is being called with the IsCode argument hard-coded to 'true' and one other place where it is already doing what your patch does.

I'd like to hear from someone who is more familiar with MachO before we proceed, but I think your patch is a step in the right direction.

-Andy

-----Original Message-----
From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Tim Northover
Sent: Tuesday, December 11, 2012 5:23 AM
To: llvm-commits
Subject: [llvm-commits] [patch] Identifying code sections in MCJIT

Hi,

After investigating why adding mapping symbols on ARM (r169609) caused segfaults in the MCJIT, I discovered that the .text section wasn't being allocated as code in the MemoryManager, and hence wasn't being marked executable before jumping there.

This was because RuntimeDyldImpl::loadObject was choosing the type based on whether the first symbol it encountered in that section was ST_Function or not. Previously it was, but now the mapping symbols (required to have STT_NOTYPE) are seen first and .text is misclassified.

This patch instead uses the SectionRef method "isText" to make that decision. It parallels what RuntimeDyldELF does while processing DWARF, and would seem to be a more sensible API anyway.

Is this correct for MachO too, and is the patch OK to commit?

Cheers.

Tim.