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.

Sign up for GitHub

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

Jump to bottom

write/xcoff: add support. #482

Merged
merged 15 commits into from
Mar 27, 2023
Merged

write/xcoff: add support. #482

merged 15 commits into from
Mar 27, 2023

Conversation

EsmeYi
Copy link
Contributor

@EsmeYi EsmeYi commented Nov 7, 2022

No description provided.

philipc
philipc requested changes Nov 8, 2022
View reviewed changes
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.

There's a few things that need fixing. I think there will also be more that needs fixing once you are actually using this. Have you used this to write any object files other than the one written by the test added in this PR?

src/write/xcoff.rs Outdated Show resolved Hide resolved
src/write/xcoff.rs Outdated Show resolved Hide resolved
src/write/xcoff.rs Show resolved Hide resolved
src/write/xcoff.rs Outdated Show resolved Hide resolved
src/write/xcoff.rs Outdated Show resolved Hide resolved
src/write/xcoff.rs Outdated Show resolved Hide resolved
src/write/xcoff.rs Outdated Show resolved Hide resolved
src/write/xcoff.rs Outdated Show resolved Hide resolved
src/write/xcoff.rs Outdated Show resolved Hide resolved
src/write/xcoff.rs Outdated Show resolved Hide resolved
@EsmeYi
Copy link
Contributor Author

EsmeYi commented Nov 9, 2022

Convert this PR to draft for now, since this PR will rebase on #484.

@EsmeYi EsmeYi marked this pull request as draft November 9, 2022 08:42
philipc
philipc reviewed Dec 27, 2022
View reviewed changes
src/write/xcoff.rs Outdated Show resolved Hide resolved
src/write/xcoff.rs Outdated Show resolved Hide resolved
philipc
philipc requested changes Dec 27, 2022
View reviewed changes
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.

Is this still a draft?

src/write/xcoff.rs Outdated Show resolved Hide resolved
src/write/xcoff.rs Outdated Show resolved Hide resolved
@EsmeYi EsmeYi marked this pull request as ready for review January 5, 2023 05:01
philipc
philipc reviewed Jan 5, 2023
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
@EsmeYi
Copy link
Contributor Author

EsmeYi commented Feb 2, 2023

@philipc Thanks a lot for your comment and I apologize for my late reply as I was on vacation last month. I just post a PR rust-lang/rust#107583 to support metadata on AIX, which is related to this PR, specifically the C_INFO symbol here.

philipc
philipc requested changes Feb 3, 2023
View reviewed changes
src/write/xcoff.rs Outdated Show resolved Hide resolved
src/write/xcoff.rs Outdated Show resolved Hide resolved
src/write/xcoff.rs Outdated Show resolved Hide resolved
philipc
philipc reviewed Feb 8, 2023
View reviewed changes
src/common.rs Outdated Show resolved Hide resolved
@philipc
Copy link
Contributor

philipc commented Feb 23, 2023

I think this is good to merge once CI is passing, so please continue working on fixes for that.

@EsmeYi
Copy link
Contributor Author

EsmeYi commented Feb 23, 2023

Thank you for your review. Checks are clean now.

@bzEq
Copy link

bzEq commented Mar 14, 2023

Hi @philipc , what abstraction level do we expect from object crate? Is it user's responsibility to make the final object file legal?
For example, in XCOFF, there is a .info section, AIX toolchain expects the data inside this section is a sequence of string whose first 4 bytes is the length of this string. When user appends data to this section, should user be aware of this knowledge?

@philipc
Copy link
Contributor

philipc commented Mar 14, 2023

The object crate will never be able to ensure that the final object is 100% legal. We rely on the user to have knowledge about the file formats and what they require. For the .info section specifically, the object crate will never validate the data that the user appends to the section. We could add a helper function that appends data in the correct format, but we wouldn't prevent the user from appending invalid data using other functions.

bzEq
bzEq reviewed Mar 20, 2023
View reviewed changes
src/write/xcoff.rs Show resolved Hide resolved
src/write/xcoff.rs Outdated Show resolved Hide resolved
philipc
philipc approved these changes Mar 23, 2023
View reviewed changes
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! I'll merge this soon. Before doing a release I will also do a bit more validation myself, and complete some other unrelated work.

@bzEq
Copy link

bzEq commented Mar 23, 2023

Thanks @philipc , I'll also validate this PR via bootstrapping rustc on AIX. I'll get it done as soon as possible.

bzEq
bzEq reviewed Mar 23, 2023
View reviewed changes
src/write/xcoff.rs Outdated
}
}
} else {
return Err(Error(format!(
Copy link

Choose a reason for hiding this comment

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

There is situation we have a symbol with kind SymbolSection::Undefined, we will set n_scnum to N_UNDEF in line 378. This situation should be valid in XCOFF's view.

bzEq
bzEq approved these changes Mar 24, 2023
View reviewed changes
Copy link

@bzEq bzEq left a comment

Choose a reason for hiding this comment

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

I've verify the PR via bootstrapping rustc 1.70.0-dev on AIX. Current implementation looks good to me.

@philipc philipc merged commit b348128 into gimli-rs:master Mar 27, 2023
philipc
philipc reviewed Apr 12, 2023
View reviewed changes
src/read/traits.rs
@@ -119,6 +119,21 @@ pub trait Object<'data: 'file, 'file>: read::private::Sealed {
/// Returns an error if the index is invalid.
fn symbol_by_index(&'file self, index: SymbolIndex) -> Result<Self::Symbol>;

/// Get the address of symbol named `symbol_name`, if such a symbol exists.
fn symbol_address_by_name(&'file self, symbol_name: &str) -> Result<u64> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I overlooked the addition of this when reviewing. It didn't really belong in this PR, and I'm not confident that this is an API we want to support because it seems too specialized to me, so I'm going to remove it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it seems quite specific, however the function is going to be called in https://github.com/rust-lang/rust/pull/107583/files#diff-6bbf6edd930f26d3b180eb5c250b0e48d8f3c5eb474e3274909ef6ae6f0d1e61R109

Copy link

@bzEq bzEq Apr 12, 2023 •
edited

Choose a reason for hiding this comment

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

Since rust-lang/rust#107583 is not landed yet, we can still look for a solution without these new APIs. And as I asked in #482 (comment) and @philipc answered in #482 (comment), we can still use existing APIs and our knowledge of XCOFF to get the work done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have no objection to removing this function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philipc philipc philipc approved these changes

@bzEq bzEq bzEq approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@EsmeYi @philipc @bzEq