Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add predefined global version for Glibc #4111

Merged
merged 2 commits into from
Dec 1, 2014
Merged

Add predefined global version for Glibc #4111

merged 2 commits into from
Dec 1, 2014

Conversation

joakim-noah
Copy link
Contributor

This is necessary to start separating out linux and glibc APIs in druntime. Right now, version(linux) means linux+glibc APIs in druntime/phobos, whereas afterwards linux will refer to linux kernel APIs, CRuntime_Glibc to glibc APIs, and CRuntime_Bionic to bionic APIs, at least in druntime and phobos. I don't care what names are actually used, this is just what seemed to make sense. I've updated my pull which added Android support to dmd, so you can see how they'll be defined. I've started updating druntime to use these new versions, the changes should be completely transparent to users.

I'm not sure why the C runtimes were left out of the list of predefined global versions in cond.c, so I added them. Also, I noticed that D_InlineAsm isn't in the list either, presumably because it predated 64-bit support and is only around for legacy reasons, so I added a comment to that effect. Let me know if it should be added to the list in cond.c anyway.

@yebblies
Copy link
Member

yebblies commented Nov 4, 2014

They are automatically predefined if they start with D_.

@joakim-noah
Copy link
Contributor Author

You're right, I see that now. Should I take out the 12 "D_*" names that are in the list?

@yebblies
Copy link
Member

yebblies commented Nov 4, 2014

I don't see much point in that. I added them last year when I copy-pasted the list from the spec. It might be nice to add an assert in addPredefinedGlobalIdent that asserts it's in the reserved list.

@joakim-noah
Copy link
Contributor Author

Added another commit to get rid of the last usage of std.c in dmd, from the samples.

@WalterBright
Copy link
Member

I'm not really understanding this. Why have CRuntime_Digitalmars when it would be defined if and only if Windows and X86 was defined? Same for the others.

C compilers tend to have an absolute blizzard of predefined names (gcc has 240 the last time I checked), most of which are alternate names for others, a pile of fairly unused ones, etc.

All proposals for new ones for D need to not be "it would be nice if..." and more like "it is absolutely necessary that...".

@yebblies
Copy link
Member

yebblies commented Nov 8, 2014

I'm not really understanding this. Why have CRuntime_Digitalmars when it would be defined if and only if Windows and X86 was defined? Same for the others.

The dmc runtime is not the only one available on windows/x86.

@rainers
Copy link
Member

rainers commented Nov 8, 2014

Adding CRuntime_DigitalMars and CRuntime_Microsoft was absolutely necessary to distinguish building against dmc or msc for Win32.

I wasn't aware of this list of predefined versions when adding them, I guess I assumed calling addPredefinedGlobalIdent would be enough. But that doesn't make them reserved on any other platform.

Adding other version identifiers was expected to happen, I think it is necessary when building against different runtimes on one platform.

#else
VersionCondition::addPredefinedGlobalIdent("CRuntime_GNU");
#elif TARGET_LINUX
VersionCondition::addPredefinedGlobalIdent("CRuntime_Glibc");
Copy link
Member

Choose a reason for hiding this comment

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

Glibs might be the better identifier, but why nuke it for other platforms? Isn't this the usual runtime for all non-Windows platforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Glibc is the GNU C library. It is the default on linux, but isn't used on Android, OS X, the BSDs, Solaris, etc.

Copy link
Member

Choose a reason for hiding this comment

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

What is used on the BSDs? Is it interface compatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They have their own locally-grown libc versions. Android's bionic pulls heavily from them, as does OS X I believe. I don't know exactly how compatible they all are, probably varies.

@joakim-noah
Copy link
Contributor Author

@WalterBright, what is the alternative you'd prefer, defining these in the makefiles for druntime/phobos? I was just following Rainer's lead with these definitions, I don't really care where they go, whether here or in the makefiles.

@dnadlinger
Copy link
Member

Why have CRuntime_Digitalmars when it would be defined if and only if Windows and X86 was defined?

and you are building with DMD, not LDC or GDC.

@WalterBright
Copy link
Member

… and you are building with DMD, not LDC or GDC.

So 'and' it with DigitalMars.

I know what happens when there's a blizzard of overlapping predefined names. They all get misused. They need to be orthogonal. New names that are various combinations of existing names should not be added.

Adding CRuntime_DigitalMars and CRuntime_Microsoft was absolutely necessary to distinguish building against dmc or msc for Win32.

They are not 'absolutely' necessary. Each can be built out of existing predefined names.

@yebblies
Copy link
Member

yebblies commented Nov 9, 2014

They are not 'absolutely' necessary. Each can be built out of existing predefined names.

But the compiler knows exactly which cruntime it's targeting - why not communicate this in a simple and clear way to the program being compiled?

We have versions for both BigEndian and LittleEndian. Are you saying it would be better if only one of these was defined?

There is (or was?) plenty of code in druntime/phobos that assumed DigitalMars/win32 means the dmc runtime. With support for the msvc32 runtime, all of this code needed updating.

@joakim-noah
Copy link
Contributor Author

Each can be built out of existing predefined names.

This is not true, as both CRuntime_DigitalMars and CRuntime_Microsoft have all the same existing predefined versions defined alongside, ie Windows, X86, DigitalMars for dmd, etc. The only way to tell the difference is whether the user passes the -m32mscoff flag or not, which is how the dmd frontend decides on one or the other Windows C runtime right now.

Now, as I mentioned earlier, we could instead set these C runtime versions in the makefiles for druntime/phobos, as we have to pass them the -m32mscoff flag anyway. We can check for that flag and then set the corresponding C runtime in the makefile. However, I just realized that this won't account for users compiling against druntime imports- for example, in core.stdc.stdio- so they will have to do the same for their code, ie add the -m32mscoff flag and define CRuntime_Microsoft themselves.

The way Rainer set it up, which this pull follows along with, users simply have to add the appropriate dmd flag and the versions are all taken care of for them. We just need to make a call on which is better, providing that in the frontend or making users define them.

@joakim-noah
Copy link
Contributor Author

Updated this pull to remove CRuntime_Bionic, as Rainer asked for.

@joakim-noah joakim-noah changed the title Add Glibc and Bionic predefined global versions Add Glibc predefined global version Nov 9, 2014
@joakim-noah joakim-noah changed the title Add Glibc predefined global version Add predefined global version for Glibc Nov 9, 2014
@joakim-noah
Copy link
Contributor Author

What's the hold up on this pull at this point? It's a minor change and all the ins and outs have been discussed. Somebody just needs to make a decision.

@rainers
Copy link
Member

rainers commented Nov 23, 2014

They are not 'absolutely' necessary. Each can be built out of existing predefined names.

Which one would that be for an MS-COFF32 build?

I'm inclined to merge this as it fixes the existing CRuntime_ versions to be reserved. I trust joakim that Glibc is the better name than GNU (which wasn't in any release yet).

@joakim-noah
Copy link
Contributor Author

As I noted originally, I don't really care what names are used, but after seeing the discussion on Rainer's pull that added CRuntime_GNU, I figured maybe this name is better.

@ibuclaw, want to chime in on the name? I plan on using it throughout druntime and phobos, as shown in the linked pull requests.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 23, 2014

@joakim-noah - Other than thinking that it will be impractical to implement and we already have version identifiers can achieve the same goal.

There may be a strong common-sense behind the idea of: We are compiling for Linux, so the C Runtime library must be Glibc. But it still just feels wrong to look at in the compiler.

Matters of C runtime - should you wish to add a glorified shortcut for version (DigitalMars && X86) - is better to have the batten passed over to D runtime, ie core.stdc.config.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 23, 2014

is better to have the batten passed over to D runtime, ie core.stdc.config.

With this in mind, should you fork druntime, eg: minilibd, or go for bare metal (no C library). Then the compiler is not emitting false information.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 23, 2014

But the compiler knows exactly which cruntime it's targeting - why not communicate this in a simple and clear way to the program being compiled?

s/the compiler/dmd and maybe ldc too/ :-)

@joakim-noah
Copy link
Contributor Author

Other than thinking that it will be impractical to implement and we already have version identifiers can achieve the same goal.

Please elaborate: what is impractical about it and which version identifiers could we use instead, especially on Windows as Walter claims? On linux, you do have a point because we'll have the Android identifier also, as shown in #3643, but that is a separate pull and we can decide that later. This pull currently makes no mention of Android.

There may be a strong common-sense behind the idea of: We are compiling for Linux, so the C Runtime library must be Glibc. But it still just feels wrong to look at in the compiler.

Well, the idea is to be able to pick different C runtimes for linux, just as you can do with dmd/Windows and gcc/linux. Take a look at the link in my original post that shows how bionic would be added there. Even if we don't define bionic, you could add other C runtimes there.

Matters of C runtime - should you wish to add a glorified shortcut for version (DigitalMars && X86) - is better to have the batten passed over to D runtime, ie core.stdc.config.

But CRuntime_DigitalMars is not a glorified shortcut, as currently DigitalMars is defined for both OMF and COFF output and according to the docs simply means that dmd is the compiler. The only way to differentiate using DigitalMars would be to not define it when compiling for the Microsoft C runtime, which would then break its meaning of being tied to dmd.

But the compiler knows exactly which cruntime it's targeting - why not communicate this in a simple and clear way to the program being compiled?

s/the compiler/dmd and maybe ldc too/ :-)

Are you saying gdc can't do this? According to your previous comment, gcc already has this built in and would be trivial to enable.

@ibuclaw
Copy link
Member

ibuclaw commented Nov 24, 2014

But CRuntime_DigitalMars is not a glorified shortcut, as currently DigitalMars is defined for both OMF and COFF output and according to the docs simply means that dmd is the compiler. The only way to differentiate using DigitalMars would be to not define it when compiling for the Microsoft C runtime, which would then break its meaning of being tied to dmd.

But OMF and COFF are binary formats, which have nothing to do with the runtime library (???). It just so happens that CRuntime_DigitalMars is for X86-only and uses OMF.

Recently there was an addition for ELFv1 and ELFv2 version identifiers (not yet in documentation). You can quite happily add OMF and COFF to that list too to fill that void.

@joakim-noah
Copy link
Contributor Author

But OMF and COFF are binary formats, which have nothing to do with the runtime library (???).

They do in this case, because the Digital Mars C Runtime is only provided in OMF format and the Microsoft C Runtime in COFF format. I never suggested adding OMF and COFF because that's besides the point, which is that DigitalMars is currently defined for everything compiled by dmd, so it cannot be used to differentiate between binary formats or C runtimes.

@joakim-noah
Copy link
Contributor Author

btw, are there plans to provide COFF druntime/phobos libraries with the next release? Once the zip file is split up for each OS, as Martin has redone the downloads page to surface, there should be no complaint about download size.

@joakim-noah
Copy link
Contributor Author

I've been thinking about this pull some more, as I'd like to move forward with separating out linux and Android into linux, CRuntime_Glibc, and CRuntime_Bionic in druntime and phobos. I really don't need dmd to define any C runtimes to do this, as Android will be defined and I can use that to set CRuntime_Bionic and CRuntime_Glibc, ie !version(Android), in the various config files. However, this assumes that bionic is only used on Android, but bionic outside Android is probably sufficiently rare that this is a workable assumption.

I simply proposed this pull, which only changes CRuntime_GNU to CRuntime_Glibc and fixes up some stuff to formalize the Windows C runtimes which Rainer added, because Rainer's win32 COFF pull seemed to set the precedent of defining C runtimes in dmd, which Walter seems to have changed his mind about. I did not mean to get caught up in this debate about whether C runtimes should be defined in dmd at all.

If Walter is so against it, Iain's suggestion will work: we can only define OMF and COFF in dmd instead of CRuntime_DigitalMars and CRuntime_Microsoft, then add the following in the config files in druntime:

if version(Windows)
{ 
   if version(OMF)
        version = CRuntime_Digitalmars;
    else version(COFF)
        version = CRuntime_Microsoft;
}   

There is already precedent for adding binary formats, as Iain noted that Kai recently added ELFv1 in dmd, unless somebody changes their mind about that too. ;)

The only drawbacks to this approach are that it assumes that CRuntime_Microsoft is COFF and CRuntime_DigitalMars is OMF, which as Iain says is true in practice but not in theory, and we'll have to import more config files in druntime and phobos to make these available, though we could avoid this by modifying druntime/phobos to directly use version(OMF) version(Windows) instead of version(CRuntime_DigitalMars).

Really all we need is a way for dmd to signal to druntime and phobos what C runtime to use, so OMF, COFF, and Android can take the place of CRuntime_DigitalMars, CRuntime_Microsoft, CRuntime_Glibc, and CRuntime_Bionic, if it's decided that we want to keep C runtime names out of dmd. As I said from the beginning, I don't care what names are actually used, but we do need some way to send this info from the compiler. If we ever want to support more alternate C runtimes, we will probably need to add them directly in the compiler, but since we will only support two alternate C runtimes in the near future, ie 32-bit MSVC on Windows and bionic for linux, we can kick that can down the road.

@rainers and @WalterBright, if you both say you're okay with moving forward by defining OMF and COFF instead, I'll update these pulls to remove the C runtimes from dmd and define them in the druntime config files instead.

@rainers
Copy link
Member

rainers commented Nov 26, 2014

if version(Windows) { if version(OMF) version = CRuntime_Digitalmars; else version(COFF) version = CRuntime_Microsoft; }

This doesn't work in an imported config module, because the defined version is only valid for this module. It would have to be some enum tested with static if.

An alternative to defining the CRuntime versions in the compiler would be to put the definition in the appropriate environment block in sc.ini/dmd.conf as DFLAG command lline options. Compiling with -m32mscoff selects block [Environment32mscoff], thus everything is compiled with the required versions. But this is almost as coupled to the compiler as an internal version, so I doubt it is an improvement. It just adds complications with local ini-files.

Selecting the C runtime via the object file format is a bit strange. The conditions in the druntime select the different API, not the file format. You can link a DLL version of the C runtime without following it's object file format.
If we get more system programming features later like pragma(data_seg) and pragma(data_grp), knowing the object format might be required, but I'd defer defining OMF/COFF until they have some use case.

@joakim-noah
Copy link
Contributor Author

This doesn't work in an imported config module, because the defined version is only valid for this module. It would have to be some enum tested with static if.

You're right, I didn't look at the config files closely so I thought versions could be imported. They'd have to be enums instead, and all the version statements for the various C runtimes would need to be changed to static if.

Selecting the C runtime via the object file format is a bit strange. The conditions in the druntime select the different API, not the file format. You can link a DLL version of the C runtime without following it's object file format.
If we get more system programming features later like pragma(data_seg) and pragma(data_grp), knowing the object format might be required, but I'd defer defining OMF/COFF until they have some use case.

I thought it'd be a way to sidestep the C runtime issue, as Iain suggested, just as druntime used to have version (DigitalMars) version (Win32) blocks in the past to refer to APIs specific to the Digital Mars C runtime, which relied on the fact that dmd happened to only work with the DigitalMars C Runtime on Win32 at the time, but which isn't true anymore. I don't know what can be done with DLLs of different binary formats on Windows, maybe using OMF and COFF instead won't work.

@joakim-noah
Copy link
Contributor Author

Well, since those who'd prefer to keep C runtimes out of dmd have not been able to offer a workable alternative, can someone just go ahead and merge this pull? I'd like to move forward with the linked druntime/phobos pulls, which currently fail the auto-tester only because they use the new CRuntime_Glibc global version name from this pull.

@rainers
Copy link
Member

rainers commented Dec 1, 2014

dmd has a strong dependency on the C runtime: https://github.com/D-Programming-Language/dmd/blob/master/src/backend/cod1.c#L1863
It seems there are no major objections, let's move forward.

@rainers
Copy link
Member

rainers commented Dec 1, 2014

Auto-merge toggled on

rainers added a commit that referenced this pull request Dec 1, 2014
Add predefined global version for Glibc
@rainers rainers merged commit e17631e into dlang:master Dec 1, 2014
@joakim-noah joakim-noah deleted the add_glibc branch December 1, 2014 08:43
@MartinNowak
Copy link
Member

I agree with Iain here, glibc vs. bionic should be defined in druntime.

@joakim-noah
Copy link
Contributor Author

I agree with Iain here, glibc vs. bionic should be defined in druntime.

What about the Microsoft vs Digital Mars C runtimes? This pull primarily deals with those and nobody in this thread has been able to suggest an alternate way to differentiate those two.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 14, 2014

What about MinGW? All these new Runtime version additions and druntime changes has achieved for me is breaking GDC/Windows support - should I eventually move across from 2.065 :)

I think you could have avoided complicating everything had there been simply one addition, eg: MSVC or Microsoft, or made DMC/MSVC runtime a private extension, eg: DigitalMars_DMRuntime, etc...

@rainers
Copy link
Member

rainers commented Dec 14, 2014

All that has actually changed so far is that a number of Win32/Win64 version checks have been replaced by CRuntime_DigitalMars/CRuntime_Microsoft. Judging from the comment in core.stdc.stdio regarding the printf/scanf functions, MinGW builds on top of the MS runtime.
If you define version CRuntime_Microsoft and specialize on MinGW where there is a replacement, I only see one necessary reordering of version in math.d, but that looks suspicious for the previous Win32/Win64 versions, too.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 14, 2014

What about the Microsoft vs Digital Mars C runtimes?

If DMD just supported one, then everything would be fine.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 14, 2014

If you define version CRuntime_Microsoft and specialize on MinGW where there is a replacement

As opposed to defining Windows, and specialise on MinGW (which is what's currently done) ?

@rainers
Copy link
Member

rainers commented Dec 14, 2014

As opposed to defining Windows, and specialise on MinGW (which is what's currently done) ?

The Windows version is not going away, but it describes the target OS, not the target C runtime. MinGW is already treated like a C runtime library selection (though the description at http://dlang.org/version.html#VersionCondition isn't very clear).

I'm not sure where you are driving at. Is the problem that CRuntime_Microsoft is not defined by GDC, but MinGW can use most of the non-DigitalMars-C-Runtime code because it wraps msvcrt for most functions? That this doesn't work anymore because the code is now selected by CRuntime_Microsoft?

I guess it boils down to: Should we treat MinGW as a separate C runtime or a modifier of the Microsoft runtime?

BTW: Grepping druntime/phobos 2.065 for MinGW, it was not correctly applied for Win64 in core.stdc.math and core.stdc.stdlib. That has not changed with the CRuntime versions, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants