Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

read/xcoff: initial support. #469

Merged
merged 12 commits into from
Oct 19, 2022
Merged

read/xcoff: initial support. #469

merged 12 commits into from
Oct 19, 2022

Conversation

EsmeYi
Copy link
Contributor

@EsmeYi EsmeYi commented Sep 29, 2022

This patch adds a skeleton for reading XCOFF, which now can parses FileHeader/Sections/Symbols.
Comments are welcome.

src/read/xcoff/file.rs Outdated Show resolved Hide resolved
src/read/xcoff/file.rs Show resolved Hide resolved
src/xcoff.rs Show resolved Hide resolved
@philipc
Copy link
Contributor

philipc commented Oct 10, 2022

As part of this, can you add some simple test files to https://github.com/gimli-rs/object-testfiles, and add tests here for the objdump example output. We'll also want readobj support and associated test too, but that's fine to leave for another PR.

Copy link
Contributor

@philipc philipc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a high level, this looks great, and thanks for working on it. I'll wait until you've finished before doing a detailed review.

src/common.rs Outdated Show resolved Hide resolved
2. Fix some unimplemented issues.
@EsmeYi EsmeYi marked this pull request as ready for review October 11, 2022 16:44
@EsmeYi
Copy link
Contributor Author

EsmeYi commented Oct 11, 2022

The PR is ready for review now and the format will be clean in next commit.
@philipc @ecnelises Thanks for your review and look forward to your more comments.
The resource of test files are added to gimli-rs/object-testfiles#4.

@EsmeYi EsmeYi requested review from philipc and ecnelises and removed request for philipc and ecnelises October 11, 2022 16:47
@EsmeYi EsmeYi requested review from ecnelises and philipc and removed request for philipc and ecnelises October 11, 2022 16:54
@philipc
Copy link
Contributor

philipc commented Oct 13, 2022

Please add a command to build with only the xcoff format feature in CI:

- run: cargo build --no-default-features --features read_core,write_core,coff
- run: cargo build --no-default-features --features read_core,write_core,elf
- run: cargo build --no-default-features --features read_core,write_core,macho
- run: cargo build --no-default-features --features read_core,pe
- run: cargo build --no-default-features --features read_core,wasm

Copy link
Contributor

@philipc philipc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's okay if you want to leave some of this work for future PRs, but please add TODO comments to indicate that support is missing.

src/read/xcoff/file.rs Outdated Show resolved Hide resolved
src/read/xcoff/file.rs Show resolved Hide resolved
src/read/xcoff/file.rs Show resolved Hide resolved
src/read/xcoff/file.rs Outdated Show resolved Hide resolved
src/read/xcoff/file.rs Show resolved Hide resolved
src/read/xcoff/symbol.rs Outdated Show resolved Hide resolved
src/read/xcoff/symbol.rs Outdated Show resolved Hide resolved
src/read/xcoff/symbol.rs Outdated Show resolved Hide resolved
src/read/xcoff/symbol.rs Outdated Show resolved Hide resolved
src/read/xcoff/symbol.rs Outdated Show resolved Hide resolved
Add TODO comments.
src/read/xcoff/section.rs Outdated Show resolved Hide resolved
src/read/xcoff/symbol.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@philipc philipc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few small things to fix and then I think this is ready to merge.

src/read/xcoff/file.rs Outdated Show resolved Hide resolved
src/read/xcoff/symbol.rs Outdated Show resolved Hide resolved
src/read/xcoff/symbol.rs Outdated Show resolved Hide resolved
src/read/xcoff/symbol.rs Outdated Show resolved Hide resolved
@philipc
Copy link
Contributor

philipc commented Oct 19, 2022

You'll also need to add the 32-bit files in gimli-rs/object-testfiles#4 and wait for it to be merged so that this PR can update it.

@EsmeYi
Copy link
Contributor Author

EsmeYi commented Oct 19, 2022

Addressed comments.
@philipc Thank you for your patient review.

@philipc
Copy link
Contributor

philipc commented Oct 19, 2022

Can you include testfiles in the PR as well?

@EsmeYi EsmeYi requested a review from philipc October 19, 2022 05:12
Copy link
Contributor

@philipc philipc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@philipc philipc merged commit 5514d1b into gimli-rs:master Oct 19, 2022
@EsmeYi EsmeYi deleted the read_xcoff branch October 19, 2022 08:32
mcbegamerxx954 pushed a commit to mcbegamerxx954/object that referenced this pull request Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants