[WIP] support building for x86 with MSVC runtime #1168
Conversation
|
Could you please put the LLVM 3.8 related changes in a separate PR? Then I'll merge them asap. |
|
These are actually the changes you just merged with the other PRs. I'll rebase this to remove them here. |
| @@ -352,6 +352,9 @@ llvm::GlobalVariable * IrAggr::getInterfaceVtbl(BaseClass * b, bool new_instance | |||
| SET_COMDAT(thunk, gIR->module); | |||
| thunk->copyAttributesFrom(irFunc->func); | |||
|
|
|||
| // thunks don't need exception handling themselves | |||
| thunk->setPersonalityFn(nullptr); | |||
|
|
|||
There was a problem hiding this comment.
This causes a compile error for LLVM < 3.7. Is this really needed?
There was a problem hiding this comment.
It doesn't seem necessary to compile druntime. Isn't it a valid optimization, though?
There was a problem hiding this comment.
Disabled for LLVM < 3.7 now.
There was a problem hiding this comment.
I think this is the right solution. With LLVM < 3.7, the personality is attached to the landing pad. The thunk does not have a landing pad...
|
The build now fails for LLVM 3.1-3.4. You need not care about this - support for these versions is dropped in next release. |
rainers
force-pushed
the
msvc_x86
branch
2 times, most recently
from
98db446 to
7b3cdf8
Compare
|
Rebasing causes too many conflicts due to changed formatting, so I've reset this to master. This now builds druntime with unittests (but one with exceptions in fibers), and passes all but about 10. |
| LLFunctionType *fty = nullptr; | ||
| if (global.params.targetTriple.isWindowsMSVCEnvironment()) { | ||
| if (!global.params.targetTriple.isArch64Bit()) | ||
| fname = "__CxxFrameHandler3"; |
There was a problem hiding this comment.
Is this hard-coded, and you provide that function in druntime? What happens when there is both C++ and D code in the same executable?
There was a problem hiding this comment.
That function is provided by the MSVC runtime; it's the Visual C++ personality function (for C++ exceptions, not SEH ones).
There was a problem hiding this comment.
kinke's right. The D runtime just raises a C++ compatible exception: https://github.com/ldc-developers/druntime/blob/ldc/src/ldc/eh/win32.d
|
Only a couple of superficial comments – I didn't have time to look into the new EH IR constructs more closely yet, unfortunately. |
| llvm::StructType *BaseClassDescriptorType; | ||
| llvm::StructType *ClassHierarchyDescriptorType; | ||
| llvm::StructType *CompleteObjectLocatorType; | ||
| llvm::DenseMap<llvm::Constant*, llvm::GlobalVariable*> TypeDescriptorMap; // classInfoPtr -> typeDescriptor |
There was a problem hiding this comment.
Is it ok to have these variables as globals? These are in the context in clang IIRC, but the conext is a global in LDC anyway.
There was a problem hiding this comment.
I didn't read through the code in any detail, but since it uses llvm::GlobalVariable, I presume this is specific to each LLVM module? In this case, it should probably go into IRState so it is appropriately reset for compilations that produce multiple object files (there is a 1:1 mapping between IRState and llvm::Module instances).
|
Fixed returning from the catch block. The druntime tests now freeze for 5 unittests, 3 of these also don't pass on Win64. |
| assert(!"MSVC/x86 exception handling not supported with LLVM < 3.8"); | ||
| #endif | ||
| } | ||
| return endbb; |
There was a problem hiding this comment.
The return value is now unneeded, right?
What? According to AppVeyor (using LLVM 3.7), all druntime unittests pass except for a crash in |
|
This already compiles (native x86 LDC + LLVM master build): import core.stdc.stdio;
class MyException : Exception {
int x;
this(int x) { super("MyException"); this.x = x; }
}
struct Cleanup {
int x;
~this() {}
}
void may_throw(ref Cleanup c, bool doThrow) {
printf("may_throw %d\n", c.x);
if (doThrow) throw new MyException(c.x);
}
void may_throw() {}
void main() {
Cleanup a = Cleanup(1);
try {
//Cleanup b = Cleanup(2);
//may_throw(b, true);
may_throw();
} catch (MyException e) {
printf("caught e.x %d\n", e.x);
Cleanup c = Cleanup(3);
may_throw(c, false);
} catch (Exception e) {
assert(0);
} finally {
may_throw(a, false);
}
}When uncommenting the 2 lines in the try-block, we hit an LLVM assertion ( Maybe it's really just a /MD vs. /MT issue for some C parts. |
I see two unittests in core.thread that are waiting to join threads forever. The unittest in core.sync.barrier sometimes freezes, sometimes passes. |
I get that same assertion with the mentioned unittest in core.thread: https://github.com/ldc-developers/druntime/blob/ldc/src/core/thread.d#L5349
I build druntime using a Visual D project. C files compile without any special options (just cl -c /EHsc /Z7). Linking is done with "/NODEFAULTLIB:libcmt libcmtd.lib" though. Sounds like you are linking against the dynamic C runtime library. I've only tried the static library. |
|
Yeah, I've been using LDC for compiling and linking (and Ninja for druntime-ldc-debug). It's working with It somehow needs libcmt[d].lib for that |
|
Oh and btw - your contributions are very much appreciated. 👍 |
It seems you need to link to vcruntime.lib too when linking with msvcrt.lib. |
Thanks :-)
I see too much clang formated code at work that looks horrible... |
Sure, but we just settled on using it set to the LLVM style for our code base, sorry. You may be mad at me for making that decision. ;) I've made very positive experiences with it, by the way, plus the constant style mismatch in the LDC code base (seriously, I now know why pretty much all large projects have a style guide). We should probably formailze this somewhere soon (LLVM coding standards minus grandfathered-in differences (lower case for variables, occasional use of |
|
Btw why is your .exe that much larger than mine (665k vs. 529k)? I've linked it against release druntime/Phobos and |
|
On my co-worker's Win10 box, both .exes don't crash, regardless of whether starting them via Windows explorer or command line, and regardless of whether |
|
This all seems like a deja vu. What happens when the thread-locals are declared in C and referenced by D code? |
|
Must be a linker bug too here. This: #include <stdio.h>
thread_local char b = 42;
thread_local int i = 42;
thread_local char b2;
thread_local alignas(0x40) char data[16];
int main()
{
printf("&b: 0x%p\n", &b);
printf("&i: 0x%p\n", &i);
printf("&b2: 0x%p\n", &b2);
printf("data: 0x%p\n", data);
return 0;
}yields something like this when compiling & linking with MSVC 2015 Update 1, both with and without optimizing with So there is an appropriate padding inbetween |
|
What if you manually added the alignment flag to .tls using editbin? |
|
No luck even after restoring the alignment manually: Note that dumpbin doesn't display the .exe's TLS section alignment (for no other alignment either! I tried them all...). I hope that's not a general Windows/PE limitation (!). |
|
I tried your cpp file with cl Version 19.00.23506 for x86, and link Version 14.00.23506.0, that properly aligns the data for me, too. I might just be lucky, though. The alignment of the .tls is likely not important, because the data is copied into the TLS area anyway. So it's actually the attribute shown with I stepped through the process initialization (LdrpAllocateTls in ntdll) and it looks as if it correctly supports the alignment property of the TLS directory entry (it writes the unaligned pointer just before the start of the TLS area). |
I noticed this, too. Maybe we have slightly different build settings for the runtime lib. My path to LDC is slightly longer, too, but that should not add up to 136 kB. |
|
Ah, there is an |
|
Thx Rainer. Vanilla build, i.e., not modified via editbin: Should clearly be aligned. I'll debug it later too.
I can't believe I'm that unlucky. On two different machines! ;) |
|
Do you think that may be due to an unaligned overall size? |
Looks correct according to the dumpbin output (and works on my system). |
These adresses look unusual large (also the addresses printed at runtime). Do you change the default base address for the exe (/BASE)? It's 0400000h with my x64 builds.
I don't think so. Maybe there is something in your syystem hooked up to the allocation/process initialization that actually changes the base address (virus/anti-virus/debug-allocator). |
|
I've just decided it was time to finally make the move to Windows 10. So here you go, problem solved, everything's aligned now using the identical executables. So it may have really been an outdated ntdll (pre-8.1). My Win7 at work isn't that outdated (lagging a few months behind, company policy...), but has also shown this bug. Pretty lame.
Nope. I've just compiled and linked it with |
|
86f5ecb enables Win32 LDC builds again. This means that adding 32-bit AppVeyor jobs will be a breeze. |
|
We should make sure these TLS issues are documented somewhere (wiki, etc.) we can easily point users to. |
Makes me wonder whether users will hit that issue, too. D code is likely to make more use of thread local data than C++. I checked the definition of IMAGE_TLS_DIRECTORY in the SDKs, and the characteristics field only got the union definition containing the alignment in the Win8.1 SDK. I wonder if an implementation might have been added just then? Actually I did not fnd a documentation of the characteristics field at all, it surely isn't used by this code: http://www.nynaeve.net/Code/VistaImplicitTls.cpp @kinke Have you been using Win 8.0 or 8.1? |
That, and more exceptions than C++, at least in phobos unittests, with some big stack frames. There are a collection of cases with either LLVM or OS X toolchain (ld, dyld) where LDC hit an unreported bug in these areas. |
8.0.
I'm struggling to believe that, but it all points in that direction - it didn't work on a more or less up-to-date Win7 either. So the general recommendation for MSVC users would be to use bleeding-edge stuff only - Windows, VS, LLVM, LDC. :) |
I got hold of an old Win8.0 and a recent Windows7 version of ntdll.dll: they both don't have the same code as my current win8.1 DLL for aligning TLS memory. While I wouldn't care for Win8.0 too much, Windows 7 is still rather popular. I think this could be a rather troublesome issue for clang, too. Is there a way to disable optimizations that rely on alignment of TLS? Alternatively, we could verify TLS base address alignment at startup and issue a warning. |
The latter seems reasonable to me. Even without compiler optimizations, a TLS global of a type T with explicit alignment should be aligned appropriately, so that manual optimizations (cache-line, SIMD etc.) work as expected. |
|
Yes, at least issuing a warning seems to be reasonable, given how popular Windows 7 still is. Wouldn't DMD run into this as well? IIRC it only uses emulated TLS on OS X? |
IMO a warning might not be good enough. I'll have a look at fixing it at startup.
With optlink, it has no idea about aligning TLS at all. For COFF builds, it always sets 16-byte alignment. Obviously this can also fail for 32-bit (default alignment is size of 2 pointers), but dmd doesn't have optimizations that take advantage of alignment. So you don't hit it with the standard lib. |
Hmm, are you thinking about manually moving the TLS block and setting the address in the TIB appropriately? That might just work, though I'm not sure where else Windows might store that address. |
I don't think it stores it elsewhere, too. This seems to work ldc-developers/druntime#66 |
|
Finally merged as part of #1354 (rebased to fix merge conflicts, that's why this PR doesn't show up as merged). |
Work in progress, but made public to show there is some progress.
Needs LLVM master (3.8) and
ldc-developers/druntime#35ldc-developers/druntime#60Exceptions work in simple cases, but combinations still cause trouble:Open for comments...