[PATCH] Preliminary support for dynamically loadable coff objects

Lang Hames lhames at gmail.com
Thu Feb 26 16:28:52 PST 2015


Thanks very much for doing the refactor. This is looking great.

Are you still having trouble running rtdyld on your objects? Assuming you've got that working, please feel free to go ahead and commit.

If you're still having trouble let me know - I'm busy for the next few days, but later next week I will have time to test this out.


REPOSITORY
  rL LLVM

================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.cpp:94
@@ +93,3 @@
+  SectionEntry &Section = Sections[SectionID];
+  uint8_t *Target = Section.Address + Offset;
+  switch (RelType) {
----------------
andrew.w.kaylor wrote:
> AndyAyers wrote:
> > andrew.w.kaylor wrote:
> > > Shouldn't you be using Section.ObjAddress here?
> > You should get the same result either way, but I agree the intent might be clearer if we read from the original object instead of its loaded copy.
> > 
> > However once we implement the rel forms (which I am ending up doing as part of adding test cases) we need to know the actual address of the location to fix up; then it seems simpler to use that address to fetch any possible target displacement too.
> One of issues that ObjAddress is meant to address, and I agree you don't have this problem the way you implemented things here, is that after the relocation has been applied once you no longer have access to to original placeholder value by way of the loaded object.  If relocations are applied a second time the second pass can't rely on the loaded object having the correct placeholder value.
> 
> If I've got everything straight, your implementation here is grabbing the placeholder value and rolling it into the relocation entry, which is a decent solution.  So using Section.Address instead of Section.ObjAddress is really only very technically incorrect.  I would still advocate for the use of Section.ObjAddress for the sake of any future refactoring that is meant to address this class of behavior in some currently unanticipated way.
> 
> I know Lang wanted to do something to eliminate the need to keep the original object image around.  Are the "rel forms" related to that?  I don't think I'm familiar with that term.  Whether yes or no, how will the rel forms address this issue?
I definitely want to remove the need to keep the original object around, but I don't have a timeline for it yet. I think using ObjAddress is the appropriate solution for now.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.cpp:132
@@ +131,3 @@
+      if (SecI == Obj.section_end())
+        llvm_unreachable("Symbol section not found, bad object file");
+      bool IsCode = SecI->isText();
----------------
andrew.w.kaylor wrote:
> majnemer wrote:
> > Would `report_fatal_error` be more reasonable here?
> I know we're doing this all over the existing code, but RuntimeDyld should really avoid fatal errors, including llvm_unreachable, as much as possible.  With a static compiler it is kind of reasonable to bring down the program if a catastrophic error occurs, but RuntimeDyld really shouldn't do that.
> 
> I'm not suggesting that this problem needs to be addressed in this change set.  I'm just bringing it up as a general concern that needs to be dealt with sooner or later.
Yep - I don't mind this going in as is. I'm going to do a thorough audit for these in all the RuntimeDyld subclasses fairly soon. I can change it to something more reasonable then.

http://reviews.llvm.org/D7793

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list