Conversation
|
Thanks for your pull request and interest in making D better, @kassane! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#16361" |
|
To me this does not look like the correct / complete fix. gperftools/gperftools#693 (comment)
So this needs a fix here: dmd/druntime/src/core/sys/posix/sys/mman.d Lines 294 to 300 in e9c6884 |
|
I looked a little bit further into the musl druntime code, and I think this needs fixing in several places. enum __USE_FILE_OFFSET64 = true;
enum __USE_LARGEFILE = __USE_FILE_OFFSET64 && !__REDIRECT;
enum __USE_LARGEFILE64 = __USE_FILE_OFFSET64 && !__REDIRECT;And everywhere where these are used, druntime needs adjustment. For example also here: dmd/druntime/src/core/sys/posix/dirent.d Lines 388 to 396 in d3db2e7 I have fixed the code for mmap (see commit e48132b).
DMD CI does not test Musl, so please do that manually offline :) Thanks for the work. |
JohanEngelen
left a comment
There was a problem hiding this comment.
Needs changes as mentioned in this comment: #16361 (comment)
Yeah! My test occurs on https://github.com/kassane/alpine-ldc2-docker. |
This comment was marked as resolved.
This comment was marked as resolved.
|
Thanks for the work, looks good now (some minor tweaks to whitespace needed) |
Thanks, for help-me. |
JohanEngelen
left a comment
There was a problem hiding this comment.
I'm changing my approval, because of the large amount of code duplication seen in the extended PR for LDC druntime... Very sorry that my advice lead you down this path...
|
@JohanEngelen Is this good again? |
yes |
Cherry-picked from dlang/dmd#16361.
|
This change caused My knowledge of dlang is very limited (0 to be honest) so I hope someone could help me with fixing this. Edit: 32-bit musl support seems to be broken in multiple places so I will probably need to create a larger patchset. |
Can you submit a new bug report about this? Then you can discuss the correct fix for it there. Thanks! |
|
I hope a draft is fine as well: #21249 |
off_t needs to be 64-bits on all arches Reverts b31aa94 fix: mmap64 error (dlang#16361)
off_t needs to be 64-bits on all arches Reverts b31aa94 fix: mmap64 error (dlang#16361)
off_t needs to be 64-bits on all arches Reverts b31aa94 fix: mmap64 error (dlang#16361)
cc: @kinke