Add GOFF read/write support - #969
Conversation
|
Thanks for the PR. I probably won't get to review this for a few days or more. You may have noticed I moved the GOFF support to an unstable feature in #962. My plan is to do an However, if there is anywhere that you think it would be beneficial to have a breaking change in the non-GOFF code in order to accommodate GOFF then now is the time to do it. I didn't notice anything from a quick look through this PR. It would be useful to have the fixtures in |
|
Thanks for your consideration
Sounds good, I'll make sure to update this PR to reflect any changes that are made if necessary inbetween this and the next release
Noted, I don't believe we'll require any breaking changes to satisfy our needs. I don't see any issue with leaving
I've opened a PR to add sample testfiles here gimli-rs/object-testfiles#45 . If that lands ahead of time, I'll update this PR to bump the submodule and add my read tests |
philipc
left a comment
There was a problem hiding this comment.
Some comments from a quick read through, I haven't looked at anything in detail yet.
Also, there doesn't appear to be any testing of continuation records, either here or in the object-testfiles additions.
GOFF is still new and foreign to me, so please push back if I make any suggestions that don't make sense to you.
| ObjectKind::Unknown | ||
| } | ||
|
|
||
| fn segments(&self) -> GoffSegmentIterator<'data, '_, R> { |
There was a problem hiding this comment.
The segments API in this crate is intended for executable files, so we probably shouldn't be returning any segments here.
There was a problem hiding this comment.
Okay, I switched GoffSegmentIterator::next() to always return None (same approach currently used in xcoff)
There was a problem hiding this comment.
The ObjectSegment implementation should be changed to unreachable!(); stubs too.
|
|
||
| let goffsymbol = GoffSymbol { | ||
| symbol_index: symbolindex, | ||
| esdid: esdid, |
There was a problem hiding this comment.
| esdid: esdid, | |
| esdid, |
There's a few other clippy warnings too.
There was a problem hiding this comment.
All clippy warnings warnings are fixed now
| } | ||
|
|
||
| #[test] | ||
| fn goff_relocation_multiple() { |
There was a problem hiding this comment.
It would be good to test continuation records for these too.
There was a problem hiding this comment.
I added a new test specifically for triggering continuation records for relocations here, and a small fix in the writer
There was a problem hiding this comment.
What testing have you done besides what is in this PR? I'm guessing the relocation writing code path isn't actually used for your purposes (which is fine, I just need know where things stand).
This PR adds read and write support for GOFF
Read Support
data()returns an error directing users touncompressed_data(), which assembles the complete section data from all contributing records.name()returns an error in favor ofname_parts(), which returns a vec of raw byte slices that can be assembled by the caller.RelocationFlags::Goff. GenericRelocationKindmapping is best-effort based on relocation flags.Write Support
Testing
tests/round_trip/goff.rsverify write/read consistency for sections, symbols, and basic relocations.tests/round_trip/goff_relocation.rsverify relocation parsing and reconstruction.Note: you can refer to the spec here IBM z/OS GOFF Specification