Skip to content
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.

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

avoid using aligned_alloc; posix_memalign is better-behaved #125003

Merged
merged 1 commit into from May 15, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented May 11, 2024

Also there's no reason why wasi should be different than all the other Unixes here.

@rustbot
Copy link
Collaborator

rustbot commented May 11, 2024

r? @m-ou-se

rustbot has assigned @m-ou-se.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 11, 2024
@RalfJung
Copy link
Member Author

Is this rounding even still necessary? Looking at the code, I don't see any indication that the size has to be a multiple of the alignment... but also going back in the history I can't see such a requirement, so maybe I am looking at the wrong thing.

Cc @sunfishcode

Or... should we just use posix_memalign on wasi and remove this code path entirely? It seems to exist.

@RalfJung
Copy link
Member Author

Even emmalloc seems to have posix_memalign, so I think we should just use that.
Cc @cuviper

@RalfJung RalfJung changed the title update comment about using aligned_alloc avoid using aligned_alloc; posix_memalign is better-behaved May 14, 2024
@sunfishcode
Copy link
Member

That code in wasi-libc is unmodified upstream musl code, so all musl targets presumably behave the same way.

On the standards side, the behavior of aligned_alloc when size is not a multiple of alignment appears to have been undefined behavior in C11, though it has since been revised in DR460.

@RalfJung
Copy link
Member Author

RalfJung commented May 14, 2024

That code in wasi-libc is unmodified upstream musl code, so all musl targets presumably behave the same way.

Yeah I since realized this rounding up the size was added only for emmalloc, not for the default wasi-libc allocator.

On the standards side, the behavior of aligned_alloc when size is not a multiple of alignment appears to have been undefined behavior in C11, though it has since been revised in DR460.

Correct, it's not been UB since C18 so the old arguments don't apply any more. emmalloc still decides not to support allocations when the size is not a multiple of the alignment, but only when called via aligned_alloc -- as far as I can tell, there's no good reason for that; it supports such requests just fine when they happen via posix_memalign.

aligned_alloc also has the strange requirement that the memory must always be sufficiently aligned for types with fundamental alignment that fit into the requested size... I don't see emmalloc actually do that, but the code is too much for me to follow. We don't rely on that, though, so this wouldn't affect us.

@cuviper
Copy link
Member

cuviper commented May 14, 2024

Using posix_memalign seems ok to me! In emmalloc, they only differ in the up-front checks, then call the same emmalloc_memalign implementation.

https://github.com/WebAssembly/wasi-libc/blob/2f088a99d8437fb980594f4467879555456f447e/emmalloc/emmalloc.c#L910-L915

https://github.com/WebAssembly/wasi-libc/blob/2f088a99d8437fb980594f4467879555456f447e/emmalloc/emmalloc.c#L1265-L1273

(It also seems reasonable that it should now relax that check in aligned_alloc.)

@RalfJung
Copy link
Member Author

I tried to find out why we're using a different function on wasi, and I think it's just a historic coincidence. The commit that added wasi support used aligned_alloc, and then later this got merged with the other Unix codepaths but without changing which function gets called where. So there's no indication here that using posix_memalign on wasi would be a bad idea.

r? @cuviper

@rustbot rustbot assigned cuviper and unassigned m-ou-se May 14, 2024
@cuviper
Copy link
Member

cuviper commented May 15, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 15, 2024

📌 Commit 5cc020d has been approved by cuviper

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 15, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 15, 2024
avoid using aligned_alloc; posix_memalign is better-behaved

Also there's no reason why wasi should be different than all the other Unixes here.
@bors
Copy link
Contributor

bors commented May 15, 2024

⌛ Testing commit 5cc020d with merge 4de34b7...

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-msvc failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
---- [codegen-units] tests\codegen-units\partitioning\local-inlining.rs stdout ----

error: compilation failed!
status: exit code: 1
command: PATH="C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage2\bin;C:\Program Files (x86)\Windows Kits\10\bin\x64;C:\Program Files (x86)\Windows Kits\10\bin\10.0.22621.0\x64;C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.39.33519\bin\HostX64\x64;C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.39.33519\bin\HostX64\x64;C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage0-bootstrap-tools\x86_64-pc-windows-msvc\release\deps;C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage0\bin;C:\Program Files\PowerShell\7;C:\a\_temp\msys64\mingw64\bin;C:\a\_temp\msys64\usr\local\bin;C:\a\_temp\msys64\usr\bin;C:\a\_temp\msys64\usr\bin;C:\a\rust\rust\ninja;C:\hostedtoolcache\windows\Python\3.12.3\x64\Scripts;C:\hostedtoolcache\windows\Python\3.12.3\x64;C:\a\rust\rust\sccache;C:\a\_temp\setup-msys2;C:\Program Files\MongoDB\Server\5.0\bin;C:\aliyun-cli;C:\vcpkg;C:\Program Files (x86)\NSIS;C:\tools\zstd;C:\Program Files\Mercurial;C:\hostedtoolcache\windows\stack\2.15.5\x64;C:\cabal\bin;C:\ghcup\bin;C:\mingw64\bin;C:\Program Files\dotnet;C:\Program Files\MySQL\MySQL Server 8.0\bin;C:\Program Files\R\R-4.3.3\bin\x64;C:\SeleniumWebDrivers\GeckoDriver;C:\SeleniumWebDrivers\EdgeDriver;C:\SeleniumWebDrivers\ChromeDriver;C:\Program Files (x86)\sbt\bin;C:\Program Files (x86)\GitHub CLI;C:\Program Files\Git\bin;C:\Program Files (x86)\pipx_bin;C:\npm\prefix;C:\hostedtoolcache\windows\go\1.21.9\x64\bin;C:\hostedtoolcache\windows\Python\3.9.13\x64\Scripts;C:\hostedtoolcache\windows\Python\3.9.13\x64;C:\hostedtoolcache\windows\Ruby\3.0.6\x64\bin;C:\Program Files\OpenSSL\bin;C:\tools\kotlinc\bin;C:\hostedtoolcache\windows\Java_Temurin-Hotspot_jdk\8.0.412-8\x64\bin;C:\Program Files\ImageMagick-7.1.1-Q16-HDRI;C:\Program Files\Microsoft SDKs\Azure\CLI2\wbin;C:\ProgramData\kind;C:\ProgramData\docker-compose;C:\ProgramData\Chocolatey\bin;C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0;C:\Windows\System32\OpenSSH;C:\Program Files\dotnet;C:\Program Files\PowerShell\7;C:\Program Files\Microsoft\Web Platform Installer;C:\Program Files\Microsoft SQL Server\Client SDK\ODBC\170\Tools\Binn;C:\Program Files\Microsoft SQL Server\150\Tools\Binn;C:\Program Files (x86)\Windows Kits\10\Windows Performance Toolkit;C:\Program Files (x86)\WiX Toolset v3.14\bin;C:\Program Files\Microsoft SQL Server\130\DTS\Binn;C:\Program Files\Microsoft SQL Server\140\DTS\Binn;C:\Program Files\Microsoft SQL Server\150\DTS\Binn;C:\Program Files\Microsoft SQL Server\160\DTS\Binn;C:\Strawberry\c\bin;C:\Strawberry\perl\site\bin;C:\Strawberry\perl\bin;C:\ProgramData\chocolatey\lib\pulumi\tools\Pulumi\bin;C:\Program Files\TortoiseSVN\bin;C:\Program Files\CMake\bin;C:\ProgramData\chocolatey\lib\maven\apache-maven-3.8.7\bin;C:\Program Files\Microsoft Service Fabric\bin\Fabric\Fabric.Code;C:\Program Files\Microsoft SDKs\Service Fabric\Tools\ServiceFabricLocalClusterManager;C:\Program Files\nodejs;C:\Program Files\Git\cmd;C:\Program Files\Git\mingw64\bin;C:\Program Files\Git\usr\bin;C:\Program Files\GitHub CLI;C:\tools\php;C:\Program Files (x86)\sbt\bin;C:\Program Files\Amazon\AWSCLIV2;C:\Program Files\Amazon\SessionManagerPlugin\bin;C:\Program Files\Amazon\AWSSAMCLI\bin;C:\Program Files\Microsoft SQL Server\130\Tools\Binn;C:\Program Files\LLVM\bin;C:\Users\runneradmin\.dotnet\tools;C:\Users\runneradmin\.cargo\bin;C:\Users\runneradmin\AppData\Local\Microsoft\WindowsApps;C:\a\_temp\msys64\usr\bin\site_perl;C:\a\_temp\msys64\usr\bin\vendor_perl;C:\a\_temp\msys64\usr\bin\core_perl" "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2\\bin\\rustc.exe" "C:\\a\\rust\\rust\\tests\\codegen-units\\partitioning\\local-inlining.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=C:\\Users\\runneradmin\\.cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=C:\\a\\rust\\rust\\vendor" "--sysroot" "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2" "--target=x86_64-pc-windows-msvc" "--check-cfg" "cfg(FALSE)" "-C" "incremental=C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\test\\codegen-units\\partitioning\\local-inlining\\local-inlining.inc" "-Z" "incremental-verify-ich" "-Z" "human_readable_cgu_names" "-O" "-C" "prefer-dynamic" "--out-dir" "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\test\\codegen-units\\partitioning\\local-inlining" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\native\\rust-test-helpers" "-L" "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\test\\codegen-units\\partitioning\\local-inlining\\auxiliary" "-Zprint-mono-items=lazy" "-Zinline-in-all-cgus"
--- stdout -------------------------------
MONO_ITEM fn inline::inlined_function @@ local_inlining.8ec87a7fbea559a-user2[Internal] local_inlining.8ec87a7fbea559a-user1[Internal]
MONO_ITEM fn non_user::baz @@ local_inlining.8ec87a7fbea559a-non_user[External]
MONO_ITEM fn user1::foo @@ local_inlining.8ec87a7fbea559a-user1[External]
MONO_ITEM fn user2::bar @@ local_inlining.8ec87a7fbea559a-user2[External]
--- stderr -------------------------------
--- stderr -------------------------------
error: couldn't create a temp dir: Access is denied. (os error 5) at path "C:\\a\\_temp\\msys64\\tmp\\rustcFKhJYE"
error: aborting due to 1 previous error
------------------------------------------


---
test result: FAILED. 40 passed; 1 failed; 3 ignored; 0 measured; 0 filtered out; finished in 1.07s

Some tests failed in compiletest suite=codegen-units mode=codegen-units host=x86_64-pc-windows-msvc target=x86_64-pc-windows-msvc
Build completed unsuccessfully in 0:40:14
make: *** [Makefile:101: ci-msvc-ps1] Error 1
  network time: Wed, 15 May 2024 18:00:28 GMT
##[error]Process completed with exit code 2.
Post job cleanup.
[command]"C:\Program Files\Git\bin\git.exe" version

@bors
Copy link
Contributor

bors commented May 15, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 15, 2024
@cuviper
Copy link
Member

cuviper commented May 15, 2024

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 15, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 15, 2024
Rollup of 5 pull requests

Successful merges:

 - rust-lang#125003 (avoid using aligned_alloc; posix_memalign is better-behaved)
 - rust-lang#125142 (Migrate `run-make/rustdoc-themes` to new rmake.rs)
 - rust-lang#125146 (Migrate `run-make/panic-impl-transitive` to `rmake`)
 - rust-lang#125154 (Small improvements to the documentaion of FnAbi )
 - rust-lang#125159 (Meta: Allow unauthenticated users to modify `L-*`, `PG-*` and `-Z*` labels)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c5b17ec into rust-lang:master May 15, 2024
6 of 7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 15, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 15, 2024
Rollup merge of rust-lang#125003 - RalfJung:aligned_alloc, r=cuviper

avoid using aligned_alloc; posix_memalign is better-behaved

Also there's no reason why wasi should be different than all the other Unixes here.
bors added a commit to rust-lang/miri that referenced this pull request May 16, 2024
Rustup

Pulls in rust-lang/rust#125003 so allocation tests should now work on wasm.
bors added a commit to rust-lang/miri that referenced this pull request May 16, 2024
Rustup, aligned heap-allocations on wasm

Pulls in rust-lang/rust#125003 so allocation tests should now work on wasm.
bors added a commit to rust-lang/miri that referenced this pull request May 16, 2024
Rustup, aligned heap-allocations on wasm

Pulls in rust-lang/rust#125003 so allocation tests should now work on wasm.
@RalfJung RalfJung deleted the aligned_alloc branch May 19, 2024 08:04
RalfJung pushed a commit to RalfJung/rust that referenced this pull request May 19, 2024
Rustup, aligned heap-allocations on wasm

Pulls in rust-lang#125003 so allocation tests should now work on wasm.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-unix Operating system: Unix-like S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants