Add DragonFlyBSD support : Split PR : src/core/sys/dragonflybsd related changes #2002

dkgroot · 2017-12-19T14:56:55Z

Split PR : #1999
Moved the src/core/sys/dragonflybsd related changes into this PR

Related PR:

Add DragonFlyBSD support for dmd dmd#7463

dlang-bot · 2017-12-19T14:56:57Z

Thanks for your pull request and interest in making D better, @dkgroot! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
My PR follows the DStyle
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.

joakim-noah

Just checked for version and attributes at top, not for actual accuracy of the declarations, and only yielded one style nit and one attribute question.

joakim-noah · 2017-12-19T15:45:55Z

src/core/sys/dragonflybsd/dlfcn.d

+version (DragonFlyBSD):
+extern (C):
+nothrow:
+@nogc:


You can put all three attributes on the same line, ie extern (C) nothrow @nogc: as done for some other files below, probably better style. Also, should most of these headers be @system too, @MartinNowak?

Putting the attributes on one line -> No problem.
Adding @System attribute to all of these header files, i gave that a try and all the unittests are still passed without a problem. If that is an indication that it is ok, i can commit the change.

If this change is pushed through, should this also be backported to the other sys/core/sys/${OS}/ directories (in a seperate PR) ?

Proposed

diff --git a/src/core/sys/dragonflybsd/dlfcn.d b/src/core/sys/dragonflybsd/dlfcn.d index 9a416525..86effe89 100644 --- a/src/core/sys/dragonflybsd/dlfcn.d +++ b/src/core/sys/dragonflybsd/dlfcn.d @@ -11,9 +11,8 @@ module core.sys.dragonflybsd.dlfcn; public import core.sys.posix.dlfcn; version (DragonFlyBSD): -extern (C): -nothrow: -@nogc: + +extern (C) nothrow @nogc @system: enum __BSD_VISIBLE = true; diff --git a/src/core/sys/dragonflybsd/sys/_bitset.d b/src/core/sys/dragonflybsd/sys/_bitset.d index 079115d7..24f9d9c1 100644 --- a/src/core/sys/dragonflybsd/sys/_bitset.d +++ b/src/core/sys/dragonflybsd/sys/_bitset.d @@ -7,7 +7,7 @@ module core.sys.dragonflybsd.sys._bitset; version (DragonFlyBSD): -extern (C) pure nothrow @nogc: +extern (C) pure nothrow @nogc @system: import core.stdc.config : c_long; diff --git a/src/core/sys/dragonflybsd/sys/elf_common.d b/src/core/sys/dragonflybsd/sys/elf_common.d index cd94832d..4bbddf24 100644 --- a/src/core/sys/dragonflybsd/sys/elf_common.d +++ b/src/core/sys/dragonflybsd/sys/elf_common.d @@ -7,9 +7,8 @@ module core.sys.dragonflybsd.sys.elf_common; version (DragonFlyBSD): -extern (C): -pure: -nothrow: + +extern (C) pure nothrow @nogc @system: import core.stdc.stdint;

Etcetera for the rest of the files in this PR
Most of these could also be set to 'pure' i think, correct ?

Not sure, haven't messed with @system much, will let someone who knows better answer.

joakim-noah

With the attribute reformatting, seems fine to me, just don't know if @system is merited also.

joakim-noah · 2018-01-06T16:01:12Z

@ibuclaw, do all the C declarations need to be marked @system also?

dkgroot · 2018-01-06T20:01:32Z

@joakim-noah / @ibuclaw FYI: It will build/unittest correctly either way (with and without @System). Would be happy to add @System to them, if that would result in better / more correct code.

Question: Who will backport these types of changes to the other OS implementations, do you guys have a sort of ledger for this. If not, this would come up over and over, but never really get fixed into the other (more important) implementations.

joakim-noah · 2018-01-07T01:22:33Z

Looking at the @system definition, it would seem to be created for this, so go ahead and mark all files with C function declarations with it up top, then opt out your own local functions that you know are okay as @safe.

dkgroot · 2018-01-07T13:23:29Z

@joakim-noah Done. Can you please check if this is what you intended me to do (revision: 965c1dc)

dkgroot · 2018-01-07T13:23:34Z

Note revision: cf62ee3 , might look like a massive edit, but:

it only removes checking for __BSD_VISIBLE / __POSIX_VISIBLE (which are defined in the cdefs.d file) in these subdirectories
re-indent the lines

ibuclaw · 2018-01-07T15:08:41Z

If you wanted to add __BSD_VISIBLE, shouldn't that go into core.sys.dragonflybsd.config anyway?

Seems like FreeBSD is the exception rather than the rule for defining it elsewhere.

dkgroot · 2018-01-07T17:08:40Z

@ibuclaw It is currently defined in core.sys.dragonflybsd.sys.cdefs.d (just like freebsd), but checking for it's setting, especially inside the dragonflybsd subdirectory is a bit superfluous.
I don't want to add __BSD_VISIBLE (it's already defined). I just wanted to skip checking for it, in this directory, where it is obvious that it has to be true anyway.

dkgroot · 2018-01-07T22:43:00Z

BTW: We should revisit PR1937 regarding using statvfs instead of statfs when it is being finalized, and port the changes to dragonfly implementation (and maybe the other BSD's).
(Using this comment, to link both PR's)

dkgroot · 2018-01-12T19:53:13Z

Ping @andralex / @wilzbach Is there still something keeping this PR from being merged ?

joakim-noah

Checked attributes, found some mislabeling.

joakim-noah · 2018-01-13T05:50:55Z

src/core/sys/dragonflybsd/sys/_cpuset.d

+module core.sys.dragonflybsd.sys._cpuset;
+
+version (DragonFlyBSD):
+extern (C) pure nothrow @nogc @system:


Only aliases and enums, so all these attributes unneeded?

@joakim-noah Thanks for reviewing this !
They were already defined (e.g freebsd implementation), except for the requested addition of @System.|
Does it have a negative effect to set these attributes ? It could prevent future issues (and/or request attention to think about these attributes) when a function or alias is added.

Our previous modules use these attributes inconsistently, since some of them predate the attributes. Would rather get this right for this new platform from the start, attributes do affect the mangling of D functions, ie the ABI.

Ok will work on removing them. Thanks for the explanation !

joakim-noah · 2018-01-13T06:00:51Z

src/core/sys/dragonflybsd/sys/elf32.d

+
+version (DragonFlyBSD):
+
+extern (C) pure nothrow @nogc @system:


No functions other than the few you define below with different attributes, so all attributes other than extern (C) can be removed.

joakim-noah · 2018-01-13T06:01:50Z

src/core/sys/dragonflybsd/sys/elf64.d

+
+version (DragonFlyBSD):
+
+extern (C) pure nothrow @nogc @system:


Same as last module.

joakim-noah · 2018-01-13T06:03:23Z

src/core/sys/dragonflybsd/sys/elf_common.d

+
+version (DragonFlyBSD):
+
+extern (C) pure nothrow @nogc @system:


Same as last two.

joakim-noah · 2018-01-13T06:06:04Z

src/core/sys/dragonflybsd/sys/link_elf.d

+};
+
+
+private alias extern(C) int function(dl_phdr_info*, size_t, void *) dl_iterate_phdr_cb;


extern (C) up top not enough?

For aliases, maybe not.

@ibuclaw It seems ok (unittest wise) when i remove the extra extern (C), so going for that.

joakim-noah · 2018-01-13T06:07:35Z

src/core/sys/dragonflybsd/sys/mman.d

+module core.sys.dragonflybsd.sys.mman;
+
+version (DragonFlyBSD):
+extern (C) nothrow @system:


joakim-noah · 2018-01-13T06:08:25Z

src/core/sys/dragonflybsd/time.d

+
+version(DragonFlyBSD):
+
+extern (C) nothrow @nogc @system:


Only enums, no need for attributes.

dkgroot · 2018-01-24T14:24:34Z

Ping @wilzbach @andralex @ibuclaw
Are there any issues with or objections against this PR in it's current state ?

DragonFly Based CI

wilzbach

OK let's get things rolling here. We need to be more active when porters do their initial port. It's all in version(DragonFlyBSD) - so it really doesn't affect us at all. And in this case @dkgroot even setup a CI to show that everything is passing nicely.

wilzbach · 2018-02-09T18:59:44Z

src/core/sys/dragonflybsd/netinet/in_.d

+
+public import core.sys.posix.netinet.in_;
+
+version(DragonFlyBSD):


Why don't you move this five lines higher?

Done thanks !

dkgroot · 2018-02-09T19:52:37Z

Thanks everyone for your reviews / input / suggestions and comments !
@wilzbach Thanks for accepting this PR

joakim-noah

Couple nits left.

joakim-noah · 2018-02-10T11:03:12Z

src/core/sys/dragonflybsd/sys/_cpuset.d

+module core.sys.dragonflybsd.sys._cpuset;
+
+version (DragonFlyBSD):
+extern (C):


Enums are not placed in an object file, so extern(C) makes no sense, think the same for aliases too?

Remarked out 'extern(C):'

joakim-noah · 2018-02-10T11:09:06Z

src/core/sys/dragonflybsd/time.d

+
+public import core.sys.posix.time;
+
+extern (C):


Enums are like preprocessor defines in C/C++, they're pasted in and have no linkage, so this does nothing.

Remarked out 'extern(C):'

joakim-noah · 2018-02-10T11:28:43Z

src/core/sys/dragonflybsd/sys/link_elf.d

+};
+
+
+private alias int function(dl_phdr_info*, size_t, void *) dl_iterate_phdr_cb;


When I asked about the extern(C) here, I honestly didn't know if it was required. You said the tests didn't need it, but that's understandable since you don't call the function it's used from below. Thinking about it, I guess since it's a type for an extern(C) function, which don't mangle their types anyway, I suppose the extern(C) adds nothing. I tested it on linux by removing this attribute there and the tests ran fine.

If anyone knows why it might be needed, comment here.

Should I remove the second 'extern', ie:

extern (C) nothrow @system: ... extern int dl_iterate_phdr(dl_iterate_phdr_cb __callback, void*__data); extern int ...

should become:

extern (C) nothrow @system: ... int dl_iterate_phdr(dl_iterate_phdr_cb __callback, void*__data); int ...

Is that what you want to tell me ?

No, I was talking about the extern(C) you already removed above. I don't think you need to change it again, but I'm not sure, so I left it open if anybody else wanted to comment here.

Removing the extern(C) at the top of link_elf.d is also possible, but i would have to add notrown to the extern's at the bottom. So this is almost more of a style issue.

It has nothing to do with style, these attributes normally affect linkage and functionality. However, I think they don't do anything in these cases so it's good you removed them from the bottom. The one up top is needed though.

Ok Reverting the last change. Thanks for the update and review !

joakim-noah · 2018-02-10T16:02:12Z

src/core/sys/dragonflybsd/sys/link_elf.d

+int dl_iterate_phdr(dl_iterate_phdr_cb __callback, void*__data);
+int dl_iterate_phdr(dl_iterate_phdr_cb_ngc __callback, void*__data) @nogc;
+int _rtld_addr_phdr(const void*, dl_phdr_info*) @nogc;
+int _rtld_get_stack_prot() @nogc;


I wasn't referring to these, but it seems to be a good catch: the extern here was meaningless as applied to functions?

Moved the extern(C) to the bottom to only pertain to these fout functions.

No, move it back up, I wasn't talking about the attributes up top. You need those there.

ibuclaw · 2018-02-11T07:53:17Z

Don't we check for D style in new patches? I guess not.

dkgroot · 2018-02-11T10:50:05Z

@ibuclaw Did i miss something ? Did make some style error ? Will correct it, if you tell me where.

andralex · 2018-02-12T19:03:29Z

Congratulations @dkgroot and many thanks to all reviewers!

dkgroot · 2018-02-12T19:28:34Z

Thanks to all reviewers / Thanks Andrei !

dkgroot requested review from andralex and wilzbach as code owners December 19, 2017 14:56

joakim-noah suggested changes Dec 19, 2017

View reviewed changes

joakim-noah approved these changes Dec 20, 2017

View reviewed changes

dlang-bot added Needs Rebase needs a `git rebase` performed and removed Needs Rebase needs a `git rebase` performed labels Jan 1, 2018

dkgroot force-pushed the dragonfly-core.sys.dragonflybsd branch 2 times, most recently from 4384215 to 5a8c45d Compare January 3, 2018 17:30

wilzbach mentioned this pull request Jan 9, 2018

Add a new DragonFlyBSD build host braddr/d-tester#62

Closed

dkgroot force-pushed the dragonfly-core.sys.dragonflybsd branch from b7e151a to ba90956 Compare January 12, 2018 15:44

joakim-noah suggested changes Jan 13, 2018

View reviewed changes

dkgroot force-pushed the dragonfly-core.sys.dragonflybsd branch from 3c26c17 to 607a2bd Compare January 13, 2018 16:02

dkgroot force-pushed the dragonfly-core.sys.dragonflybsd branch from 607a2bd to ad4ad37 Compare January 18, 2018 16:42

This was referenced Jan 18, 2018

Add DragonFlyBSD support for dmd dlang/dmd#7463

Merged

Add DragonFlyBSD support for druntime #1999

Merged

dkgroot force-pushed the dragonfly-core.sys.dragonflybsd branch from ad4ad37 to b116a70 Compare January 21, 2018 22:23

dkgroot force-pushed the dragonfly-core.sys.dragonflybsd branch from b116a70 to ddb040b Compare January 28, 2018 18:52

dkgroot force-pushed the dragonfly-core.sys.dragonflybsd branch from ddb040b to fcd001b Compare February 7, 2018 03:07

wilzbach approved these changes Feb 9, 2018

View reviewed changes

wilzbach added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Feb 9, 2018

dkgroot force-pushed the dragonfly-core.sys.dragonflybsd branch from aed805e to 6416582 Compare February 9, 2018 19:12

joakim-noah suggested changes Feb 10, 2018

View reviewed changes

joakim-noah reviewed Feb 10, 2018

View reviewed changes

Add DragonFlyBSD support (src/core/sys/dragonflybsd)

2110fc8

dkgroot force-pushed the dragonfly-core.sys.dragonflybsd branch from 67401d4 to 2110fc8 Compare February 11, 2018 20:40

joakim-noah approved these changes Feb 12, 2018

View reviewed changes

wilzbach added the auto-merge label Feb 12, 2018

dlang-bot merged commit 1b5d296 into dlang:master Feb 12, 2018


		version (DragonFlyBSD):

		extern (C) pure nothrow @nogc @system:

		};


		private alias extern(C) int function(dl_phdr_info, size_t, void ) dl_iterate_phdr_cb;


		public import core.sys.posix.netinet.in_;

		version(DragonFlyBSD):

		};


		private alias int function(dl_phdr_info, size_t, void ) dl_iterate_phdr_cb;

Uh oh!

Add DragonFlyBSD support : Split PR : src/core/sys/dragonflybsd related changes #2002

Add DragonFlyBSD support : Split PR : src/core/sys/dragonflybsd related changes #2002

Uh oh!

Conversation

dkgroot commented Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

joakim-noah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkgroot Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joakim-noah left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joakim-noah commented Jan 6, 2018

Uh oh!

dkgroot commented Jan 6, 2018

Uh oh!

joakim-noah commented Jan 7, 2018

Uh oh!

dkgroot commented Jan 7, 2018

Uh oh!

dkgroot commented Jan 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibuclaw commented Jan 7, 2018

Uh oh!

dkgroot commented Jan 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dkgroot commented Jan 7, 2018

Uh oh!

dkgroot commented Jan 12, 2018

Uh oh!

joakim-noah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkgroot Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkgroot commented Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dkgroot commented Dec 19, 2017 •

edited

Loading

dlang-bot commented Dec 19, 2017 •

edited

Loading

dkgroot Dec 19, 2017 •

edited

Loading

joakim-noah left a comment •

edited

Loading

dkgroot commented Jan 7, 2018 •

edited

Loading

dkgroot commented Jan 7, 2018 •

edited

Loading

dkgroot Jan 13, 2018 •

edited

Loading

dkgroot commented Jan 24, 2018 •

edited

Loading