Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

linux: add system call bindings#2820

Merged
dlang-bot merged 2 commits intodlang:masterfrom
carun:add-syscalls
Oct 15, 2021
Merged

linux: add system call bindings#2820
dlang-bot merged 2 commits intodlang:masterfrom
carun:add-syscalls

Conversation

@carun
Copy link
Contributor

@carun carun commented Oct 6, 2019

Reviving #1984

@dlang-bot
Copy link
Contributor

dlang-bot commented Oct 6, 2019

Thanks for your pull request and interest in making D better, @carun! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

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 references

Your 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 locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + druntime#2820"

@PetarKirov
Copy link
Member

PetarKirov commented Oct 7, 2019

Remaining style issues:

make -f posix.mak style
[..]
Enforce whitespace before opening parenthesis
grep -nrE "\<(for|foreach|foreach_reverse|if|while|switch|catch|version)\(" $(find src -name '*.d') ; test $? -eq 1
src/core/sys/linux/unistd.d:3:version(linux):
src/core/sys/linux/syscalls.d:3:version(linux):
src/core/sys/linux/syscalls.d:10:version(CoreDdoc)
src/core/sys/linux/syscalls.d:15:version(X86_64)
src/core/sys/linux/syscalls.d:356:else version(X86)
posix.mak:423: recipe for target 'style_lint' failed
make: *** [style_lint] Error 1

@carun
Copy link
Contributor Author

carun commented Oct 7, 2019

@ZombineDev not sure why some checks are failing. Do you mind explaining? Thanks.

@thewilsonator
Copy link
Contributor

../druntime/src/core/sys/linux/syscalls.d(19): Error: enum `core.sys.linux.syscalls.SYS` conflicts with enum `core.sys.linux.syscalls.SYS` at ../druntime/src/core/sys/linux/syscalls.d(13)

Try

version (CoreDdoc)
 {
     /// Linux system call number from Linux's asm/unistd.h
     enum SYS : c_long;
 }
 _else_ version (X86_64)
 {
    //...
}

@carun
Copy link
Contributor Author

carun commented Oct 8, 2019

@thewilsonator I'm not sure why it still fails. The errors seem to indicate unittest failure. Apparently, I don't see the errors explaining where/why it failed.

@rainers
Copy link
Member

rainers commented Oct 8, 2019

The Windows failures look unrelated, maybe the additional files trigger some compiler bug. The win32-ldc failure looks like the compiler silently exits while compiling the unittest executable. I haven't been able to reproduce locally, though.

@carun carun force-pushed the add-syscalls branch 2 times, most recently from 9dabdd1 to 55e09fa Compare October 8, 2019 08:43
@carun
Copy link
Contributor Author

carun commented Oct 8, 2019

@ZombineDev @thewilsonator help pls!

@rainers
Copy link
Member

rainers commented Oct 8, 2019

The failing test hints at a problem: PR #2821 has undone the changes in #2801. Not sure why it has passed the CI, though.
Edit: I remember the test didn't always fail, chances were about 1 fail out of 3 runs.

@wilzbach
Copy link
Contributor

As I pointed out in my PR, the mir-kernel package is far superior. It's all Boost, so the least we could do here would be to do a proper copy/paste as otherwise soon users on ARM etc. will have to add their bindings as well.

@carun
Copy link
Contributor Author

carun commented Oct 10, 2019

@wilzbach I can redo this PR with a copy paste from https://github.com/libmir/mir-linux-kernel. Honestly, the copy-paste makes me cringe. But we can't have Phobos to depend on libmir. I see no other solution at the moment!

@PetarKirov
Copy link
Member

@carun libmir/mir-linux-kernel exists for 2 reasons:

  1. Druntime didn't offer the functionality
  2. It's a dub package which makes it easy to use, independently of the compiler used.

We can fix 1 and then after all compilers catch up in a couple of releases mir-linux-kernel will stop needing to exist.

I suggest we move forward by adding all the functionality to druntime so that everyone (not only those using mir-linux-kernel) can benefit.

In long term, I believe that we should split druntime into packages that implement compiler functionality (directly tied to the version of the compiler) and bindings for the system libraries, which can be used by any compiler version. The latter should be put on code.dlang.org.

@dlang-bot dlang-bot added Needs Rebase needs a `git rebase` performed stalled labels May 27, 2021
@dlang-bot dlang-bot removed Needs Rebase needs a `git rebase` performed stalled labels Sep 29, 2021
@RazvanN7
Copy link
Contributor

@PetarKirov @Geod24 @maxhaton I rebased this. Should we get this on or should we make sure we are properly copy pasting from libmir?

Comment on lines 9 to 11
public import core.sys.linux.syscalls : SYS;
import core.stdc.config : c_long;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public import core.sys.linux.syscalls : SYS;
import core.stdc.config : c_long;
public import core.sys.linux.syscalls : SystemCall;

statx = 332,
}
}
else version (X86)
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to also add support for ARMv7, Aarch64, but that can be done in a separate PR of course.

@PetarKirov
Copy link
Member

@RazvanN7 Thank you for the reminder, this PR completely fell off my radar. Should be good to go modulo the above comments.

Perhaps if one want to go the extra mile, we could add a unittest that actually verifies that the syscall function works (e.g. that we haven't made an ABI error in the declarations.

@RazvanN7 RazvanN7 force-pushed the add-syscalls branch 2 times, most recently from e2f9fad to 7c57cce Compare October 13, 2021 06:42
@RazvanN7
Copy link
Contributor

@PetarKirov I have applied the review. Regarding the unittest, where you thinking of just doing a random syscall and check for success or test that all the defined syscalls work? If it's the latter, I would rather have it in a different PR as it seems to be a fairly large addition.

@Geod24
Copy link
Member

Geod24 commented Oct 14, 2021

Perhaps if one want to go the extra mile, we could add a unittest that actually verifies that the syscall function works (e.g. that we haven't made an ABI error in the declarations.

We shouldn't be doing IO / non-pure operations in unittests. Syscall is definitely not something we should be unittesting.
If we want to test it, it should be an integration test, but I think that'd be a mess to maintain.

@ibuclaw
Copy link
Member

ibuclaw commented Oct 14, 2021

Agreed, we should only be unit testing code that we write, not the bindings.

@RazvanN7
Copy link
Contributor

The autotester failure is a spurious one and I am tempted to think that the stdcpp test is also a spurious one as it does not have any information about why it is failing.

@dlang-bot dlang-bot merged commit 34b31e2 into dlang:master Oct 15, 2021
@ibuclaw
Copy link
Member

ibuclaw commented Dec 9, 2021

Why was this merged?!?!

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.

9 participants