Skip to content
This repository was archived by the owner on Nov 3, 2021. It is now read-only.

Delete mentions of copying order in Overview.md #134

Merged
merged 5 commits into from
Dec 16, 2019

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Dec 15, 2019

After the spec change in #126, byte copying order is not observable.

This reflects spec changes in WebAssembly#126 in Overview.md.
@aheejin aheejin requested review from binji and rossberg December 15, 2019 09:50
Copy link
Contributor

@conrad-watt conrad-watt left a comment

Choose a reason for hiding this comment

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

Thanks! Comment just pointing out an opportunity for clarification given our new semantics.

@@ -281,10 +281,8 @@ Note that it is allowed to use `memory.init` on the same data segment more than
once.

Initialization takes place bytewise from lower addresses toward higher
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we have an ahead-of-time bounds check, the copying order isn't observable. If it ever did become observable through some concurrency feature, implementations won't be able to guarantee that the order is observed to be low-to-high. This was something that came out of the discussion in #111, where it was noted that implementations are calling the system memcpy/memmove, which doesn't guarantee this ordering.

I feel that this sentence should now just state that the write/copying order isn't observable, due to the ahead-of-time bounds check. Same for copy/fill below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

stay written.
Memory copying order of bytes is not observable. When a trap resulting from an
access outside the source or target region occurs, partial copying does not take
place.

(Data are read and written as-if individual bytes were read and
Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies for missing this earlier:

Lines 316-325 above (more out-of-date ordering info) should be removed. It's too far from the diff for github to let me interact.

This line here should be replaced with a statement that copying occurs as though the source bytes are first read into an intermediate buffer (which was implied by lines 316-325 but not mentioned elsewhere). This is similar to the phrasing used to describe memmove.

Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced the paragraph with the sentence from the memmove link. Let me know if you think it's not sufficient.

addresses. A trap resulting from an access outside the target memory
only occurs once the first byte that is outside the target is reached.
Bytes written before the trap stay written.
Memory writing order of bytes is not obserable. When a trap resulting from an
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: obserable

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Done.

Copy link
Contributor

@conrad-watt conrad-watt left a comment

Choose a reason for hiding this comment

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

LGTM

@lars-t-hansen
Copy link
Contributor

FWIW, I think this partly duplicates and conflicts with #130, which I just merged (things got delayedb because @rossberg was away and then after that I was inattentive).

@aheejin
Copy link
Member Author

aheejin commented Dec 16, 2019

Oh, I didn't know your PR was pending. I deleted all duplicate paragraphs and this is what remains 😂 PTAL.

@aheejin aheejin changed the title Reflect 0-len/drop semantic changes to Overview.md Delete mentions of copying order in Overview.md Dec 16, 2019
Copy link
Contributor

@lars-t-hansen lars-t-hansen left a comment

Choose a reason for hiding this comment

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

Works for me :)

@aheejin aheejin merged commit a00705b into WebAssembly:master Dec 16, 2019
@aheejin aheejin deleted the spec_change branch December 16, 2019 09:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants