Elf gnu symbol versioning

[johannst]

GNU symbol version extension for ELF files (#263).

WIP PR to discuss overall design.

Tasks:

• [x] Parse .gnu.version_r section
• [x] Parse .gnu.version_d section
• [x] Parse .gnu.version section
• [x] Test parsing .gnu.version_r section
• [x] Test parsing .gnu.version_d section
• [x] Test parsing .gnu.version section

[johannst]

First implementation done, any feedback is welcome :]

[johannst]

Thanks for the quick review, I'll take a look at the details after work.

[johannst]

I was thinking, with the primitives available for reading, to also add some utility to lookup version strings. For example something like Versym -> Option<&str>. @philipc what do you think about that? Or should we target higher-level utilities in a new issue/PR?

[philipc]

It's certainly useful to have something like that, but it's a bit harder to be sure what the correct API is, so maybe leave it for a new PR, but doing a prototype (and using it for a real task) wouldn't hurt.

[johannst]

It's certainly useful to have something like that, but it's a bit harder to be sure what the correct API is, so maybe leave it for a new PR, but doing a prototype (and using it for a real task) wouldn't hurt.

In that sense the PR is done from my side.

The only question is if there is a better way to handle the test ELF binaries? I optimized them for size but if you have an idea I would prefer to not commit them here? Building during test execution doesn't seem to be a good idea due to the windows runners. Maybe we could maintain another repository with all the binaries and include it submodule here? But that would also be a task for another issue/PR.

[johannst]

@m4b any further feedback on style or completeness, please let me know.

[m4b]

@johannst sorry for delay, and thank you for your patience; going to give this one last review, but it looks good to me; if i don't merge by tomorrow, feel free to ping.

There is not breaking changes yes? (I can release this in a minor release once it's merged if you like.

[johannst]

Don't worry, that's fine, you can take your time to review it.

Actually I have one concrete question about the cfg guard. Other modules wrap the implementation in a macro, but as far as I can tell, rustfmt doesn't format this code. But to align on style, would you prefer to use a macro in the symver module as well?

There is not breaking changes yes? (I can release this in a minor release once it's merged if you like.

Right, it is compatible on the API level.

[m4b]

Yea I believe we've had issues in past with rustfmt and the macro stuff, it's quite annoying.

So I believe the original reason for the macro was because it put the cfg on every item, which too burdensome manually (and error prone). If the module has some entire feature applicable to it, you can just put it on the module; if there is some subset of features though, like alloc then probably macro is necessary.

I'm not sure what your cfg needs are, need to do a closer review to see what you mean (or you could summarize what you're thinking if you like too :) )

[johannst]

I'm not sure what your cfg needs are, need to do a closer review to see what you mean (or you could summarize what you're thinking if you like too :) )

As is today, to really use the features of the symver module we require (elf32 || elf64) && alloc. Due to the rustfmt issues with the macros I implemented the cfg guard with a nested impl module (see below). But you are right, in this case (where the cfg applies globally to the module) it would have been better to directly put it on the module in src/elf/mod.rs.

#[cfg(all(any(feature = "elf32", feature = "elf64"), feature = "alloc"))]
pub use symver_impl::*;

#[cfg(all(any(feature = "elf32", feature = "elf64"), feature = "alloc"))]
mod symver_impl {
     ...
}

However if we decide that is it valuable to offer the ELF type definitions in non-alloc environments then I need to put the cfg guards on the items in the module itself.

So I believe the original reason for the macro was because it put the cfg on every item, which too burdensome manually (and error prone).

Definitely agree, putting it by hand on every item is quiet error prone. If I have some time I can check why rustftmt has a problem with the macros as we use them.

[johannst]

Thanks for your review. I agree, I think it's in a good state to merge now.

[m4b]

Thank you for your patience and this awesome PR!

[m4b]

So I squashed your commits, and because some of your commit messages were quite good and looks like you took time to write them, I chose the best ones and included that in the main body; thanks again! I'll have a minor release soon, since this is non-breaking, though I may want to roll up other changes if possible, not sure; is this urgently needed for you?

[johannst]

Thanks for the effort to extract some of the commit messages.

I'll have a minor release soon, since this is non-breaking, though I may want to roll up other changes if possible, not sure; is this urgently needed for you?

That's fine for me, I mainly used this features in some experiments for now where I can use a version from gh.

[messense]

I'd love to have a new release to land https://github.com/PyO3/maturin/pull/626, thanks!

[m4b]

ok 0.4.3 is out; i trimmed the include field too to remove tests, which includes binary files, etc., so the crates.io download should be much slimmer too!