serge1 / ELFIO

ELFIO - ELF (Executable and Linkable Format) reader and producer implemented as a header only C++ library
http://serge1.github.io/ELFIO
MIT License
729 stars 158 forks source link

New function to remove sections from ELF file #61

Closed MartinCon closed 3 years ago

MartinCon commented 3 years ago

This PR adds a new function for removing sections from an ELF file.

I'm aware that this is a rather exotic use case and not as universally useful as the rest of ELFIO.

But I'm still offering it to be merged. If you don't want to have this functionality, I'm fine keeping it on my own fork (where it was living for a long time already).

MartinCon commented 3 years ago

This PullRequest is not ready for merge yet. It is intended to give a chance for early feedback.

I will further polish the code and add tests on real ELF files

MartinCon commented 3 years ago

@serge1 : I'm unsure where to place the operator<< for range. Shall I put it into elfio_range.hpp, to have to close to the range class, or elfio_dump, because that's where output code is located. What's your preference?

Please also note that I used C++ 14 code and updated the build configuration accordingly.

serge1 commented 3 years ago

Hi @MartinCon ,

Thank you for working on improving ELFIO library!

I'm aware that this is a rather exotic use case and not as universally useful as the rest of ELFIO

The use case is indeed exotic. ELFIO historically had complications with read/modify/write scenarious. Any step in direction of improving this scenario is welcomed. And the exercise in this PR is good, at least, as a demonstration.


1.

Please also note that I used C++ 14 code and updated the build configuration accordingly

I think it is OK for ELFIO testing and example compilation. But, I am aware about some users, usually on embedded systems, who still use old GCC versions. It would be great if you find a way to implement conditional compilation for the new stuff according to __STDC_VERSION__ macro. Thus, C++11 users will not get the new functionality, but, still will be able to use the existing API

When the number of such 'branches' will be bigger, version 4 of the library can be created

2.

Please try to avoid usage of exceptions. There were no exception handling mechanism in ELFIO yet. For example, you can use 3,4-tuple return code containing a clarification message and/or error code.

3.

For consistency, please don't use Camel convention var names like "sectionsToRemove" and "allSections"

4.

I'm unsure where to place the operator<< for range

There is already usage of "ostream" in elfio.hpp, so, no need to separate the operator from the "range" class.

5.

Please use set_data() instead of std::memcpy()


I didn't check the business logic, but, hope that tests will cover the correctness.

I will further polish the code and add tests on real ELF files

Thanks again!

Serge

serge1 commented 3 years ago

Hi @MartinCon,

Are you going to finish this PR?

Thank you

MartinCon commented 3 years ago

Hi @serge1 Yes, I do intend do finish it and incorporate all your suggestions. But it got buried in my backlog by other more important tasks, so it may be some time until I come back to this topic.

serge1 commented 3 years ago

Please resubmit when relevant. Thank you