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

COFF support for Win32 #3843

Merged
merged 4 commits into from
Aug 17, 2014
Merged

COFF support for Win32 #3843

merged 4 commits into from
Aug 17, 2014

Conversation

rainers
Copy link
Member

@rainers rainers commented Aug 2, 2014

As this comes up in the NG now and then, these are the compiler patches to build COFF files for Win32.

  • use command line option -m32coff
  • it defines versions CRuntime_DigitalMars and CRuntime_Microsoft (and CRuntime_GNU) as it is no longer possible to detect the C runtime from the OS. This can also replace all the similar adhoc versionings in druntime/phobos.
  • C++ interfacing isn't adjusted yet, the rest of the test suite seems to succeed

The corresponding druntime and phobos changes are here (mostly dealing with the CRuntime versions):

https://github.com/rainers/druntime/tree/coff32
https://github.com/rainers/phobos/tree/coff32

If there is a chance of this getting merged, I can make them pull requests, too.

@Trass3r
Copy link
Contributor

Trass3r commented Aug 2, 2014

@redstar What do you think about the version names?

@ghost
Copy link

ghost commented Aug 3, 2014

I'd label this with "New Feature" and "Big Pull/Major Changes", but I can't seem to add new labels.

@@ -40,7 +40,11 @@
#include "doc.h"
#include "color.h"

#include "backend/cdef.h"
Copy link
Member

Choose a reason for hiding this comment

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

This header shouldn't be leaking into the frontend. I think the usual way is to make our own types and translate in glue/backconfig?

Copy link
Member Author

Choose a reason for hiding this comment

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

I remember having been in #include hell regarding cdef.h before (https://github.com/D-Programming-Language/dmd/pull/3542/files). I guess introducing the output format in the options of the frontend won't fly with Walter, too. I'll convert it to an "bool coff;" there.

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind having the output format in the frontend, but any backend headers will need to be special-cased in ddmd.

@yebblies
Copy link
Member

yebblies commented Aug 3, 2014

I think this is definitely worth doing. I think the first step is to get the CRuntime_* versions in, preferably matching the existing identifiers that I assume gdc/ldc have.

@Trass3r
Copy link
Contributor

Trass3r commented Aug 3, 2014

Ldc's used DMC_RUNTIME vs. MSVC_RUNTIME so far.

@redstar
Copy link
Contributor

redstar commented Aug 4, 2014

I think the version names are ok. I like to see standard names instead of the ad hock ones spread around druntime/phobos.

@ibuclaw
Copy link
Member

ibuclaw commented Aug 4, 2014

I'm not a fan of the names. What would the difference be between CRuntime_GNU and MinGW? I'm assuming none.

@rainers
Copy link
Member Author

rainers commented Aug 4, 2014

Ldc's used DMC_RUNTIME vs. MSVC_RUNTIME so far.

All uppercase has little precedence in existing version identifiers.

@rainers
Copy link
Member Author

rainers commented Aug 4, 2014

I'm not a fan of the names. What would the difference be between CRuntime_GNU and MinGW? I'm assuming none.

CRuntime_GNU is currently just meant as a place holder for anything else but DigitalMars and Microsoft. I expect there should be Posix, Bionic and others.

@ghost ghost added the Enhancement label Aug 4, 2014
@WalterBright
Copy link
Member

I've been referring to coff in the source code as "mscoff", not "coff", as it really is not the usual coff format. Microsoft has diverged it substantially. I'd prefer the name mscoff was used instead.

@rainers
Copy link
Member Author

rainers commented Aug 12, 2014

I've been referring to coff in the source code as "mscoff", not "coff", as it really is not the usual coff format. Microsoft has diverged it substantially. I'd prefer the name mscoff was used instead.

I have renamed everything from "coff" to "mscoff".

@@ -64,6 +64,8 @@ void out_config_init(
config.memmodel = 0;
config.flags |= CFGuchar; // make sure TYchar is unsigned
tytab[TYchar] |= TYFLuns;
bool mscoff = model & 1;
model &= ~0x1f;
Copy link
Member

Choose a reason for hiding this comment

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

Replace with:

model &= 64 | 32;

and add the 1 bit to the documentation of model parameter

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

I think this is the ugliest part of this PR. I always wondered why the backend configuration is not a struct declared in a header file. Right now, the prototype for out_config_init is duplicated in msc.c making it inconvenient to add another option because other backends will need to be updated, too.

WalterBright added a commit that referenced this pull request Aug 17, 2014
@WalterBright WalterBright merged commit a7a3c78 into dlang:master Aug 17, 2014
@rainers
Copy link
Member Author

rainers commented Aug 17, 2014

Thanks for merging. Actually, I expected more discussion regarding the CRuntime version identifiers...

@WalterBright
Copy link
Member

I'm not up for more bikeshedding about internal identifier names :-)

@yebblies
Copy link
Member

Thanks for merging. Actually, I expected more discussion regarding the CRuntime version identifiers...

So was I, but we have up until the next release to settle on something with GDC and LDC.

@ibuclaw
Copy link
Member

ibuclaw commented Aug 20, 2014

Nothing actually touches shared code (except mars.h) so I'd be inclined to ignore that the identifier even exists.

The only time I'd start raising eyebrows is if you decide to actually use it in druntime/phobos.

@yebblies
Copy link
Member

It may not actually be shared code, but I assume you're going to mirror the addPredefinedGlobalIdent changes, right? And of course they'll be used in druntime/phobos.

@ibuclaw
Copy link
Member

ibuclaw commented Aug 20, 2014

@yebblies it makes no sense to anything other than CRuntime_GNU - but this is no different than plain old GNU.

@ibuclaw
Copy link
Member

ibuclaw commented Aug 20, 2014

Or at least, I can't immediately see what the meaning of these are. For instance, would Bionic still be CRuntime_GNU?

@ibuclaw
Copy link
Member

ibuclaw commented Aug 20, 2014

What about OSX, which uses Clang?

@ibuclaw
Copy link
Member

ibuclaw commented Aug 20, 2014

What about MinGW? Are the library changes going to break support for that / cause more sporking of the D library? (I would have thought the answer be yes)/

@WalterBright
Copy link
Member

Clang and gcc on OSX, as far as I can tell, are compatible enough that there's no need for D to distinguish between them.

If there's impetus for MinGW on Windows, then it'll need another identifier for that one.

@ibuclaw
Copy link
Member

ibuclaw commented Aug 21, 2014

GDC only supports MinGW, and there are (alpha/untested) binaries available. I don't think it needs a new identifier, it already has a suitably named one - version(MinGW)

@rainers
Copy link
Member Author

rainers commented Aug 22, 2014

According to the spec, version GNU is defined by the compiler GDC, it is not explicite about the target C-runtime that the executable is going to be linked with. In cmbination with the OS this is only a problem if there are multiple C libraries that might get used on the same platform. With using the prefix "CRuntime_" I wanted to make that more explicite.

MinGW and Cygwin are version identifiers probably meant for the same purpose, though the description "The MinGW environment" doesn't make that very clear. Taking these as precedence for other runtimes, "DigitalMars" and "Microsoft" would be natural choices, but "DigitalMars" is already used for the compiler.

If CRuntime_GNU just makes things more ambiguous I'm in favor of removing it. The current patches only use CRuntime_DigitalMars and CRuntime_Microsoft to make the distinction for the Windows platform.

@ibuclaw
Copy link
Member

ibuclaw commented Aug 22, 2014

Well, the only runtimes I can list support (or potential) for are:

  • mingw
  • cygwin
  • freebsd
  • glibc
  • uclibc
  • bionic

And the only pair that has come up recently (Linux glibc/bionic) I was never much in favour of meshing the two together in the same file anyway, but that's what you seem to be doing here with Windows.

version(Win32)
{
    version(DMC)
    /* ... */
    else version(Microsoft)
    /* ... */
    else version(MinGW)
    /* ... */
    else version(Cygwin)
    /* ... */
}
else version(Win64)
{
    /* repeat */
}

@rainers
Copy link
Member Author

rainers commented Aug 23, 2014

The API differences between Win32 and Win64 are usually smaller than the differences between C runtimes (DMC vs. Microsoft). So I don't think it is sensible to distinguish between 32-bit and 64-bit at the top-most level.
I am not convinced one fixed hierarchy for OS, processor, architecture, C-Runtime etc. is maintainable, because they are interleaved in non-orthogonal ways. It's a judgement call for each and every API subsystem.

@rainers
Copy link
Member Author

rainers commented Aug 23, 2014

Well, the only runtimes I can list support (or potential) for are:
mingw, cygwin, freebsd, glibc, uclibc, bionic

Wouldn't it be better to make it clear in the version identifier that the C runtimes are meant, not the build environment (though that is usually coupled to some extent)?

@ibuclaw
Copy link
Member

ibuclaw commented Aug 26, 2014

The API differences between Win32 and Win64 are usually smaller than the differences between C runtimes (DMC vs. Microsoft). So I don't think it is sensible to distinguish between 32-bit and 64-bit at the top-most level.

Maybe give DMD a DigitalMars_* vendor-specific version then? Only DMD will ever use dmc runtime. version(Windows) could then assume to be always Microsoft.

I'm only suggesting as it is nothing short of 'rather odd' to have an identifier for runtimes. Not even C compilers go to the bother of doing that.

@ibuclaw
Copy link
Member

ibuclaw commented Aug 26, 2014

As for MinGW, there's only a couple of places that I know of where it deviates from Microsoft signatures (ignoring ABI compat). Maybe things should be fixed up there so we're not piggy backing off the fact that most of the Win64/Win32 declarations in druntime match that in MinGW runtime.

@rainers
Copy link
Member Author

rainers commented Aug 27, 2014

I'm only suggesting as it is nothing short of 'rather odd' to have an identifier for runtimes. Not even C compilers go to the bother of doing that.

You might be right that the runtime is very much coupled to the compiler, so instead of CRuntime_... we could use an identifier that specifies the C compiler that is used to build C files we are linking to. That fits slightly better when using functions expected to exist by the compiler, like _alldiv for the long integer mul/div operations.

But we don't have an explicite identifier for that yet. It might be best to match what compiler vendors are using themselves. Here is a list of compiler identifiers: http://sourceforge.net/p/predef/wiki/Compilers/

For Windows, this suggests using __DMC__ and _MSC_VER_, maybe omitting underscores or adding some prefix, say "CC_DMC" and "CC_MSC".

@ibuclaw
Copy link
Member

ibuclaw commented Aug 27, 2014

There is a rather distinct difference between coupled compiler/compiler runtime and C (std) runtime.

The latter has no coupling at all.

@rainers
Copy link
Member Author

rainers commented Aug 27, 2014

There is a rather distinct difference between coupled compiler/compiler runtime and C (std) runtime.
The latter has no coupling at all.

I cannot tell about linux/mac, but on windows, they are coupled by being in the same library (e.g. snn.lib or libcmt.lib).
I'm not sure if you are now proposing that compiler and C runtime are uncoupled, so the latter cannot be identified by the existing version identifiers?

@ibuclaw
Copy link
Member

ibuclaw commented Aug 28, 2014

I'm not sure if you are now proposing that compiler and C runtime are uncoupled,

I'm not proposing. I'm just saying what has (or should) always been the case.

so the latter cannot be identified by the existing version identifiers?

There is no standard macro in C.

There are vendor macros in GDC though - GNU_GLibc, GNU_UCLibc and GNU_Bionic. You are free to use these if you absolutely must, but I'd rather druntime stay clean of them as it opens a can of worms for maintenance - eg: 45 version code paths just for Linux (x86/GLibc, x86_64/GLibc, ARM/GLibc, ...). I had proposed it once before to have a /ports directory with Bionic bindings for core.stdc - this didn't happen though.

ibuclaw pushed a commit to ibuclaw/dmd that referenced this pull request Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants