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

Revert "linux: add system call bindings"#3646

Merged
dlang-bot merged 1 commit intomasterfrom
revert-2820-add-syscalls
Dec 9, 2021
Merged

Revert "linux: add system call bindings"#3646
dlang-bot merged 1 commit intomasterfrom
revert-2820-add-syscalls

Conversation

@ibuclaw
Copy link
Member

@ibuclaw ibuclaw commented Dec 9, 2021

Reverts #2820

Broke every supported Linux platform.

Don't merge ANY new Linux bindings unless it supports all of the following:

  • ARM
  • AArch64
  • HPPA
  • HPPA64
  • PPC
  • PPC64
  • MIPS
  • MIPS64
  • RISCV32
  • RISCV64
  • S390
  • SystemZ
  • SPARC
  • SPARC64
  • X86
  • X86_64

Regular testing is done on these platforms and downstream testers do complain when you break the build for them.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @ibuclaw!

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#3646"

@ibuclaw ibuclaw added the Regression PRs that fix regressions label Dec 9, 2021
@dlang-bot dlang-bot merged commit 93a1997 into master Dec 9, 2021
@ibuclaw ibuclaw deleted the revert-2820-add-syscalls branch December 9, 2021 22:50
@ljmf00
Copy link
Member

ljmf00 commented Dec 9, 2021

Regular testing is done on these platforms and downstream testers do complain when you break the build for them.

Is it possible to upstream some tests?

@jacob-carlborg
Copy link
Contributor

Is it possible to upstream some tests?

It's easy to run tests through emulation using Docker and QEMU: docker run --platform "linux/$ARCH".

@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 10, 2021

@kinke might be better placed as ldc supports cross compiling to all targets from one binary, and we only need to verify it passes semantic analysis only on i.e ARM/linux, and other predefined version combinations.

@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 10, 2021

Adding some hidden/internal dmd switch to override predefined versions with an explicit list of conditionals would probably suffice as well.

dmd -o- \
  --x-predefined-version-list=AArch64,Linux,Posix,HardFloat,CRuntime_Gcc,CppRuntime_Gcc \
  druntime/src/**.d

@jacob-carlborg
Copy link
Contributor

Currently LDC does not include all the above mentioned architectures. That should only be a configuration change necessary to enable additional architectures.

@ibuclaw
Copy link
Member Author

ibuclaw commented Dec 10, 2021

Only the following are high priority targets to check:

aarch64-none-linux-gnu
arm-linux-gnueabi
i586-unknown-freebsd
i686-pc-linux-gnu
powerpc64-unknown-linux-gnu
powerpc64le-unknown-linux-gnu
sparc-sun-solaris2.11
x86_64-pc-linux-gnu

@jacob-carlborg
Copy link
Contributor

SPARC is one of the architectures that is currently not included in LDC. Here's a list of all the architectures in LDC 1.27.0:

  Registered Targets:
    aarch64    - AArch64 (little endian)
    aarch64_32 - AArch64 (little endian ILP32)
    aarch64_be - AArch64 (big endian)
    amdgcn     - AMD GCN GPUs
    arm        - ARM
    arm64      - ARM64 (little endian)
    arm64_32   - ARM64 (little endian ILP32)
    armeb      - ARM (big endian)
    avr        - Atmel AVR Microcontroller
    mips       - MIPS (32-bit big endian)
    mips64     - MIPS (64-bit big endian)
    mips64el   - MIPS (64-bit little endian)
    mipsel     - MIPS (32-bit little endian)
    msp430     - MSP430 [experimental]
    nvptx      - NVIDIA PTX 32-bit
    nvptx64    - NVIDIA PTX 64-bit
    ppc32      - PowerPC 32
    ppc32le    - PowerPC 32 LE
    ppc64      - PowerPC 64
    ppc64le    - PowerPC 64 LE
    r600       - AMD GPUs HD2XXX-HD6XXX
    riscv32    - 32-bit RISC-V
    riscv64    - 64-bit RISC-V
    thumb      - Thumb
    thumbeb    - Thumb (big endian)
    wasm32     - WebAssembly 32-bit
    wasm64     - WebAssembly 64-bit
    x86        - 32-bit X86: Pentium-Pro and above
    x86-64     - 64-bit X86: EM64T and AMD64

@PetarKirov
Copy link
Member

PetarKirov commented Dec 10, 2021

Broke every supported Linux platform.

@ibuclaw can you please share the error that you got for at least one linux platform? Because I reviewed the code again and I still couldn't see find anything wrong with it. (I thought there was an else static assert (0, "Unsupported platform"); or similar, but there wasn't.)

Edit:

Is it perhaps this line:

public import core.sys.linux.syscalls : SystemCall;

So the problem is that we're unconditionally importing something that is conditionally defined?

Don't merge ANY new Linux bindings unless it supports all of the following:

In principle I agree that we should strive to have complete bindings, but I don't understand what's wrong with an incremental approach either.

@ljmf00
Copy link
Member

ljmf00 commented Dec 10, 2021

Is it possible to upstream some tests?

It's easy to run tests through emulation using Docker and QEMU: docker run --platform "linux/$ARCH".

I think it would be a good step to implement some simple tests to avoid merging not so clear broken changes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Regression PRs that fix regressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants