First set of stdc changes for Android/x86 #681
Conversation
|
Might the linking issue be caused by the module not actually being included in the druntime build? |
| @@ -91,6 +91,24 @@ else version(FreeBSD) | |||
| enum LC_TIME = 5; | |||
| enum LC_MESSAGES = 6; | |||
| } | |||
| else version(Android) | |||
| { | |||
| enum { | |||
There was a problem hiding this comment.
OK, I'll fix this.
|
Looks pretty OK, I didn't look at the declarations in detail though. |
Who is defining |
|
And do all of these remain constant across both arm and x86 variants of android? |
| @@ -504,6 +504,138 @@ else version (Solaris) | |||
| enum EINPROGRESS = 150 /* operation now in progress */; | |||
| enum ESTALE = 151 /* Stale NFS file handle */; | |||
| } | |||
| else version( Android ) | |||
| { | |||
| enum EPERM = 1; // Operation not permitted | |||
There was a problem hiding this comment.
Copying all these comments is a problem - look at the license from where they came from. If there's any doubt that the original license is compatible with the Boost license, delete the comments (they are not necessary anyway).
Interfaces (i.e. the names and the numbers) are not copyrightable, but the comments are.
There was a problem hiding this comment.
I'm sensitive to copyright issues. I always took existing code from core.stdc and modified it where possible. In this case, I first transformed the version (linux) block into something I could diff against errno.h from Android, to see what the difference was. What I found is they were exactly the same, except that Android was missing EWOULDBLOCK and EDEADLOCK. So I then copy-pasted the version(linux) block from stdc/errno.d, changed it to version(Android), and removed those two. :)
I never copy-pasted from the Android files, so all code is copy-pasted from existing druntime blocks or manually entered by me. All comments were copied from existing druntime blocks, I added none.
There was a problem hiding this comment.
Thanks, you've done the right thing here. But it does make me a bit skeptical of the Linux comments there. Anyone want to check those?
There was a problem hiding this comment.
Actually, the more I think about it, the more I think the comments should be removed:
- we should not be attempting to document operating system APIs
- it adds considerable size and slows compilation - these files get imported a lot
- the only comments that should be there are those that explain differences between the D version and the C version
There was a problem hiding this comment.
OK, I removed all comments that could have conceivably come from somewhere else originally in my latest commit to this pull request. Perhaps we can create a different pull request to remove all such comments from core.stdc.
|
@klickverbot, no, stdio.o is included, I checked the archive for the symbols using nm. I've been going back and forth about this over email with dawgfoto. I don't think it's particular to me, as stderr doesn't link on Win32/Win64 either. You can try the example I gave in the forum post to check. @dawgfoto, the declarations can be checked with the header files here: https://android.googlesource.com/platform/bionic/+/master/libc/include/ Android is a reserved symbol in dmd. I modify the TARGET_LINUX block in dmd/src/mars.c to add version(Android) to my locally patched dmd. I'm not sure how dmd will want to officially handle cross-compiling to Android/x86 and the porting work is far from done, so perhaps we can hold off on getting that in. @braddr, good point, I hadn't checked if the ARM headers for Android were different, I was just using the x86 ones. Running a diff now, I see that all the differences are isolated to the asm/ and machine/ directories, with one exception, fenv.h. fenv_t is set to uint on ARM, as opposed to a struct on x86. Perhaps it'd be best to emphasize that this patch is for Android/x86 and whoever is doing Android/ARM later can come in and add those small differences to get it working for them. |
Can write me how you set up the toolchain? Then I might be able to help you with the linker problem.
We're using |
|
@dawgfoto, perhaps I'm doing something wrong, I'll look further into the linker command I'm running. Rainer pointed out to me on the newsgroup that the reason it doesn't work on Win32/Win64 is because of the extern (C) before main, which can be fixed by removing it or adding phobos.lib manually to the command line invocation. In any case, I follow the convention established by Win32 and Solaris in stdio.d, so if those work, stderr should work on Android also, assuming I can get everything set up right. |
https://github.com/D-Programming-GDC/GDC/blob/master/gcc/d/patches/patch-versym-os-4.9.x
No, it would be best for you to do: |
|
Also topical: https://d.puremagic.com/issues/show_bug.cgi?id=11666 |
|
@ibuclaw, no thanks, no point in adding all that architecture boilerplate when there's no Android/ARM port or dmd ARM backend right now. Yes, I know gdc/ldc have some ARM support, but they also have not upstreamed any Android patches. Whenever someone gets Android/ARM going, likely me after Android/x86, they can add in those small tweaks for Android/ARM then. I agree with you that in the medium-term we'll need to clean up the ports into their own directories, but right now there are few enough of them that that's likely premature. |
|
The ARM support is solely a library problem. Nothing to do with the frontend. |
|
Is that addressed at me? The fact remains that there is no Android/ARM port of D and no proposed patches for such a port. When that changes, we can address it. |
|
The general rule for druntime headers is that definitions should always be restricted to the versions they are actually verified. It is much easier to address an undefined symbol or a "not implemented" static assertion than it is to debug runtime problems caused by wrong constant values or strict layout mismatches. Walter is quite adamant about this - and rightfully so, as I saw over and over again when working on the MinGW and ARM ports. |
|
In other words, ahem, address it now please to make someone's life easier in the future. |
|
@klickverbot, that is what I'm doing with this patch, as everything is within version(Android) blocks. As for all the combinations of arch/OS that are possible, I don't think we can anticipate all the subtle differences for every one ahead of time and add blocks in for them nor put boilerplate everywhere to warn about such possible differences, as Iain suggests. If we were going to do that, we'd already have a ton more boilerplate in druntime, to account for all the subtle differences in linux/mips or Solaris/sparc. Better to list what platforms we've actually had some verification on in some README somewhere and anyone who wants to try a new combination of arch/libc/OS knows they will have to put in some compiler/druntime/phobos work first, ie it won't "just work." @ibuclaw, I'd rather make my life easier now. ;) Whatever I do to account for future architectures is going to be incomplete or, in the case of your example, add unnecessary boilerplate. This sounds like premature optimization to me, let's worry about it when there's an Android/ARM port. I'd understand your concern more if there were already some Android/ARM patches and you were worried about the two interacting, but the fact is there wasn't a single Android patch submitted till this one. I'd rather someone who's actually testing on ARM add those version blocks later, than try to anticipate them now. |
|
I just look at basic support in LDC and have a question about |
|
Unnecessary boilerplate is already an problem in druntime, and it happened despite forewarnings from myself of this nearly 2 years ago. All you can do for the time being is continue with the trend. As for your README idea - it's nice to tell people that it just won't work... granted that they actually read the README. Oh, and good luck knowing where you have to make changes if you are not familiar with the druntime structure... By the way, how did you know where to put in version(Android) in druntime? Oh wait, there are static asserts that halt compilation and tell you "Please add your target-specific bits here Mr. Porter". :o) |
|
@joakim-noah The |
|
@redstar GDC defines |
|
@jpf91 Thanks. LDC head defines |
|
@redstar, that is a good question. This Android patch assumes that only version(Android) is defined, that version(linux) is not defined. The reason I wanted to get this core.stdc patch in early is to hash out all these labeling issues early, so that I can use whatever versioning convention we come up with for subsequent patches. I think a good argument can be made that Android is sufficiently different that only version(Android) should be defined, but I don't care that much and will do whatever the consensus is. As for putting in more version blocks by arch, please read below. @ibuclaw, I see, so you're against "unnecessary boilerplate," yet you want me to add more? ;) I think we're all agreed that where there is OS-specific or arch-specific code, it should be versioned out. The current discussion is how to handle OS-specific and possibly arch-specific code. Would you have us add a "version(x86) else static assert" block inside every single OS block that we have now, whether version(linux) or version(solaris), simply because we haven't tested on every possible arch/OS combination yet? Or only inside the version(OS) blocks that we suspect might vary by arch, as I pointed out that fenv.d might on Android? I'm honestly unsure which you are suggesting. If the former, it seems premature and overkill to me. As for how I started the Android port, I grepped src/core/stdc for all instances of "linux" initially and then "version" later, once I saw that there were version(Windows){...} else{} blocks in some of the files. I never hit a static assert while I was porting, though that's because I defined both the linux and Android symbols in dmd when I started porting. ;) In any case, that is an example of version(OS) blocks helping me, which we both agree are useful, not version(OS){version(arch), which is what you're proposing. |
|
@joakim-noah: If you can verify for a given set of constants that they are architecture-independent by a closer look at the headers (many indeed are), then it is of course fine to not include an additional check for it. Usually, you can take a very reasonable on this based on either preprocessor conditionals in the file itself or the location/name of the file source file. So, for example the It might also be useful to know that Martin (@dawgfoto), Iain (@ibuclaw), Johannes (@jpf91), Kai (@redstar) and me probably represent a big portion of the people who have worked on porting the various compilers to other platforms (especially non-x86 architectures) so far. So, if the consensus is that versioning in OS headers should be handled carefully, it's not because we want to make our own life harder, but because it has indeed helped us when working on the respective ports. Your experience might be different due to the rather unfortunate GNU/Linux vs. Android situation, but this doesn't change the fact that correctly written/restricted definitions are immensely helpful when working on other ports. At least, I experienced exactly the same benefits as Kai described when working on the LDC MinGW and ARM ports. |
|
@klickverbot, if you just want the version(arch) statements in a few places, I still think it's premature. There's a rat's nest of #ifdefs in most of these headers: it is pointless to try and guess where version(arch) blocks might be necessary for a particular OS at some future date. As it is, very little of this is tested and so already might break in all kinds of ways once somebody actually tries to use these stdc functions. For Android/x86, I do some sanity checks here and there, which is how I stumbled across that stderr issue, there's nothing of the sort for Android/ARM. Trying to guess where these issues might crop up and stub in version(arch) statements in those places seems pointless to me. Better that it break when somebody is doing the Android/ARM port and they figure it out for themselves. If we were really to stub in what you want everywhere, there would be tons of such version(linux){ version(x86) blocks all over druntime. Is that what you really want and if so, why hasn't it been done already? We all agree that version(OS) and version(arch) blocks are very helpful where there is a clean way to isolate them. Where I disagree is that the combination, version(OS){version(arch), should either be done everywhere or based on discretion, as you suggest. This is just overhead with little likelihood of helping much. I think you are overgeneralizing from your good experience with the former to push for the latter. |
It took me several days of debugging to get the initial druntime MIPS port up and running.
All we're asking for is to put the relevant declarations in a version (X86) block. That brings up the topic, that version (linux) and version (Android) don't model the problem correctly. |
|
@dawgfoto, a new port is going to take days or weeks of debugging no matter what, there is no arguing that. You are suggesting instead that the initial linux/X86 programmer do your work for you, which is impossible. I realize that you would like some help with new ports where possible, but this just adds red tape for porters, red tape that was not foisted on previous porters and the version(X86) blocks that you want have largely not been added inside the existing version(linux) blocks to this day. Why should porters to new OSs be held to higher standards than the existing druntime is held to today? I have no problem with sticking in some version(X86) blocks here and there, based on where I think ARM might differ, especially since I will be trying this out on Android/ARM myself at some point. I just don't think this policy is generally worthwhile and that's proven by the fact that druntime doesn't follow it. :) As for the glibc/bionic divide, is there anybody running bionic on non-Android linux or glibc on Android? If not, there is no point on insisting on the distinction. Someone else can come in later and clean it up if necessary. Does anybody have a strong desire to have Android define both version(Android) and version(linux), as gdc apparently does now? I'm leaning towards only defining the former on Android, but don't care too much about which we choose: we just need to decide. |
|
@joakim-noah: Sorry, but I think we'll have to disagree here. There is no arguing that a port takes work, but also not that explicit error messages make the process much, much easier than silently broken runtime behavior. And adding the necessary version blocks upfront takes only a minimal amount of work. And yes, druntime started out as very x86-centric, but for new contributions, we have been requiring "defensive" use of version statements for a while. If I remember correctly, there should be plenty of examples for e.g. catch-all So, it's not as if we held work on new ports to higher standards than other contributions at all. It's just that recognizing a problem in the existing codebase doesn't magically resolve it – ideally, the whole existing codebase would be scrutinized for such issues, but nobody has volunteered to do that so far. |
|
@klickverbot, there are fundamentally three different approaches here:
I think 3) is the best approach and is in fact the approach that druntime has taken. 1) is just a lot of useless boilerplate, as any new linux/sparc porter should know that he'll have to check all the version(linux) blocks for arch differences. Reminding him of that in every instance adds little. If adding such version blocks upfront takes so little time, why hasn't it been done all throughout druntime already? As for 2), expecting the first few porters to do the work for future porters is not realistic. Of course, if the first porter happens to know that something will vary by arch, one would hope he would stick a version(x86) in there to guide future porters, but he shouldn't be expected to track all these OS/arch differences down through the headers to make sure. That is the work of future Android/ARM, linux/ARM, or Android/mips porters. Put this burden on the initial porter and you will soon find that there are no initial porters. |
|
The way I see it, there are three options:
I think 1) is the best route, the cleanest for now, and we can always refactor into 3) eventually. |
|
Ian, to define both |
|
That's fine @d@da@dawg^H^H^H @MartinNowak - I don't have a problem with this pull per say. I do have a problem with keeping it this way when people actually want to start getting the Android port working. |
|
Looks like the auto-tester has been building this pull request for the last week without any problems and the discussion has been over for 10 days, someone want to commit? |
|
Ping. Looks like this pull has entered the Bermuda triangle of D pull requests, like the more than dozen pulls that lie waiting before it. I think discussion on this pull is over. If it isn't going to be merged, perhaps I should just close it? |
|
As for the claim that there are two more changes to be made to this pull, I disagree. This pull is based on Android API 9, whose errno.h headers show no differences across the supported platforms. And it isn't my job to fix fpos_t or any other declaration that is currently incorrectly defined in druntime: I noted the mistake and moved it inside a version block. Those who actively maintain those platforms can fix the problems, as they know them better and can track down the differences easier. |
Right, only one left.
I'm kind of tired to reiterate the same argument. You're adding declarations that vary across different architectures so they are wrapped in version arch blocks. Whether or not your version of Android knows those additional architectures, I don't care. |
|
Sigh, I'm getting tired of this argument too. Android API 9 "knows" all the same architectures as any other future API version, ARM, X86, MIPS. For whatever reason, the errno.h headers do not vary across architectures in Android API 9. Looking back at our discussion on your pull request to modify this one, it appears the errno.h headers changed with API 13 for the mips architecture, an arch practically nobody uses. As I mentioned earlier, if you think this version-dependence is intrinsic to the linux kernel, why is it that the current stdc/errno.d does not have version(arch) blocks for the various architectures inside the linux block? We are quibbling about nothing. I am uninterested in tracking down exactly how one header file that nobody cares about varies for an arch that nobody uses. Clearly you feel the same, as there is no version(arch) block inside the version(linux) block in errno.d from the current druntime. Can we just continue ignoring this silly header and move on? :) |
|
It's just adding a few lines and I already did it for you.
That's the problem, druntime was written X86 centric and now we have to make the existing code architecture aware but you insist on adding code that only works on some architectures. |
|
OK, done. I guess I haven't been bitten by cross-arch porting as you have, so I wasn't as worried about it. Sorry I took so long to come around, I thought it was needless but perhaps you're right that it's better to be safe. |
No problem, it took me quite a while to accept this too. The current approach isn't perfect but it's simple and safe. |
First set of stdc changes for Android/x86
|
I also filed a bug report for the wrong fpos_t. |
You cannot actually link against core.stdc in druntime using these patches, as a few missing symbols remain, but since most of it is just declarations for symbols in libc, you should be able to use this patch as D "headers" to write basic command-line utilities for Android/x86. The few symbols that would be provided by core.stdc, such as stderr/stdin/stdout on Android, wouldn't work in that scenario.