Skip to content

Conversation

@chris-oo
Copy link
Member

@chris-oo chris-oo commented Sep 25, 2025

The implementation for intrinsics in minimal_rt was incorrect. For example, the inline asm block for memcpy correctly describes that rdi will be clobbered by rep movsb, but memcpy's API requires that the return value is the original destination pointer, not the incremented pointer. Fix this by removing the mut on dest and indicating to the compiler to discard the clobbered value.

It seems that in most cases, the compiler ignored the return value of memcpy, except when #2031 was being added. Certain compilation options or function implementations would cause the compiler to use the inlined returned value of memcpy directly from the call to realloc, which would then cause a later an assertion failure due to the pointer not being what was expected and overlapping with the new destination.

Fix all the intrinsics in minimal_rt to correctly return the unmodified dest or ptr as they should.

@chris-oo chris-oo requested a review from a team as a code owner September 25, 2025 18:33
Copilot AI review requested due to automatic review settings September 25, 2025 18:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug in the memcpy implementation in minimal_rt where the function was returning the modified destination pointer instead of the original destination pointer as required by the C API standard.

  • Removes the mut keyword from the dest parameter to prevent accidental modification
  • Changes the inline assembly output to discard the clobbered rdi register value instead of capturing it

@smalis-msft
Copy link
Contributor

Check memset, ARM impls, and any other similar functions too?

@chris-oo chris-oo changed the title minimal_rt: fix memcpy to correctly return original dest minimal_rt: fix intrinsics to correctly return original dest Sep 25, 2025
@chris-oo chris-oo changed the title minimal_rt: fix intrinsics to correctly return original dest minimal_rt: fix intrinsics to correctly return original pointer Sep 25, 2025
Copy link
Contributor

@smalis-msft smalis-msft left a comment

Choose a reason for hiding this comment

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

I am amazed we got this far before this became an issue.

@chris-oo
Copy link
Member Author

chris-oo commented Sep 25, 2025

I know right? Seems like pure dumb luck if anything. But I am wondering if in a full optimized build with fat LTO, if the compiler actually knows that dest is mutated and can't be relied on, and must spill the original value if it wants it. That's at least the final assembly code I saw for the working binaries, on release builds.

@chris-oo chris-oo enabled auto-merge (squash) September 25, 2025 21:21
@chris-oo chris-oo merged commit 801e808 into microsoft:main Sep 25, 2025
29 checks passed
@chris-oo chris-oo deleted the minimalrt-memcpy branch September 25, 2025 21:42
andyplank-msft pushed a commit to andyplank-msft/openvmm that referenced this pull request Nov 4, 2025
…osoft#2041)

The implementation for intrinsics in minimal_rt was incorrect. For
example, the inline asm block for `memcpy` correctly describes that
`rdi` will be clobbered by `rep movsb`, but `memcpy`'s API requires that
the return value is the _original_ destination pointer, not the
incremented pointer. Fix this by removing the mut on `dest` and
indicating to the compiler to discard the clobbered value.

It seems that in most cases, the compiler ignored the return value of
memcpy, except when microsoft#2031 was being added. Certain compilation options
or function implementations would cause the compiler to use the inlined
returned value of `memcpy` directly from the call to realloc, which
would then cause a later an assertion failure due to the pointer not
being what was expected and overlapping with the new destination.

Fix all the intrinsics in `minimal_rt` to correctly return the
unmodified `dest` or `ptr` as they should.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants