Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

fix some compatibility issues for Solaris systems #3624

Merged
merged 10 commits into from
Nov 30, 2021

Conversation

BarrOff
Copy link
Contributor

@BarrOff BarrOff commented Nov 13, 2021

Hello,

some fixes to aid compilation on Solaris/Illumos based systems.
I successfully tested the changes on a recent Solaris/Illumos installation.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @BarrOff! 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#3624"

@thewilsonator
Copy link
Contributor

cc @ibuclaw

@thewilsonator thewilsonator added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Nov 13, 2021
const(T)* _M_str;

alias __data = _M_str;
alias __size = _M_len;
Copy link
Member

Choose a reason for hiding this comment

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

Developer Studio doesn't implement this, so maybe add a pragma saying as much. Falling back on doing what gcc does is the safe option, which seems to be what you're doing here.

src/core/sys/posix/signal.d Show resolved Hide resolved
src/core/internal/backtrace/elf.d Show resolved Hide resolved
@thewilsonator thewilsonator removed the 72h no objection -> merge The PR will be merged if there are no objections raised. label Nov 16, 2021
@RazvanN7
Copy link
Contributor

@ibuclaw is this good to go?

@ibuclaw
Copy link
Member

ibuclaw commented Nov 19, 2021

SIG stuff are already defined elsewhere (with the wrong value), and missing SHF_COMPRESSED.

@BarrOff
Copy link
Contributor Author

BarrOff commented Nov 23, 2021

@ibuclaw, @RazvanN7

SIG stuff are already defined elsewhere (with the wrong value), and missing SHF_COMPRESSED.

What can/should I still do about the SIG stuff? I already moved SHF_ORDERED and SHF_EXCLUDE to their correct location. The version check has also been removed.

Concerning SHF_COMPRESSED: it is not available in the headers, at least on my Illumos instance, and as far as my research shows on Solaris, too. However, this enum is just used at one place: processDebugLineSectionData of core.internal.backtrace.elf.d. We could put a version(Solaris) guard around it. But I am no expert with ELF file format, and can not say what needs to change for the Solaris version.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 23, 2021

@ibuclaw, @RazvanN7

SIG stuff are already defined elsewhere (with the wrong value), and missing SHF_COMPRESSED.

What can/should I still do about the SIG stuff? I already moved SHF_ORDERED and SHF_EXCLUDE to their correct location. The version check has also been removed.

SIGVTALRM and SIGXFSZ are already defined here:

enum SIGVTALRM = 31;
enum SIGXCPU = 30;
enum SIGXFSZ = 25;

They just have the wrong value.

Concerning SHF_COMPRESSED: it is not available in the headers, at least on my Illumos instance, and as far as my research shows on Solaris, too. However, this enum is just used at one place: processDebugLineSectionData of core.internal.backtrace.elf.d. We could put a version(Solaris) guard around it. But I am no expert with ELF file format, and can not say what needs to change for the Solaris version.

On Solaris 11.4, it's defined as 0x800 in the sys/elf.h header (sandwiched inbetween SHF_TLS and SHF_MASKOS).

@BarrOff
Copy link
Contributor Author

BarrOff commented Nov 25, 2021

@ibuclaw

I fixed the two values and added SHF_COMPRESSED at the right place. On Illumos it really is not defined, see here. There is simply a blank line between SHF_TLS and SHF_MASKOS.

If that is ok now, the only remaining problem is the part about CppRuntime_Sun.
I don't have access to Developer Studio, and don't know what has to be inserted for CppRuntime_Sun.

However, Developer Studio has been replaced by gcc in OpenIndiana, in OmniOS also and I suppose in SmartOS too. The only system still using Developer Studio is the official Oracle Solaris.

To solve this problem I see two possible solutions:

  1. a check could be added to see if we have an "Illumos" Solaris version. If we do, use what gcc does. This however does not fix the root of the problem. That is, dmd predefines CppRuntime_Sun on all Solaris systems.
  2. Therefore moving such a check to dmd might be more sensible. If the build detects an Illumos system, it should predefine CppRuntime_Gcc instead. Then my change here would not be necessary.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 25, 2021

@ibuclaw

I fixed the two values and added SHF_COMPRESSED at the right place. On Illumos it really is not defined, see here. There is simply a blank line between SHF_TLS and SHF_MASKOS.

Maybe someone should send a patch in. :-)
IIUC, the OSS Solaris should be compatible with the proprietary version.

If that is ok now, the only remaining problem is the part about CppRuntime_Sun.
I don't have access to Developer Studio, and don't know what has to be inserted for CppRuntime_Sun.

However, Developer Studio has been replaced by gcc in OpenIndiana, in OmniOS also and I suppose in SmartOS too. The only system still using Developer Studio is the official Oracle Solaris.

To solve this problem I see two possible solutions:

  1. a check could be added to see if we have an "Illumos" Solaris version. If we do, use what gcc does. This however does not fix the root of the problem. That is, dmd predefines CppRuntime_Sun on all Solaris systems.
  2. Therefore moving such a check to dmd might be more sensible. If the build detects an Illumos system, it should predefine CppRuntime_Gcc instead. Then my change here would not be necessary.

I think it's reasonable to default to CppRuntime_Gcc on Solaris, and use Sun only if requested by the -target triplet.

Comment on lines 1451 to 1452
enum SIGVTALRM = 28;
enum SIGXFSZ = 31;
Copy link
Member

Choose a reason for hiding this comment

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

You can remove these now. :-)

@BarrOff
Copy link
Contributor Author

BarrOff commented Nov 30, 2021

@ibuclaw

I have removed the redundant signals and reverted the change to string_view.d .
I hope this is now ok to be merged? :-)

Besides, I opened a pull request on DMD to default to CppRuntime_Gcc on Solaris.

enum SI_ASYNCIO = -4;
enum SI_MESGQ = -5;

enum SIGIO = SIGPOLL;
Copy link
Member

Choose a reason for hiding this comment

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

I'd move this with the others, but don't think it would be harming anything at this point in time.

@dlang-bot dlang-bot merged commit 6498b9a into dlang:master Nov 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants