New functions to read elf files and parse modules (SP3) #155
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
softwarecki
requested review from
lgirdwood,
aborisovich,
abonislawski,
kv2019i,
marc-hb,
marcinszkudlinski,
kfrydryx,
pblaszko and
pjdobrowolski
|
I don't understand, there are |
Probably not as simple to use elfutils for the required usage and nothing wrong with C here. |
It's however a legitimate question to ask where this code comes from and how it was validated, no? Please tell me it didn't come from ChatGPT :-) |
This was referenced Apr 10, 2023
|
looks legit for me. I put tactical dot to watch for more changes. |
marc-hb
added a commit
to marc-hb/rimage
that referenced
this pull request
Apr 18, 2023
The deleted "install" target copied only the rimage executable and left the config files behind. This gave the wrong impression that the executable is useful without config files. "Installing" also gave the wrong impression that rimage versions are somewhat stable and relatively independent of SOF versions: they're not. In fact there is no such thing as an rimage "version": everyone should always use the exact rimage _git commit_ from the west manifest or git submodule. There are no rimage "releases" and no semantic versioning or anything like it, rimage is effectively part of the SOF source and build and run directly from its build directory by practically every SOF developer and SOF CI thanks to sof/src/arch/xtensa/CMakeLists.txt#ExternalProject_Add(rimage_ep ... sof/scripts/xtensa-build-zephyr.py#def build_rimage() sof/zephyr/CMakeLists.txt#set(RIMAGE_CONFIG_PATH ... etc. Providing an "install" target greatly increases the chances of different people and CIs using different rimage versions which is the last thing we want considering the many significant rimage changes happening right now, examples: - zephyrproject-rtos/zephyr#56099 - zephyrproject-rtos/zephyr#54700 - thesofproject/sof#7270 - thesofproject/sof#7304 - thesofproject/sof#7320 - thesofproject#155 While it's useful for multiple files (e.g.: config files), a CMake target was always overkill to copy a single file. Someone or some script who really wants to copy the rimage binary to some other place that the build directory for some (discouraged) reason _can still do so_ with a much more basic, simpler and more transparent file copy command. Finally, the default "bin" DESTINATION required root access which is otherwise totally unncessary to build SOF. Signed-off-by: Marc Herbert <[email protected]>
marc-hb
added a commit
to marc-hb/rimage
that referenced
this pull request
Apr 18, 2023
The deleted "install" target copied only the rimage executable and left the config files behind. This gave the wrong impression that the executable is useful without config files. "Installing" also gave the wrong impression that rimage versions are somewhat stable and relatively independent of SOF versions: they're not. In fact there is no such thing as an rimage "version": everyone should always use the exact rimage _git commit_ from the west manifest or git submodule. There are no rimage "releases" and no semantic versioning or anything like it, rimage is effectively part of the SOF source and build and run directly from its build directory by practically every SOF developer and SOF CI thanks to sof/src/arch/xtensa/CMakeLists.txt#ExternalProject_Add(rimage_ep ... sof/scripts/xtensa-build-zephyr.py#def build_rimage() sof/zephyr/CMakeLists.txt#set(RIMAGE_CONFIG_PATH ... etc. Providing an "install" target greatly increases the chances of different people and CIs using different rimage versions which is the last thing we want considering the many significant rimage changes happening right now, examples: - zephyrproject-rtos/zephyr#56099 - zephyrproject-rtos/zephyr#54700 - thesofproject/sof#7270 - thesofproject/sof#7304 - thesofproject/sof#7320 - thesofproject#155 While it's useful for multiple files (e.g.: config files), a CMake target was always overkill to copy a single file. Someone or some script who really wants to copy the rimage binary to some other place that the build directory for some (discouraged) reason _can still do so_ with a much more basic, simpler and more transparent file copy command. Finally, the default "bin" DESTINATION required root access which is otherwise totally unncessary to build SOF. Signed-off-by: Marc Herbert <[email protected]>
kv2019i
pushed a commit
that referenced
this pull request
Apr 26, 2023
The deleted "install" target copied only the rimage executable and left the config files behind. This gave the wrong impression that the executable is useful without config files. "Installing" also gave the wrong impression that rimage versions are somewhat stable and relatively independent of SOF versions: they're not. In fact there is no such thing as an rimage "version": everyone should always use the exact rimage _git commit_ from the west manifest or git submodule. There are no rimage "releases" and no semantic versioning or anything like it, rimage is effectively part of the SOF source and build and run directly from its build directory by practically every SOF developer and SOF CI thanks to sof/src/arch/xtensa/CMakeLists.txt#ExternalProject_Add(rimage_ep ... sof/scripts/xtensa-build-zephyr.py#def build_rimage() sof/zephyr/CMakeLists.txt#set(RIMAGE_CONFIG_PATH ... etc. Providing an "install" target greatly increases the chances of different people and CIs using different rimage versions which is the last thing we want considering the many significant rimage changes happening right now, examples: - zephyrproject-rtos/zephyr#56099 - zephyrproject-rtos/zephyr#54700 - thesofproject/sof#7270 - thesofproject/sof#7304 - thesofproject/sof#7320 - #155 While it's useful for multiple files (e.g.: config files), a CMake target was always overkill to copy a single file. Someone or some script who really wants to copy the rimage binary to some other place that the build directory for some (discouraged) reason _can still do so_ with a much more basic, simpler and more transparent file copy command. Finally, the default "bin" DESTINATION required root access which is otherwise totally unncessary to build SOF. Signed-off-by: Marc Herbert <[email protected]>
|
@softwarecki any update ? |
|
@lgirdwood I'll try to finish work on this this PR soon. |
|
Why parsing binaries in C in 2023 is (at best) a waste of time: |
A structure was created to represent a file, a section and a strings section. Created a new functions to read a elf file, retrieve a section header based on index or name, read a section contents based on a header or name, retrieve section name and functions that print file, section and program headers. Signed-off-by: Adrian Warecki <[email protected]>
|
@plbossart: The code is the result of human work and was not created by chatGPT. The latter has the ability to give elaborate answers without any substance and sometimes has hallucinations. To perform a validation, I created a PR in SOF thesofproject/sof#7413. @lgirdwood I think its ready now :) |
lgirdwood
previously approved these changes
Sep 22, 2023
|
@kv2019i fyi. |
Ping, @softwarecki please explain how is this not re-inventing https://www.freshports.org/devel/elfutils/ |
A structure was created to represent the module, module section and section informations. Added a set of new functions for parsing a module and reading its contents. Signed-off-by: Adrian Warecki <[email protected]>
Moved the module information to the new module structure. Used new functions to parse module. Signed-off-by: Adrian Warecki <[email protected]>
Removed elf.c file that is no longer in use. Signed-off-by: Adrian Warecki <[email protected]>
|
@marc-hb I concluded that it would be better to write a few simple functions for reading files than add a dependency to another (quite large) library. If this is a problem for you, you can use |
lgirdwood
approved these changes
Sep 25, 2023
kv2019i
approved these changes
Sep 25, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rewritten elf file reading and module parsing:
informations. Added a set of new functions for parsing a module and reading its contents.
Moved the module information to the new module structure. Removed elf.c file that is no longer in use.
SOF CI PR thesofproject/sof#7413