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

[AIX] Fix XCOFF metadata #118905

Merged
merged 3 commits into from
Dec 18, 2023
Merged

[AIX] Fix XCOFF metadata #118905

merged 3 commits into from
Dec 18, 2023

Conversation

bzEq
Copy link
Contributor

@bzEq bzEq commented Dec 13, 2023

#118344 accidentally changed the way to get metadata from XCOFF file and broken our internal CI.

This PR reverts part of #118344 .

@rustbot
Copy link
Collaborator

rustbot commented Dec 13, 2023

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 13, 2023
@bzEq
Copy link
Contributor Author

bzEq commented Dec 13, 2023

BTW, we are seeking feedback on adding buildbot for AIX. See https://users.rust-lang.org/t/rfc-add-ci-builder-for-aix/103809/2. If we have CI for AIX, such errors can be avoided in the first place.

@bzEq bzEq changed the title Fix XCOFF metadata [AIX] Fix XCOFF metadata Dec 13, 2023
@WaffleLapkin
Copy link
Member

cc @saethlin

@workingjubilee
Copy link
Member

I believe the decision on whether and how to add AIX to CI is a matter for @rust-lang/infra.

saethlin
saethlin reviewed Dec 13, 2023
View reviewed changes
compiler/rustc_codegen_ssa/src/back/metadata.rs Outdated
@@ -158,12 +158,13 @@ pub(super) fn get_metadata_xcoff<'a>(path: &Path, data: &'a [u8]) -> Result<&'a
file.symbols().find(|sym| sym.name() == Ok(AIX_METADATA_SYMBOL_NAME))
{
let offset = metadata_symbol.address() as usize;
if offset < 8 {
// The offset specifies the location of rustc metadata in the .info section of XCOFF.
// Each string stored in .info section of XCOFF is preceded by a 4-byte lenght field.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Each string stored in .info section of XCOFF is preceded by a 4-byte lenght field.
// Each string stored in .info section of XCOFF is preceded by a 4-byte length field.

saethlin
saethlin reviewed Dec 13, 2023
View reviewed changes
compiler/rustc_codegen_ssa/src/back/metadata.rs Outdated
@@ -599,12 +600,12 @@ pub fn create_compressed_metadata_file_for_xcoff(
section: SymbolSection::Section(data_section),
flags: SymbolFlags::None,
});
let len = data.len() as u64;
let offset = file.append_section_data(section, &len.to_le_bytes(), 1);
let len = data.len() as u32;
Copy link
Member

@saethlin saethlin Dec 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The length of data here is not guaranteed to fit in a u32. Ideally we'd produce a compile error here, but it's not clear to me if we have access to a TyCtxt to do that. At the very least we should use .try_into().unwrap() to ICE early instead of producing a truncated metadata section.

Copy link
Member

Choose a reason for hiding this comment

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

At the very least there is a session:

rust/compiler/rustc_codegen_ssa/src/back/metadata.rs

Line 535 in 14e6f3f

return create_compressed_metadata_file_for_xcoff(file, &packed_metadata, symbol_name);

Copy link
Member

Choose a reason for hiding this comment

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

@bzEq even though we panic above, this should also be .try_into().unwrap()

@bzEq
Copy link
Contributor Author

bzEq commented Dec 14, 2023

@rustbot ready

WaffleLapkin
WaffleLapkin approved these changes Dec 17, 2023
View reviewed changes
Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

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

r=me with the nitpick

@bors delegate+

compiler/rustc_codegen_ssa/src/back/metadata.rs Outdated
@@ -599,12 +600,12 @@ pub fn create_compressed_metadata_file_for_xcoff(
section: SymbolSection::Section(data_section),
flags: SymbolFlags::None,
});
let len = data.len() as u64;
let offset = file.append_section_data(section, &len.to_le_bytes(), 1);
let len = data.len() as u32;
Copy link
Member

Choose a reason for hiding this comment

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

@bzEq even though we panic above, this should also be .try_into().unwrap()

@WaffleLapkin
Copy link
Member

I said @bors delegate+

@bors
Copy link
Contributor

bors commented Dec 17, 2023

✌️ @bzEq, you can now approve this pull request!

If @WaffleLapkin told you to "r=me" after making some further change, please make that change, then do @bors r=@WaffleLapkin

@WaffleLapkin WaffleLapkin added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 17, 2023
@bzEq
Copy link
Contributor Author

bzEq commented Dec 18, 2023

@bors r=@WaffleLapkin

@bors
Copy link
Contributor

bors commented Dec 18, 2023

📌 Commit a8e1da3 has been approved by WaffleLapkin

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 18, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 18, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 18, 2023
@bors
Auto merge of rust-lang#119069 - matthiaskrgr:rollup-xxk4m30, r=matth…
e004adb 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#118852 (coverage: Skip instrumenting a function if no spans were extracted from MIR)
 - rust-lang#118905 ([AIX] Fix XCOFF metadata)
 - rust-lang#118967 (Add better ICE messages for some undescriptive panics)
 - rust-lang#119051 (Replace `FileAllocationInfo` with `FileEndOfFileInfo`)
 - rust-lang#119059 (Deny `~const` trait bounds in inherent impl headers)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 18294d6 into rust-lang:master Dec 18, 2023
11 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 18, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 18, 2023
@rust-timer
Unrolled build for rust-lang#118905
a617504 
Rollup merge of rust-lang#118905 - bzEq:revert-u64-on-xcoff, r=WaffleLapkin

[AIX] Fix XCOFF metadata

rust-lang#118344  accidentally changed the way to get metadata from XCOFF file and broken our internal CI.

This PR reverts part of rust-lang#118344 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saethlin saethlin saethlin left review comments

@WaffleLapkin WaffleLapkin WaffleLapkin approved these changes

Assignees

@WaffleLapkin WaffleLapkin

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.76.0
Development

Successfully merging this pull request may close these issues.
6 participants
@bzEq @rustbot @WaffleLapkin @workingjubilee @bors @saethlin