[llvm] r208257 - [RuntimeDyld] Make RuntimeDyldImpl::resolveExternalSymbols preserve the
Lang Hames
lhames at gmail.com
Thu May 8 11:44:21 PDT 2014
Hi Andy,
That all makes perfect sense. I've suggested to the LLDB folks that they
call getSymbolAddress rather than getGlobalValueAddress - if they can avoid
finalizing before they move the sections their issue should just go away.
Longer term, we do want to support recompilation of modules, but I see now
why simply reapplying the relocations to the existing (finalized) sections
is a non-starter. I just had to personally go through the process of
shooting myself in the foot to appreciate it.
Thanks, as always, for all the help. :)
Cheers,
Lang.
On Thu, May 8, 2014 at 10:01 AM, Kaylor, Andrew <andrew.kaylor at intel.com>wrote:
> I changed the model that MCJIT uses for module finalization when I added
> the multiple module support because it ended up being too complicated to
> expect clients to manage it. Requiring explicit finalization was really
> never a good model anyway. I tried to keep clients using the older model
> working, but I marked a few methods as deprecated. LLDB is still using the
> older model.
>
>
>
> As I recall, MCJIT::getGlobalValueAddress implicitly finalizes the loaded
> modules because of it is being used to get the address of a function, the
> function needs to be executable when the pointer is returned. I believe
> this was needed to support the behavior of existing clients. I haven’t
> looked at the LLDB code in a while, but I’m guessing that this issue is
> caused by the way that they are doing their own pointer replacement instead
> of trusting the relocation mechanism.
>
>
>
> MCJIT has a method, getSymbolAddress, which will return an address without
> forcing finalization, but it isn’t currently exposed through the
> ExecutionEngine interface. RuntimeDyld, in its public interface, exposes
> both getSymbolAddress, which returns the address of a symbol in local
> memory (which I’m guessing is what LLDB actually wants) and
> getSymbolLoadAddress, which returns the address that a symbol will have in
> the address space in which it will be executed. Of course, the fact that
> it is in this interface and not MCJIT’s interface reflects the fact that we
> typically don’t want MCJIT clients mucking around with RuntimeDyld
> directly. To put a finer point on it, we don’t want MCJIT clients trying
> to do the kinds of things LLDB is doing.
>
>
>
> I suspect that the best path forward here is to bring LLDB up-to-date with
> the latest MCJIT interface and only change MCJIT if some legitimate general
> use case is uncovered in the process of bringing LLDB up to date.
>
>
>
> -Andy
>
>
>
>
>
> *From:* Lang Hames [mailto:lhames at gmail.com]
> *Sent:* Wednesday, May 07, 2014 11:26 PM
> *To:* Jim Grosbach
> *Cc:* Kaylor, Andrew; llvm-commits at cs.uiuc.edu
>
> *Subject:* Re: [llvm] r208257 - [RuntimeDyld] Make
> RuntimeDyldImpl::resolveExternalSymbols preserve the
>
>
>
> Hi Andy,
>
>
>
> Looking at the LLDB code that led me to this issue, I see it's not
> actually calling finalize directly, it's actually calling
> MCJIT::getGlobalValueAddress, but that method automatically calls
> finalizeLoadedModules if a value is found. I wasn't expecting that - why
> should asking for a global's address should force finalization?
>
>
>
> The crashes on the bots appears to be due to exactly the issue Andy
> raised, which I hadn't groked previously - we don't currently have any
> option for 'de-finalising' a section so that relocations can be re-applied,
> so this patch will cause crashes on any client that is actually marking the
> memory read-only. Obviously I'll hold off recommitting until I've got a
> solution for that. :)
>
>
>
> Cheers,
>
> Lang.
>
>
>
> On Wed, May 7, 2014 at 5:38 PM, Jim Grosbach <grosbach at apple.com> wrote:
>
> FWIW, support for recompilation of modules was a conscious intention of
> the design originally in order to support tiering up levels of
> optimization. The implementation since then has obviously diverged from
> that a bit, but it’s still a relevant design goal.
>
>
>
> You make a good point on the section permissions. Supporting recompilation
> is more than just relocation handling and would require nontrivial juggling
> in a memory manager to do effectively.
>
>
>
> -Jim
>
>
>
> On May 7, 2014, at 4:43 PM, Kaylor, Andrew <andrew.kaylor at intel.com>
> wrote:
>
>
>
> I’m not sure I’d agree that MCJIT is supposed to support recompilation
> of a module. I believe we’ve explicitly said that you can’t modify a
> module after it has been added to MCJIT, so why would you recompile it? In
> any event, if you do recompile a module, the relocation information will be
> recreated.
>
>
>
> But I take it that you’re actually addressing some case where a module is
> moved after it has been compiled and put into an executable state. I don’t
> understand this case either. Why would you move a module after it has been
> made executable?
>
>
>
> Also, I would expect that you’ll run into problems with memory managers
> that set the permissions for code sections to RX after a module has been
> finalized. In those cases, if you try to reapply relocations (presumably
> to a module that didn’t move) it will cause a protection fault.
> Conversely, if you allow a module to be moved after it has been finalized,
> don’t you run into issues with the client having to keep track of whether
> or not the module is in an executable state?
>
>
>
> -Andy
>
>
>
> *From:* Lang Hames [mailto:lhames at gmail.com <lhames at gmail.com>]
> *Sent:* Wednesday, May 07, 2014 4:09 PM
> *To:* Kaylor, Andrew
> *Cc:* llvm-commits at cs.uiuc.edu
> *Subject:* Re: [llvm] r208257 - [RuntimeDyld] Make
> RuntimeDyldImpl::resolveExternalSymbols preserve the
>
>
>
> Hi Andy,
>
>
>
> MCJIT is supposed to support recompilation of a module, and linking
> multiple modules (thanks for making the latter work! :). So we need to keep
> external relocations around in case the module they point to is recompiled.
>
>
>
> Cheers,
>
> Lang.
>
>
>
> On Wed, May 7, 2014 at 3:56 PM, Kaylor, Andrew <andrew.kaylor at intel.com>
> wrote:
>
> The discarding of processed relocations was an intentional choice. The
> relocation map can take up a pretty fair amount of memory, particularly in
> the case where you have a large number of modules that reference one
> another. Keeping the relocation map around will cause memory bloat issues
> for some clients.
>
> My thinking was that once a module was finalized you can't remap its
> sections anymore. Do you have a use case where this is an unacceptable
> limitation?
>
> -Andy
>
>
> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:
> llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Lang Hames
> Sent: Wednesday, May 07, 2014 3:34 PM
> To: llvm-commits at cs.uiuc.edu
> Subject: [llvm] r208257 - [RuntimeDyld] Make
> RuntimeDyldImpl::resolveExternalSymbols preserve the
>
> Author: lhames
> Date: Wed May 7 17:34:08 2014
> New Revision: 208257
>
> URL: http://llvm.org/viewvc/llvm-project?rev=208257&view=rev
> Log:
> [RuntimeDyld] Make RuntimeDyldImpl::resolveExternalSymbols preserve the
> relocation entries it applies.
>
> Prior to this patch, RuntimeDyldImpl::resolveExternalSymbols discarded
> relocations for external symbols once they had been applied. This causes
> issues if the client calls MCJIT::finalizeLoadedModules more than once, and
> updates the location of any symbols in between (e.g. by calling
> MCJIT::mapSectionAddress).
>
> No test case yet: None of our in-tree memory managers support moving
> sections around. I'll have to hack up a dummy memory manager before I can
> write a unit test.
>
> Fixes <rdar://problem/16764378>
>
>
> Modified:
> llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
>
> Modified: llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp?rev=208257&r1=208256&r2=208257&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp (original)
> +++ llvm/trunk/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp Wed May
>
> +++ 7 17:34:08 2014
>
> @@ -620,6 +620,8 @@ void RuntimeDyldImpl::resolveRelocationL
> }
>
> void RuntimeDyldImpl::resolveExternalSymbols() {
> + StringMap<RelocationList> ProcessedSymbols;
> +
> while (!ExternalSymbolRelocations.empty()) {
> StringMap<RelocationList>::iterator i =
> ExternalSymbolRelocations.begin();
>
> @@ -665,8 +667,20 @@ void RuntimeDyldImpl::resolveExternalSym
> resolveRelocationList(Relocs, Addr);
> }
>
> + ProcessedSymbols[i->first()] = i->second;
> ExternalSymbolRelocations.erase(i);
> }
> +
> + // Restore the relocation entries that were consumed in the loop above:
> + //
> + // FIXME: Replace the following loop with:
> + // std::swap(ProcessedSymbols, ExternalSymbolRelocations)
> + // once StringMap has copy and move construction.
> + for (StringMap<RelocationList>::iterator I = ProcessedSymbols.begin(),
> + E = ProcessedSymbols.end();
> + I != E; ++I) {
> + ExternalSymbolRelocations[I->first()] = I->second; }
> }
>
>
> //===----------------------------------------------------------------------===//
>
>
> _______________________________________________
> 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
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140508/03d00123/attachment.html>
More information about the llvm-commits
mailing list