conversion from ElfFile to ObjectFile #141
Conversation
|
this is totally not ready (it breaks existing unit tests, there are no unit tests for this conversion ...) |
|
ElfFile to ObjectFile conversion works now. (all tests pass) |
LukeGrainwalker
force-pushed
the
linkelf
branch
from
3d70415 to
f453769
Compare
ppci/binutils/objectfile.py
Outdated
| def load(input_file): | ||
| """Load object file from file""" | ||
| return deserialize(json.load(input_file)) | ||
| # if is_json(input_file): |
There was a problem hiding this comment.
This commented code can go.
ppci/binutils/objectfile.py
Outdated
|
|
||
| start = input_file.read(4) | ||
| input_file.seek(0) | ||
| if b"ELF" in start: |
There was a problem hiding this comment.
I would rather not include ELF file specific logic in this objectfile class. If we want to make the switch between ELF file and json object file, it should be outside the objectfile.load function.
ppci/binutils/objectfile.py
Outdated
| start = input_file.read(4) | ||
| input_file.seek(0) | ||
| if b"ELF" in start: | ||
| return deserialize(ElfFile.load(input_file)) |
There was a problem hiding this comment.
Allthough it is a nifty idea to re-use the deserialize function together with getitem calls, I would rather refactor this code such that there is a function introduced called: elf_to_object, which takes an ELF file, and creates an objectfile instance of this elf file.
There was a problem hiding this comment.
the elf directory has a read file for reading elf files (currently just a wrapper for the load function) we could put this function into there into there
There was a problem hiding this comment.
i would also add, that the refactoring would also need to change all the places like the linker and objdump/copy ....
There was a problem hiding this comment.
Why would we need to change the objdump / copy code? If we first convert the ELF file into generic objectfile, then objdump can operate on the ObjectFile class.
|
|
||
| @classmethod | ||
| def _missing_(cls, value): | ||
| return cls.NULL |
There was a problem hiding this comment.
Is using NULL when we encounter an unknown section type ok? It would also be fine to me to raise an error in this case.
There was a problem hiding this comment.
it broke when loading bash, because it uses special gnu section types which we can't map to this enum
There was a problem hiding this comment.
Can we add the gnu section types to the enum? Or are there too many?
There was a problem hiding this comment.
i don't know i can't seam to find any documentation (like official ones) about these.
ppci/format/elf/headers.py
Outdated
|
|
||
| if bits == 64: | ||
| self.RelocationTableEntryWA = header.mk_header( | ||
| "RelocationTableEntryWA", |
There was a problem hiding this comment.
WA == with addend? Could we think of better naming here? I see in ELF terminology there is REL and RELA, maybe we can turn the two options in RelTableEntry and RelaTableEntry, or even RelEntry and RelaEntry? What do you think?
There was a problem hiding this comment.
that's a good idea the WA was just a quick fix.
ppci/format/elf/file.py
Outdated
| def __init__(self, header): | ||
| self.header = header | ||
|
|
||
| def __getitem__(self, key): |
There was a problem hiding this comment.
Rather than making the conversion here to objectfile, I would make this conversion in a separate function.
| def read_symbol_table(self, f): | ||
| return [s.header for s in self.symbole_table] | ||
|
|
||
| def get_str(self, offset, *strtab): |
There was a problem hiding this comment.
Hmm, I see the purpose of passing the strtab along here. Maybe we could introduce a StringTable class, which has the get_str method? Then we can modify read_strtab such that it is cached, using functools.cache decorate. read_strtab should then return a new StringTable class.
|
The refactoring seams to be done now. |
LukeGrainwalker
force-pushed
the
linkelf
branch
from
d612a21 to
246d862
Compare
2 participants
This enables the conversion from ElfFile to ObjectFile which then can be used to link against elf files.