Skip to content

Allow passing Mach-O flags to set_segment_section #13137

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

Merged
alexcrichton merged 1 commit into from
Apr 20, 2026
+22 −7
Merged

Allow passing Mach-O flags to set_segment_section #13137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions cranelift/module/src/data_context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ pub struct DataDescription {
pub function_relocs: Vec<(CodeOffset, ir::FuncRef)>,
/// Data addresses to write at specified offsets.
pub data_relocs: Vec<(CodeOffset, ir::GlobalValue, Addend)>,
/// Object file section
pub custom_segment_section: Option<(String, String)>,
/// Object file segment, section and Mach-O flags.
pub custom_segment_section: Option<(String, String, u32)>,
Comment on lines +63 to +64
Copy link
Copy Markdown
Contributor Author

@madsmtm madsmtm Apr 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Another design for this could be to take:

segment: String,
section: String,
macho_section_type: String,
macho_section_attributes: Vec<String>,

And then have the "pure_instructions" -> S_ATTR_PURE_INSTRUCTIONS mappings in cranelift-object? What would you prefer?

Copy link
Copy Markdown
Contributor

@bjorn3 bjorn3 Apr 19, 2026

Choose a reason for hiding this comment

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

Maybe directly use object::write::SectionFlags and then or those flags with the ones cranelift-object would set by default?

Copy link
Copy Markdown
Contributor Author

@madsmtm madsmtm Apr 19, 2026

Choose a reason for hiding this comment

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

But that would introduce a dependency on object to cranelift-module?

Copy link
Copy Markdown
Contributor

@bjorn3 bjorn3 Apr 19, 2026

Choose a reason for hiding this comment

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

Ah, right this is cranelift-module, not cranelift-object.

Copy link
Copy Markdown
Contributor Author

@madsmtm madsmtm Apr 20, 2026

Choose a reason for hiding this comment

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

A third alternative would be:

custom_segment_section: Option<String>,

And do the entire parsing inside cranelift-object.

/// Alignment in bytes. `None` means that the default alignment of the
/// respective module should be used.
pub align: Option<u64>,
Expand Down Expand Up @@ -112,8 +112,8 @@ impl DataDescription {
}

/// Override the segment/section for data, only supported on Object backend
pub fn set_segment_section(&mut self, seg: &str, sec: &str) {
self.custom_segment_section = Some((seg.to_owned(), sec.to_owned()))
pub fn set_segment_section(&mut self, seg: &str, sec: &str, macho_flags: u32) {
self.custom_segment_section = Some((seg.to_owned(), sec.to_owned(), macho_flags))
}

/// Set the alignment for data. The alignment must be a power of two.
Expand Down
21 changes: 18 additions & 3 deletions cranelift/object/src/backend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,8 +434,8 @@ impl Module for ObjectModule {
"Custom section not supported for TLS"
)));
}
let (seg, sec) = &custom_segment_section.as_ref().unwrap();
self.object.add_section(
let (seg, sec, macho_flags) = &custom_segment_section.as_ref().unwrap();
let section = self.object.add_section(
seg.clone().into_bytes(),
sec.clone().into_bytes(),
if decl.writable {
Expand All @@ -445,7 +445,22 @@ impl Module for ObjectModule {
} else {
SectionKind::ReadOnlyDataWithRel
},
)
);

match self.object.section_flags_mut(section) {
SectionFlags::MachO { flags } => {
*flags = *macho_flags;
Copy link
Copy Markdown
Contributor

@bjorn3 bjorn3 Apr 20, 2026

Choose a reason for hiding this comment

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

Actually shouldn't this be |=? As is this would overwrite the default flags even when no flags are passed.

Copy link
Copy Markdown
Contributor Author

@madsmtm madsmtm Apr 20, 2026

Choose a reason for hiding this comment

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

I considered that, but there are no default flags for (SectionKind::Data, SectionKind::ReadOnlyData and SectionKind::ReadOnlyDataWithRel), and if there were, it would break if we tried to |= the section type.

Copy link
Copy Markdown
Contributor

@bjorn3 bjorn3 Apr 20, 2026

Choose a reason for hiding this comment

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

Would skipping the flag set if macho_flags is 0 makes sense?

Copy link
Copy Markdown
Contributor Author

@madsmtm madsmtm Apr 20, 2026

Choose a reason for hiding this comment

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

Maybe we could do:

let section_type = *macho_flags & SECTION_TYPE;
let section_attributes = *macho_flags & SECTION_ATTRIBUTES;
let current_attributes = (*flags & SECTION_ATTRIBUTES);
*flags = section_type | current_attributes | section_attributes;

But I feared that might be confusing too, if I request only no_dead_strip, it might be weird if I got other attributes too.

Though we should probably have an assert that *flags is 0.

Copy link
Copy Markdown
Contributor Author

@madsmtm madsmtm Apr 20, 2026

Choose a reason for hiding this comment

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

I opened #13153 for adding a debug assertion instead

}
_ => {
if *macho_flags != 0 {
return Err(cranelift_module::ModuleError::Backend(anyhow::anyhow!(
"unsupported Mach-O flags for this platform: {macho_flags:?}"
)));
}
}
}

section
};

if used {
Expand Down
Loading