[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