Separate linux kernel and glibc APIs as much as possible

[joakim-noah]

This pull uses the new CRuntime_Glibc and CRuntime_Bionic predefined global versions from dmd pulls #4111 and #3643 to start separating out linux kernel APIs from glibc and bionic APIs. I used dstep and a modified Dscanner to help find type and struct declarations, but had to check enums again by hand. dmd pull #4111 will have to be merged before this one.

[joakim-noah]

Updated this pull so that it separates out all linux/Android blocks from my original Android pulls #681 and #734, plus some low-hanging fruit from rt.* and a couple other core.* modules, into linux/glibc/bionic. Most of the declarations were really from glibc or bionic, so it ended up being a lot of search and replace after checking the contents of blocks to make sure they came from glibc. I renamed rt.sections_linux to rt.sections_glibc and rt.sections_android to rt.sections_bionic, as they seemed the more appropriate names given this effort and the APIs they were using. I also fixed some bionic constants in core.sys.posix.time that I happened to notice were wrong, not sure why.

This pull is now ready for review. I'll submit a separate pull for the remaining core.sys.posix modules later. I'll also take a look at separating out core.sys.linux but not sure if I'll ever do much with that.

[joakim-noah]

Now that dmd pull #4111 has been merged, this pull passes the auto-tester. I just updated core.stdc.errno because it was failing on linux/x64, rest of the pull is the same.

I haven't tested this pull on Android yet, as I have to separate the rest of the linux/Android blocks before it can compile for Android, but this pull should be a drop-in replacement on non-Android linux, with the exception of the new arch check in core.stdc.errno.

I'll submit a separate pull for the rest of the linux/Android blocks in druntime, then check it on Android and fix whatever's necessary in that pull. For now, this pull works on non-Android linux and can be merged.

[joakim-noah]

Oh, the other change that could possibly affect someone is the renaming of the internal modules rt.sections_linux and rt.sections_android, but it's extremely unlikely that anybody outside druntime is importing those. A google search for "import rt.sections_linux" turns up two references to rt.sections from druntime and that's it.

[joakim-noah]

btw, can someone explain what the point of completely redundant version blocks like these are for? @complexmath or @MartinNowak, I don't see the point. I'd rather combine them all into one declaration and only specialize when necessary for some new OS.

[MartinNowak]

The approach in this pull looks good.

[MartinNowak]

btw, can someone explain what the point of completely redundant version blocks like these are for? @complexmath or @MartinNowak, I don't see the point. I'd rather combine them all into one declaration and only specialize when necessary for some new OS.

Yes, default cases are terribly bad for porting because you'll notice incorrect declarations only at runtime. The first druntime port to MIPS almost took me more than a week of debugging to find all wrong numbers.
In fact all of the headers you linked too are missing a static assert(0, "unimplemented"); as default case.

[yebblies]

Yes, default cases are terribly bad for porting because you'll notice incorrect declarations only at runtime. The first druntime port to MIPS almost took me more than a week of debugging to find all wrong numbers.

Not using default cases doesn't mean you have to copy+paste the declarations - define an in-module version symbol for the common-code case.

[joakim-noah]

I don't understand: are you saying all these have been found to be different for some as yet unsupported Posix platform? They are all exactly the same for the supported platforms, I see no reason to define them over and over again.

[MartinNowak]

I don't understand: are you saying all these have been found to be different for some as yet unsupported Posix platform?

We had that discussion already and I will not start it again.
This also helps during reviews, because it's easier to see that everything was addressed.

Not using default cases doesn't mean you have to copy+paste the declarations

You shouldn't copy paste anything but find the relevant C headers and use the declarations from there.

define an in-module version symbol for the common-code case

That would be fine with me, as long as it remains neat.

[joakim-noah]

We had that discussion already and I will not start it again.

If you're talking about me, we once discussed constants that you found to be different on a certain arch, MIPS, not OSs, which are being prematurely separated for no apparent reason in these examples I gave.

This also helps during reviews, because it's easier to see that everything was addressed.

Simply writing the same thing for every OS cannot possibly help anything, as has been done so far in many cases, it's just busywork for no reason.

[yebblies]

@joakim-noah

btw, can someone explain what the point of completely redundant version blocks like these are for?

There is no point, it's stupid. And it's partly caused by not being able to do this:
version(This || That)

The workaround is to just do this:

version(This)
    version = thisOrThat;
else version(That)
    version = thisOrThat;

version(thisOrThat)
{
}
else
    static assert(0);

Which is ugly, but works, removes the duplication, and avoids having any code in 'else block'.

[joakim-noah]

@yebblies, yes, I'm aware that boolean logic inside version statements is not allowed, but I'm asking the larger question: why are we differentiating these OSs at all? There are many examples like this, where we have 5-6 OS blocks with no meaningful difference between them and often no default static assert case either, that Martin seems to be justifying them with.

Did someone see that these declarations were different on some OS when druntime was first started and planned ahead by separating them? Otherwise, it seems like Sean probably put the first one inside a version block just in case, and we've been blindly adding more OS blocks to that first one for no reason.

I ask because I'd rather submit a pull to combine each case like the three above into a single declaration, rather than have to sit here and add another one onto them again for Android.

[joakim-noah]

Updated this pull so it doesn't rename rt.sections_linux and rt.sections_android anymore, as I may be able to merge the two instead in a later PR, similar to how Martin is trying in #1068.

[dnadlinger]

and often no default static assert case either, that Martin seems to be justifying them with.

There should be at least one per module. Otherwise, it's a bug.

[dnadlinger]

[…] and we've been blindly adding more OS blocks to that first one for no reason.

This is definitely not the case. You will find it hard to argue that you haven't been told this already.

[joakim-noah]

and often no default static assert case either, that Martin seems to be justifying them with.

There should be at least one per module. Otherwise, it's a bug.

Then there's a lot of bugs in druntime.

[…] and we've been blindly adding more OS blocks to that first one for no reason.

This is definitely not the case. You will find it hard to argue that you haven't been told this already.

Being told something and actually providing a reason are two different things. If you believe that one of the three linked examples above needs to be separated for a reason, please make a case. The fact that no case has been made, either here or on the forum, suggests that there is none.

In any case, this pull has little to do with such repeated OS blocks. If you have something to say, take a look at the in-progress #1069, where I remove all such redundantly repeated OS blocks.

[ibuclaw]

No comments?

[joakim-noah]

Walter said to remove them, the last time I added to this module.

[dnadlinger]

Yep, you are right in that. @ibuclaw, the problem is that code comments could be copyrightable, whereas the API itself definitely isn't.

[ibuclaw]

and often no default static assert case either, that Martin seems to be justifying them with.

There should be at least one per module. Otherwise, it's a bug.

Then there's a lot of bugs in druntime.

Yes, there are lots of bugs, but we are dealing with that as and when we stumble across them.

[dnadlinger]

Then there's a lot of bugs in druntime.

Patches are welcome.

If you believe that one of the three linked examples above needs to be separated for a reason, please make a case. The fact that no case has been made, either here or on the forum, suggests that there is none.

As Martin said, we did make a case for the current policy already, and if I remember correctly more than once. I don't get why you claim that nobody ever gave you any reasons and then conclude that our current strategy must clearly be unfounded. That's simply not the case, and refusing to acknowledge the arguments of your opponent in a debate is just rude. Sure, you can still disagree and try to convince us that we chose the wrong trade-off in favoring explicitness over conciseness of code. But not even making an effort to understand the arguments and then criticizing that none are given isn't going to work. Sorry for being very direct, but I'm afraid this is going to turn into quite the waste of time for all involved parties otherwise.

[joakim-noah]

As Martin said, we did make a case for the current policy already, and if I remember correctly more than once. I don't get why you claim that nobody ever gave you any reasons and then conclude that our current strategy must clearly be unfounded. That's simply not the case, and refusing to acknowledge the arguments of your opponent in a debate is just rude. Sure, you can still disagree and try to convince us that we chose the wrong trade-off in favoring explicitness over conciseness of code. But not even making an effort to understand the arguments and then criticizing that none are given isn't going to work. Sorry for being very direct, but I'm afraid this is going to turn into quite the waste of time for all involved parties otherwise.

Just so we're clear, precisely what is the "current policy" that you believe has been adequately explained? In one of my first PRs, we had a disagreement about whether new declarations should be speculatively protected by version(arch) statements with a static assert as the default, particularly for one case where Martin found some constants varied for one arch on a different Android API version. You two are conflating that discussion with the current linked examples of version(OS) statements, where none of the supported OS's actually differ.

I'd say that conflation by the two of you and then refusing to acknowledge that this situation is different, since it's by OS and not by arch, is what's rude, not my pointing out that the two situations are different and asking for a specific reason why you believe any one of the three linked examples should not be collapsed. If your claim is that all C declarations should be speculatively separated in this way, druntime would be 10 times larger and nobody would attempt to port it to a new platform.

If you haven't looked at my linked examples above, I'm talking about blocks like this, which are littered around druntime:

version( linux )
{
    int fsync(int) @trusted;
}
else version( OSX )
{
    int fsync(int) @trusted;
}
else version( FreeBSD )
{
    int fsync(int) @trusted;
}
else version( Android )
{
    int fsync(int) @trusted;
}
else version( Solaris )
{
    int fsync(int) @trusted;
}

No difference in any of the supported OSs, no static assert, and that function is unused anywhere in druntime/phobos. The only reason it has an Android block is because I'm a stickler for completeness and grepped for it. In my PR #1069, I collapse it down to a single declaration, as it's separated by OS for no apparent reason.

Now, the question is: why was it ever separated? Is it known that fsync varies on some yet-to-be-supported platforms? If so, shouldn't that be documented in a comment? If not, why are we wasting our time?

[joakim-noah]

Updated this pull so that the remaining linux/Android blocks use the appropriate CRuntime instead. When combined with #1069, there are no more uses of version(Android) in druntime and all tests pass on both linux/Glibc/x86 and linux/Bionic/x86, ie Android/x86. Some blocks had a mix of declarations from Glibc and the linux kernel, so I added a comment indicating that. I also modified core.sys.posix.sys.utsname so that the initial extern(C) attribute is a label instead.

Update: Added one last tweak, defining IPPROTO_GGP on linux, because it was necessary to clean up a little of the Phobos PR that goes with this one.

[joakim-noah]

Any reason this pull has been sitting unmerged for almost two months now?

[MartinNowak]

Lack of time, sorry, scheduled for review next week.

[joakim-noah]

No need to be sorry. I was just worried that the controversy above was mistakenly presumed to be about this pull, when most of the discussion above was about changes made in #1069.

[joakim-noah]

Updated this pull to deal with the new modules core.sys.linux.stdio and core.sys.posix.sys.msg, plus amended a comment in core.sys.windows.stat to not refer to std.c from phobos anymore. Not sure why core.sys.posix.sys.msg is in core.sys.posix when it only has declarations for linux.

[joakim-noah]

Ping, this pull was marked as ready for review more than two months ago when it passed the auto-tester. Unless someone is planning on checking all the APIs to see where they actually come from, which I doubt since it took me days to do so in the first place, should be pretty straightforward to check for other, smaller mistakes and merge.

[joakim-noah]

Been a couple months, @MartinNowak, what is the status of this? I know you've been busy pushing out the stable release, but hopefully this can get in now.

[joakim-noah]

Can someone merge this? I don't see what the holdup is, most of it is simple renaming.

I'm about to submit another PR for Android/ARM support, but I need the core.stdc.errno portion of this pull to get merged first, so I can add ARM support to that new portion.

[schveiguy]

I noticed you removed all the docs for the errnos for linux. Was that on purpose?

[joakim-noah]

Yep.

[schveiguy]

Ugh... How annoying. But I guess that's life.

[MartinNowak]

Better move this non-standard thing to core.sys.linux.time.CLOCK_MONOTONIC_RAW.

[MartinNowak]

No need to be sorry. I was just worried that the controversy above was mistakenly presumed to be about this pull, when most of the discussion above was about changes made in #1069.

No, you finally seem to hit the sweet spot ;).

[ibuclaw]

This PR caused a regression. https://issues.dlang.org/show_bug.cgi?id=22689