-
-
Notifications
You must be signed in to change notification settings - Fork 411
Add DragonFlyBSD support for druntime #1999
Conversation
|
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 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. |
|
It would be very nice if you could split this into several commits (as per your checklist in the PR), and amend it to say "Add DragonFlyBSD support for druntime" instead of "Port druntime to ..." |
| { | ||
| int ai_flags; | ||
| int ai_family; | ||
| int ai_socktype = SOCK_STREAM; /* socktype default value required to be able to perform getAddrInfo on DragonFlyBSD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed getAddrInfo issue in phobos/std/socket.d by giving addrinfo.ai_socktype a default value of SOCK_STREAM.
Without a default value for socktype, getAddrInfo() will generate an error:'servname not supported for ai_socktype' when hints.ai_socktype is not set on DragonFlyBSD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it not be better to change std.socket instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean to set the default value for ai_socktype, right ?
I did try to find a good location in std.socket first (When the unittest failed and pointed out the issue). I tried this, and it worked, but i did not like this solution very much. Maybe mostly caused by the location i choose to 'fix' the issue.
diff --git a/std/socket.d b/std/socket.d
index 361ec73da..ba68e1738 100644
--- a/std/socket.d
+++ b/std/socket.d
@@ -963,6 +963,13 @@ AddressInfo[] getAddressInfo(T...)(in char[] node, T options)
else
static assert(0, "Unknown getAddressInfo option type: " ~ typeof(option).stringof);
}
+ version (DragonFlyBSD)
+ {
+ if (!hints.ai_socktype)
+ {
+ hints.ai_socktype = SOCK_STREAM;
+ }
+ }
return () @trusted { return getAddressInfoImpl(node, service, &hints); }();
}Also doing this in runtime, 'fixes' the issue even when/if running without using phobos. And by just setting the default value it does not prevent anyone from overwriting the value or even initializing it with void.
src/rt/util/typeinfo.d
Outdated
|
|
||
| assert(f1 == f2); | ||
| assert(f1 == f2); | ||
| version (DragonFlyBSD) {} else // FIXME: //core.exception.AssertError@src/rt/util/typeinfo.d(249): unittest failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// FIXME:
core.exception.AssertError@src/rt/util/typeinfo.d(249): unittest failure
Have not figured out what is going on here, nor what this unittest is trying to check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can see the F array there,it has four elements, that they all have the same value in the complex field, and first assert is checking if their value comparison is sane (they all should equal each other, as they should all be in the (0, 0) point). The next assert (I assume this is the failing one, you forgot braces around else) f1 !is f2 checks if these elements are all different (because they are different members of the array). Not sure what's going on in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(The {} are not missing, it's only excluding a single line, without having to change it's surroundings, Wanted to keep the 'DragonFlyBSD' string and 'FIXME' on a single line, so it's easy to find using grep. I noticed this 'unittest exclusion' pattern in other commits.).
It's just a little unclear based on the line i choose to attach this comment to. I should have include the next time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actual assert Issue reraised below.
src/core/sync/mutex.d
Outdated
| // For example, Bionic doesn't appear to do so, so this test is | ||
| // not run on Android. | ||
| version (CRuntime_Bionic) {} else | ||
| version (DragonFlyBSD) {} else // FIXME : Will have to write a test program to see what is going on here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// FIXME :
Will have to write a little c-program to check if DragonFlyBSD pthread does allow trylock after the mutex has been destroyed and does return 0 in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have not found a good solution for this. But trying to lock after destroying the lock does not really make sense anyway. Reading up on some of the documentation regarding destroying a lock, it looks like it is undefined behaviour to try and lock the mutex after destruction. Even though it seems to "work" in some implementations (e.g.Linux).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving it like this for now.
Would require changing pthread implementation on dragonfly, or adding a check to "class Mutex" to verify if it has been initialized/constructed. Both don't really seem reasonable.
posix.mak
Outdated
| # $(GREP) -n -U -P "([ \t]$$|\r)" $(CWS_MAKEFILES) ; test "$$?" -ne 0 | ||
| # $(GREP) -n -U -P "( $$|\r|\t)" $(NOT_MAKEFILES) ; test "$$?" -ne 0 | ||
| #endif | ||
| checkwhitespace: $(INSTALL_DIR)/${OS}/bin${MODEL}/dmd $(TOOLS_DIR)/checkwhitespace.d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using platform dependent grep
we should use the tools/checkwhitespace.d D-version
Note: This not directly related to supporting DragonFlyBSD, and should maybe have been in a separate commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not as easy as that. There's currently no logic in place that ensures that tools is actually checked out and there have been long discussion of tools being an option or required dependency for building DMD + Druntime + Phobos.
The current status quo is that it's okay to use use tools in non essential targets.
Anyhow as you said yourself, please put this in a separate PR.
|
@nemanja-boric-sociomantic |
| assert(errno == fakePureErrno()); // errno shouldn't change | ||
|
|
||
| version (DragonFlyBSD) {} else // FIXME : z seems to be allocated and not null. Why would size_t.max - 2 not be allowed to allocate ? or is the assertion wrong | ||
| assert(z is null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
z seems to be allocated and not null.
Why would size_t.max - 2 not be allowed to allocate ? or is the assertion wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find any documented reference, but I believe this is testing if the glibc's malloc would fail because of the internal block size and the additional alignment (and any other internal metadata) that should be added to the internal allocation, which while adding with the values close to size_t.max would lead to the overflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrote a little test program in ansi-c
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <errno.h>
// On Linux
#if !defined(SIZE_MAX)
#define SIZE_MAX __SIZE_MAX__ // ((size_t)-1) = 9223372036854775807 = LONG_MAX
#endif
// On DragonFlyBSD
//#define SIZE_MAX UINT64_MAX // from /usr/include/machine/int_limits.h
int main () {
printf("SIZE_MAX=%lu\n", SIZE_MAX);
unsigned long tests[] = {
9223372036854775807,
223372036854775808,
23372036854775808,
3372036854775808,
372036854775808,
72036854775808,
2036854775808,
36854775808,
INTPTR_MAX,
UINTPTR_MAX,
SIZE_MAX - 2,
SIZE_MAX,
SIZE_MAX + 'a',
SIZE_MAX + SIZE_MAX,
SIZE_MAX * 20};
for (int test = 0; test < 14; test++) {
printf("test%d: malloc size:%lu, ", test, tests[test]);
uint *ptr = (int *) malloc(tests[test]);
if (ptr == NULL) {
printf("malloc failed, ptr == NULL, errno:%d\n", errno);
} else {
printf(" ptr == %p\n", ptr);
free(ptr);
ptr = NULL;
}
}
return 0;
}Compiled using gcc test_malloc.c -o test_malloc
Linux x86_64 Results:
SIZE_MAX=18446744073709551615
test0: malloc size:9223372036854775807, malloc failed, ptr == NULL, errno:12
test1: malloc size:223372036854775808, malloc failed, ptr == NULL, errno:12
test2: malloc size:23372036854775808, malloc failed, ptr == NULL, errno:12
test3: malloc size:3372036854775808, malloc failed, ptr == NULL, errno:12
test4: malloc size:372036854775808, malloc failed, ptr == NULL, errno:12
test5: malloc size:72036854775808, malloc failed, ptr == NULL, errno:12
test6: malloc size:2036854775808, malloc failed, ptr == NULL, errno:12
test7: malloc size:36854775808, malloc failed, ptr == NULL, errno:12
test8: malloc size:9223372036854775807, malloc failed, ptr == NULL, errno:12
test9: malloc size:18446744073709551615, malloc failed, ptr == NULL, errno:12
test10: malloc size:18446744073709551613, malloc failed, ptr == NULL, errno:12
test11: malloc size:18446744073709551615, malloc failed, ptr == NULL, errno:12
test12: malloc size:96, ptr == 0x7f6cd80008c0
test13: malloc size:18446744073709551614, malloc failed, ptr == NULL, errno:12
On Linux the destination pointer seems to be checked as well.
DragonFlyBSD (x86_64) Results:
SIZE_MAX=18446744073709551615
test0: malloc size:9223372036854775807, malloc failed, ptr == NULL, errno:12
test1: malloc size:223372036854775808, malloc failed, ptr == NULL, errno:12
test2: malloc size:23372036854775808, malloc failed, ptr == NULL, errno:12
test3: malloc size:3372036854775808, malloc failed, ptr == NULL, errno:12
test4: malloc size:372036854775808, malloc failed, ptr == NULL, errno:12
test5: malloc size:72036854775808, ptr == 0x800800000
test6: malloc size:2036854775808, ptr == 0x800800000
test7: malloc size:36854775808, ptr == 0x800800000
test8: malloc size:9223372036854775807, malloc failed, ptr == NULL, errno:12
test9: malloc size:18446744073709551615, ptr == 0x800455000
test10: malloc size:18446744073709551613, ptr == 0x800455000
test11: malloc size:18446744073709551615, ptr == 0x800455000
test12: malloc size:96, ptr == 0x8006200c0
test13: malloc size:18446744073709551614, ptr == 0x800455000
On DragonFly it seems to be ok, as long as the upperbound of '9223372036854775807' is observed. Using SIZE_MAX however results faulty behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps is worth raising the bug report in the DragoFlyBSD tracker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After creating the above 'proof' i think that makes sense.
DragonFlyBSD Bug Report
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a potential solution to the malloc(SIZE_MAX) issue. Send a diff to upstream dragonfly issue manager (Issue 3114), seems to resolve the case reported above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue has been fixed in dragonfly master
Removing this unittest escape
Comments do really help, thanks and thank you for the work! |
|
@nemanja-boric-sociomantic Thanks and Thanks for the review. |
posix.mak
Outdated
| # pkg_add -r gmake | ||
| # and then run as gmake rather than make. | ||
|
|
||
| #QUIET:=@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for this addition?
|
|
||
| # build with shared library support | ||
| # (defaults to true on supported platforms, can be overridden w/ make SHARED=0) | ||
| SHARED=$(if $(findstring $(OS),linux freebsd),1,) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted all the posix.mak changes i made (They should not have been committed).
posix.mak
Outdated
| # $(GREP) -n -U -P "([ \t]$$|\r)" $(CWS_MAKEFILES) ; test "$$?" -ne 0 | ||
| # $(GREP) -n -U -P "( $$|\r|\t)" $(NOT_MAKEFILES) ; test "$$?" -ne 0 | ||
| #endif | ||
| checkwhitespace: $(INSTALL_DIR)/${OS}/bin${MODEL}/dmd $(TOOLS_DIR)/checkwhitespace.d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not as easy as that. There's currently no logic in place that ensures that tools is actually checked out and there have been long discussion of tools being an option or required dependency for building DMD + Druntime + Phobos.
The current status quo is that it's okay to use use tools in non essential targets.
Anyhow as you said yourself, please put this in a separate PR.
| assert(f1 == f2); | ||
| assert(f1 == f2); | ||
| version (DragonFlyBSD) {} else // FIXME: //core.exception.AssertError@src/rt/util/typeinfo.d(249): unittest failure | ||
| assert(f1 !is f2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the failing assertion.
I think i might need some help from one of the 'src/rt/typeinfo/*' code owners on how to debug this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mathias-lang-sociomantic @Geod24 @WalterBright @redstar
Can one of you help me figure out why/how 'assert(f1 !is f2);' could be failing.
Could it be that comparing complex numbers is causing trouble. Does one of you have a good way of tracing ?
src/core/stdc/errno.c
Outdated
| errno = val; | ||
| return val; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit, but use an editor that adds newline at bottom of files.
| }; | ||
| _x87 x87; | ||
|
|
||
| uint mxcsr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this patch, but I've just noticed that the OpenBSD branch is wrong.
| else version (Solaris) | ||
| { | ||
| // No unsafe pointer manipulation. | ||
| @trusted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this version(Solaris) block has been unintentionally duplicated.
e279d63 to
9a82aed
Compare
src/core/stdc/errno.c
Outdated
| int * __error1(void) { | ||
| return __error(); | ||
| } | ||
| #else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this some more, the idea behind the block in core.stdc.errno below is to avoid needing this C file from druntime, as implemented in #1917. Would something like this work to replace the D declaration and alias below, so you could drop this C patch altogether?
pragma(mangle, "errno") extern int __errno;
ref int errno() { return __errno;}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joakim-noah Thanks for this much nicer solution !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it work, ie all the stdlib tests pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it does. I run all the unittests before each/any commit. Thanks for this much nicer solution.
bf847b3 to
48bb186
Compare
|
|
||
| #include <errno.h> | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change here.
d253cf1 to
ee89791
Compare
|
Hi guys, would it help if this PR were be split by file, so that it would make a little more progress ? |
|
No, somebody just needs to make sure all review comments have been addressed and make a decision. |
In my experience it does help. However, splitting it up by file might be a bit to granular.
For example,
|
|
He already split this pull into three after Nemenja said that. Don't split again, I will round up the review and try to get it going here. |
ee89791 to
e09d5f7
Compare
e09d5f7 to
0ee0954
Compare
|
Ping @ZombineDev @CyberShadow @klickverbot @MartinNowak |
48c5110 to
3454101
Compare
3454101 to
8fe321d
Compare
wilzbach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK everything looks properly scoped in version blocks to me. Anything else blocking this PR?
|
@wilzbach Thanks a lot ! |
3e5e903 to
bf676c5
Compare
joakim-noah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits left.
posix.mak
Outdated
|
|
||
| $(DMD): | ||
| make -C $(DMD_DIR)/src -f posix.mak BUILD=$(BUILD) OS=$(OS) MODEL=$(MODEL) | ||
| $(MAKE) -C $(DMD_DIR)/src -f posix.mak BUILD=$(BUILD) OS=$(OS) MODEL=$(MODEL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see one more on line 541, don't know if you care about that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just moved the posix.mak file to the other PR (#2002), because it was mostly needed to compile the dragonflybsd specific files, so a better fit (and i hope it will fix the autotester issues when compiling on win32).
I did checkout the posix.mak file, but it only has 374 lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, checked the Phobos posix.mak for make by mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving the make files to the other PR, seems to have been a good idea. It fixed the autotester win32 issue.
I will check the phobos posix.mak file, to see if it needs correcting (And create a PR if necessary).
src/core/stdc/errno.d
Outdated
| } | ||
| else version( DragonFlyBSD ) | ||
| { | ||
| enum EPERM = 1; /// Operation not permitted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing you didn't write these comments yourself but copied them from somewhere? If so, take out all the comments that you copied, see the linux block for an example. We don't really want to deal with copyright issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIll do !
Funny how to most 'copyright bothersome os-ses' (ie: windows and osx) do have similar comments :-) Might be a good idea, to to the same for them as well.
src/core/sync/mutex.d
Outdated
| // not run on Android. | ||
| version (CRuntime_Bionic) {} else | ||
| version (CRuntime_Musl) {} else | ||
| version (DragonFlyBSD) {} else // FIXME : Will have to write a test program to see what is going on here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to test this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested / Checked with dragonflybsd developers / Will not be corrected
FIXME: Comment removed
Comment above the test extended/updated.
src/rt/backtrace/dwarf.d
Outdated
|
|
||
| version(CRuntime_Glibc) version = linux_or_freebsd; | ||
| else version(FreeBSD) version = linux_or_freebsd; | ||
| else version(DragonFlyBSD) version = linux_or_freebsd; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as we're here, can you change this to glibc_or_bsd? This name has annoyed me for awhile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used "glibc_or_bsdlibc" instead (thought that would be a better fit, and also usable by netbsd/openbsd when needed), if that is ok with you.
src/rt/backtrace/elf.d
Outdated
|
|
||
| version(linux) version = linux_or_freebsd; | ||
| else version(FreeBSD) version = linux_or_freebsd; | ||
| else version(DragonFlyBSD) version = linux_or_freebsd; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, linux_or_bsd.
joakim-noah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit more to do.
src/core/sync/mutex.d
Outdated
| // For example, Bionic doesn't appear to do so, so this test is | ||
| // not run on Android. | ||
| // BTW: Bionic/Musl and DragonFlyBSD don't (appear) to do so, so this test is | ||
| // not skipped on/for them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to 'The Bionic and Musl C runtimes and DragonFly don't appear to do so, so skip this test.'
src/rt/backtrace/elf.d
Outdated
|
|
||
| version(linux) version = linux_or_freebsd; | ||
| else version(FreeBSD) version = linux_or_freebsd; | ||
| version(linux) version = glibc_or_bsdlibc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't actually enable Glibc, as linux refers to kernel APIs. I don't know where these declarations are from, so either change the version selected to CRuntime_Glibc or the new version to linux_or_bsd.
1e118d9 to
d580191
Compare
|
@joakim-noah Thanks for your remarks/comments/review ! |

Related: DMD Pull Request
Bootstrapped via 'dmd-cxx' branch: See
Related PR:
Notes:
(Without it getAddrInfo() will generate an error:'servname not supported for ai_socktype' when hints.ai_socktype is not set.)