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

cranelift-object: Make sections read only by default #5619

Merged
merged 1 commit into from
Feb 18, 2023

Conversation

afonso360
Copy link
Contributor

This changes the default section type to be ReadOnlyDataWithRel instead of Data.

On COFF the CRT initializers do not run unless their section is read only. (See: rust-lang/rustc_codegen_cranelift#1249 (comment))

The new SectionKind makes these sections read only for COFF and MachO, but leaves it as Writable as required by ELF.

cc: @bjorn3

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:module labels Jan 23, 2023
@cfallin
Copy link
Member

cfallin commented Jan 23, 2023

The diff itself looks fine here; I'll trust that this section type is what is needed :-)

Regarding CI:

  • Can you see if you can consolidate deps back to one version of object? There's probably another crate somewhere still using the old version.
  • One of us will need to vet the diff for the object upgrade; I'll get to this in a bit if no one else does first!

@afonso360
Copy link
Contributor Author

Can you see if you can consolidate deps back to one version of object? There's probably another crate somewhere still using the old version.

Did the best I could, we are now only duplicating the hashbrown crate. The old version is a dependency of indexmap which itself is a dependency of 3-4 other crates, and I figured this would be a good place to stop.

Also hashbrown has moved some of their API that we used behind a feature flag.

One of us will need to vet the diff for the object upgrade; I'll get to this in a bit if no one else does first!

👍, You also recently did a similar vet for object up to 0.30 in #5434 (comment) so we're just missing 0.30 -> 0.30.3.

@cfallin
Copy link
Member

cfallin commented Feb 8, 2023

@afonso360 would you be willing to update this with an exemption for the hashbrown version duplication to the cargo-deny config (if the issue hasn't gone away with updates in the meantime)? Otherwise I think this PR is ready to go...

@afonso360
Copy link
Contributor Author

afonso360 commented Feb 9, 2023

I've rebased this and added the exception, but I think we still have the same issue with cargo vet of missing a few vet's similar to #5550.

@afonso360
Copy link
Contributor Author

Rebased this on top of main, and that resolves pretty much all of our vet issues. With the exception of object 0.30.1 -> 0.30.3, that update is introduced in this PR since SectionKind::ReadOnlyDataWithRel didn't exist before.

jameysharp added a commit to jameysharp/wasmtime that referenced this pull request Feb 17, 2023
This audit is needed for bytecodealliance#5619. I'm going ahead and updating Cargo.toml
and Cargo.lock at the same time because no source code changes are
required for this update.
Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

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

Once #5827 merges you should be able to rebase this without any cargo-vet complaints. At that point, go ahead and merge!

jameysharp added a commit that referenced this pull request Feb 18, 2023
This audit is needed for #5619. I'm going ahead and updating Cargo.toml
and Cargo.lock at the same time because no source code changes are
required for this update.
This changes the default section type to be `ReadOnlyDataWithRel` instead of `Data`.
On COFF types the CRT initializers do not run unless their section is read only.

The new SectionKind makes these sections read only for COFF and MachO, but leaves it as Writable as required by ELF.
@afonso360
Copy link
Contributor Author

Thanks for unblocking this! 🎉

@afonso360 afonso360 added this pull request to the merge queue Feb 18, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 18, 2023
@afonso360
Copy link
Contributor Author

The merge queue failure seems to have been sporadic, I retried the job and it passed. I'm going to retry the merge queue again.

@afonso360 afonso360 added this pull request to the merge queue Feb 18, 2023
Merged via the queue into bytecodealliance:main with commit 1e6c94b Feb 18, 2023
@afonso360 afonso360 deleted the crt-initializers branch February 18, 2023 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:module cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants