[PATCH] D14674: [RuntimeDyld] Add accessors to `SectionEntry`; NFC

Andy Kaylor via llvm-commits llvm-commits at lists.llvm.org
Fri Nov 20 15:15:18 PST 2015


andrew.w.kaylor added a comment.

It seems like "Section.getAddress() + Offset" shows up pretty frequently.  Do you think it's worth making a function to do that too?

Otherwise, this all looks pretty straightforward.  All of my comments are about cleaning up things that exist in the current code.


================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp:866
@@ -864,3 +865,3 @@
   if (I != ObjSecToIDMap.end()) {
-//    llvm::dbgs() << "Found ID " << I->second << " for Sec: " << Sec.getRawDataRefImpl() << ", LoadAddress = " << RTDyld.Sections[I->second].LoadAddress << "\n";
-    return RTDyld.Sections[I->second].LoadAddress;
+    //    llvm::dbgs() << "Found ID " << I->second << " for Sec: " <<
+    //    Sec.getRawDataRefImpl() << ", LoadAddress = " <<
----------------
Is there a reason to keep the commented out debug code here?  It should either be put inside a DEBUG() wrapper or deleted.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp:442
@@ -440,3 +441,3 @@
   // TODO: Add Thumb relocations.
-  uint32_t *TargetPtr = (uint32_t *)(Section.Address + Offset);
-  uint32_t FinalAddress = ((Section.LoadAddress + Offset) & 0xFFFFFFFF);
+  uint32_t *TargetPtr = (uint32_t *)(Section.getAddress() + Offset);
+  uint32_t FinalAddress = ((Section.getLoadAddress() + Offset) & 0xFFFFFFFF);
----------------
I know you didn't introduce them, but it would be nice to get rid of the C-style casts while you're making these changes.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h:293
@@ -277,3 +292,3 @@
   uint8_t *getSectionAddress(unsigned SectionID) const {
-    return (uint8_t *)Sections[SectionID].Address;
+    return (uint8_t *)Sections[SectionID].getAddress();
   }
----------------
This cast is unnecessary.

================
Comment at: lib/ExecutionEngine/RuntimeDyld/RuntimeDyldMachO.cpp:303
@@ -303,3 +302,3 @@
 
-    unsigned char *P = EHFrame->Address;
-    unsigned char *End = P + EHFrame->Size;
+    unsigned char *P = EHFrame->getAddress();
+    unsigned char *End = P + EHFrame->getSize();
----------------
These variables should be changed to be uint8_t* and the processFDE function should be updated similarly.  I know it's equivalent, but it would be better to be consistent.


http://reviews.llvm.org/D14674





More information about the llvm-commits mailing list