Stream: git-wasmtime

Topic: wasmtime / PR #1836 Cranelift: Module data apis should al...

Wasmtime GitHub notifications bot (Jun 06 2020 at 14:11):

carlokok opened PR #1836 from feature/object_file_section to master:

This fixes #1640.

What it does is two things:

• Expose a new api on the data context type to set the section of the current data item
• Emit it in the Object backend

I have no idea how to write a testcase for this or if it's meaningful at all.
I don't know who can review this.

Wasmtime GitHub notifications bot (Jun 06 2020 at 14:34):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Jun 06 2020 at 14:34):

bjorn3 created PR Review Comment:

Can you add an assertion that it isn't a tls data object? Those only work for StandardSection::Tls.

Wasmtime GitHub notifications bot (Jun 08 2020 at 05:23):

carlokok updated PR #1836 from feature/object_file_section to master:

This fixes #1640.

What it does is two things:

• Expose a new api on the data context type to set the section of the current data item
• Emit it in the Object backend

I have no idea how to write a testcase for this or if it's meaningful at all.
I don't know who can review this.

Wasmtime GitHub notifications bot (Jun 08 2020 at 17:20):

carlokok updated PR #1836 from feature/object_file_section to master:

This fixes #1640.

What it does is two things:

• Expose a new api on the data context type to set the section of the current data item
• Emit it in the Object backend

I have no idea how to write a testcase for this or if it's meaningful at all.
I don't know who can review this.

Wasmtime GitHub notifications bot (Jun 08 2020 at 17:56):

carlokok updated PR #1836 from feature/object_file_section to master:

This fixes #1640.

What it does is two things:

• Expose a new api on the data context type to set the section of the current data item
• Emit it in the Object backend

I have no idea how to write a testcase for this or if it's meaningful at all.
I don't know who can review this.

Wasmtime GitHub notifications bot (Jun 08 2020 at 18:15):

pchickey requested pchickey for a review on PR #1836.

Wasmtime GitHub notifications bot (Jun 08 2020 at 18:29):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Jun 08 2020 at 18:29):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Jun 08 2020 at 18:29):

pchickey created PR Review Comment:

idiomatic would be to just say seg.to_owned() rather than use String::from. Also, maybe this method should be named set_segment_section?

Wasmtime GitHub notifications bot (Jun 08 2020 at 18:29):

pchickey created PR Review Comment:

In Rust, we don't use assertions to indicate that the user provided bad input to a library function, instead we use the error type of the function's Result. In this case, we want to do something like:

if let Some((segment, section)) = datasection {
    return Err(cranelift_module::ModuleError::Backend(anyhow::anyhow!("Custom section not supported by cranelift-faerie: `{}:{}`", segment, section)));
}
~~~

Wasmtime GitHub notifications bot (Jun 08 2020 at 18:29):

pchickey created PR Review Comment:

same as before, return an Err(ModuleError::Backend(anyhow!(...))) rather than assert here please.

Wasmtime GitHub notifications bot (Jun 08 2020 at 18:29):

pchickey created PR Review Comment:

std::string::String is always in scope as String so style would be to just say String here. Also, we should note in the comment what each member of the pair is for - I think they are for segment and section, right? should we make the field named custom_segment_section to reflect that? Is there ever a use case where you'd want to specify a section but not a segment (maybe you're letting a linker script determine segments from section names?)

Wasmtime GitHub notifications bot (Jun 08 2020 at 18:29):

pchickey created PR Review Comment:

I think for the simplejit backend we'd be better off just ignoring the custom segment/section than throwing an error here.

Wasmtime GitHub notifications bot (Jun 08 2020 at 18:49):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Jun 08 2020 at 18:49):

bjorn3 created PR Review Comment:

For mach-O files the linker is always responsible for picking the right segment. In fact the object files should only contain a single segment.

Wasmtime GitHub notifications bot (Jun 08 2020 at 18:50):

pchickey submitted PR Review.

Wasmtime GitHub notifications bot (Jun 08 2020 at 18:50):

pchickey created PR Review Comment:

Thanks @bjorn3. So it sounds like we should be specifying a custom section and a custom segment in two different fields.

Wasmtime GitHub notifications bot (Jun 08 2020 at 18:52):

carlokok submitted PR Review.

Wasmtime GitHub notifications bot (Jun 08 2020 at 18:52):

carlokok created PR Review Comment:

@bjorn3 maybe i misunderstand but this:
seg: __OBJC
sec: __image_info

besides
seg: __DATA
sec __data

isn't unusual on mach-o. (that said, segments are ignored on non mach-o afaik)

Wasmtime GitHub notifications bot (Jun 12 2020 at 22:17):

philipc created PR Review Comment:

For mach-O files the linker is always responsible for picking the right segment. In fact the object files should only contain a single segment.

My understanding is that the linker uses the segment name specified in the object file. Note that segment names appear in two places: in the segment and in the section. The linker ignores the first of these, but still uses the segment name in the section. It has to do this, because __const can appear in both __TEXT and __DATA.

Wasmtime GitHub notifications bot (Jun 12 2020 at 22:17):

philipc submitted PR Review.

Wasmtime GitHub notifications bot (Jun 15 2020 at 18:46):

carlokok updated PR #1836 from feature/object_file_section to master:

This fixes #1640.

What it does is two things:

• Expose a new api on the data context type to set the section of the current data item
• Emit it in the Object backend

I have no idea how to write a testcase for this or if it's meaningful at all.
I don't know who can review this.

Wasmtime GitHub notifications bot (Jun 17 2020 at 04:48):

carlokok updated PR #1836 from feature/object_file_section to master:

This fixes #1640.

What it does is two things:

• Expose a new api on the data context type to set the section of the current data item
• Emit it in the Object backend

I have no idea how to write a testcase for this or if it's meaningful at all.
I don't know who can review this.

Wasmtime GitHub notifications bot (Jun 17 2020 at 05:29):

carlokok requested pchickey for a review on PR #1836.

Wasmtime GitHub notifications bot (Jun 17 2020 at 17:49):

pchickey merged PR #1836.

Last updated: Dec 23 2024 at 13:07 UTC