Skip to content

Conversation

@adamperlin
Copy link
Contributor

No description provided.

Comment on lines +94 to +102
if (_insPaddingByte.HasValue)
{
sectionData = new SectionData(section.Type == SectionType.Executable ? _insPaddingByte.Value : (byte)0);
}
else
{
sectionData = new SectionData();
}

Copy link
Member

Choose a reason for hiding this comment

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

Why not just use 0 as the padding byte for executable sections for wasm (that's the default padding)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good call, that would work as well here.

GetOrCreateSection(ObjectNodeSection.ManagedCodeWindowsContentSection);
}
else
else if (_nodeFactory.Target.OperatingSystem != TargetOS.Browser)
Copy link
Member

Choose a reason for hiding this comment

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

Once #122511 is in, we should remove this OS check and instead handle this with an override of GetEmitSection in WasmObjectWriter.

Comment on lines 85 to 87
// Write the number of functions
writer.WriteULEB128((ulong)methodBodies.Count);
for (int i = 0; i < methodBodies.Count; i++)
Copy link
Member

Choose a reason for hiding this comment

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

For these array-based sections, could we do the following?

  • Skip the "count" header.
  • When a node for such a section is seen, emit the data into the section and record that we emitted a node into the section.
  • When emitting the object file, emit the "count" header before emitting the section data.

With this design, we could avoid having this split between "emit into the section as we process the nodes" and "emit at the end".

Comment on lines 66 to 72
public static readonly ObjectNodeSection WasmCodeSection = new ObjectNodeSection("wasm.code", SectionType.Executable, needsAlign: false);
// TODO-Wasm: consider alignment of data sections
public static readonly ObjectNodeSection WasmDataSection = new ObjectNodeSection("wasm.data", SectionType.Writeable, needsAlign: false);
public static readonly ObjectNodeSection WasmFunctionSection = new ObjectNodeSection("wasm.function", SectionType.ReadOnly, needsAlign: false);
public static readonly ObjectNodeSection WasmTypeSection = new ObjectNodeSection("wasm.type", SectionType.ReadOnly, needsAlign: false);
public static readonly ObjectNodeSection WasmExportSection = new ObjectNodeSection("wasm.export", SectionType.ReadOnly, needsAlign: false);
public static readonly ObjectNodeSection WasmMemorySection = new ObjectNodeSection("wasm.memory", SectionType.ReadOnly, needsAlign: false);
Copy link
Member

Choose a reason for hiding this comment

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

If these sections won't be returned by ObjectNode.GetSection, then they should be defined in WasmObjectWriter.

Comment on lines 188 to 192
// Text section will have already been created by the base class when this method is called
GetOrCreateSection(ObjectNodeSection.WasmMemorySection);
GetOrCreateSection(ObjectNodeSection.WasmTypeSection);
GetOrCreateSection(ObjectNodeSection.WasmFunctionSection);
GetOrCreateSection(ObjectNodeSection.WasmExportSection);
Copy link
Member

Choose a reason for hiding this comment

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

If these don't need to be created in a particular order, it's fine to let them get created at use time (first usage of GetOrCreateSection elsewhere for a given section).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't need to be created in a particular order so I think I can change to the approach you're suggesting and remove this!

Comment on lines +521 to +528
// Write the data if:
// 1. We are in unified code/data layout mode so separating code and data nodes doesn't matter, OR
// 2. We are in separate code data layout mode but are writing data nodes
// Note that this has to be done last as not to advance the section writer position.
if (LayoutMode == CodeDataLayout.Unified || node.GetSection(_nodeFactory) != ObjectNodeSection.TextSection)
{
sectionWriter.EmitData(nodeContents.Data);
}
Copy link
Member

Choose a reason for hiding this comment

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

If we change RecordMethod to only write the signature into the signature table and the signature index into the function table, we should be able to change this code to just emit the data directly.

I guess we'd need a way to also prepend length here. We could do that with a bool property that wasm sets (and we only do it for when that property is true and we're emitting into the text section).

Or, we could add a feature to the section writer so the writer for the Text section always prepends length.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants