Page MenuHomePhabricator

Don't assume mingw is providing SSP functions
ClosedPublic

Authored by vchuravy on Dec 1 2016, 2:11 AM.

Details

Summary

Mingw is indirectly targeting msvcrt*.dll and we can't guarantee that
these functions will be available during JIT'ing.

Diff Detail

Repository
rL LLVM

Event Timeline

vchuravy updated this revision to Diff 79878.Dec 1 2016, 2:11 AM
vchuravy retitled this revision from to Don't assume mingw is providing SSP functions.
vchuravy updated this object.

The Julia project is currently evaluating updating LLVM from 3.7 to 3.9 and we are working on making sure that it will continue to work on all supported platforms.
We currently support Windows through Mingw and encountered LLVM ERROR: Program used external function '__atomic_store' which could not be resolved! in JIT'ed code.
So far we did not need MSVCRT as a runtime library with Mingw, greatly simplifying deployment.

An alternative might be to change the meaning of isOSMSVCRT to not include MinGW, I at least find the following code and comment confusing:

else if (!Is64Bit && !canGuaranteeTCO(CallConv) &&
         !Subtarget.getTargetTriple().isOSMSVCRT() &&
         SR == StackStructReturn)
  // If this is a call to a struct-return function, the callee
  // pops the hidden struct pointer, so we have to push it back.
  // This is common for Darwin/X86, Linux & Mingw32 targets.
  // For MSVC Win32 targets, the caller pops the hidden struct pointer.
---
 include/llvm/ADT/Triple.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/llvm/ADT/Triple.h b/include/llvm/ADT/Triple.h
index b98f840..102b59b 100644
--- a/include/llvm/ADT/Triple.h
+++ b/include/llvm/ADT/Triple.h
@@ -517,8 +517,7 @@ public:
 
   /// Is this a "Windows" OS targeting a "MSVCRT.dll" environment.
   bool isOSMSVCRT() const {
-    return isWindowsMSVCEnvironment() || isWindowsGNUEnvironment() ||
-           isWindowsItaniumEnvironment();
+    return isWindowsMSVCEnvironment() || isWindowsItaniumEnvironment();
   }
 
   /// Tests whether the OS is Windows.
-- 
2.10.2

I should also note the mingw64 provides "libssp" so the MSVCRT functions are not needed on this target.

vchuravy abandoned this revision.Feb 13 2017, 6:04 PM

In the progress of updating the Julia frontend to LLVM 6.0.0 I encountered this issue again.
Julia is using mingw for our windows support and as far as I understand mingw doesn't provide __security_cookie and __security_check_cookie
as part of the runtime (https://sourceforge.net/p/mingw-w64/mailman/message/27235169/). I suspect partly due to the fact that the version of
msvcrt.dll that mingw only has the symbols in an external static library (bufferoverflowU.lib). On the other hand mingw does provide libssp.

Since Julia is currently carrying this patch I would welcome a discussion if this is the right approach or what we should be doing differently.

vchuravy updated this revision to Diff 138831.Mar 17 2018, 2:38 PM

Rebase onto master

Isn't this only an issue if you actually enable it (via -fstack-protector-strong to clang, or something similar in parameters to llvm codegen)?

This change in itself is right though, since we shouldn't be using those MSVC specific if targeting mingw.

But if I try building code with clang with this patch in place with -fstack-protector-strong, I get an assertion failure ("Assertion failed: (InChain.getValueType() == MVT::Other && "Not a chain"), function HandleMergeInputChains, file ../lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp, line 2557.") while I previously got a successful compile (but referencing the MSVC specific symbols). So I'm not sure exactly what and how this patch fixes.

So IMO the patch looks something in the right direction, but it doesn't seem to be complete to me. It at least needs a test case and showing what it fixes, and the assertion failure I quoted needs to be sorted out in one way or another.

@mstorsjo Thanks for your feedback. Might I ask how you tested this? Did you simply compiled clang as well and run the testsuite?

Yes this should only be an issue when StackProtectStrong is set on functions, which Julia does in certain build configurations (we could also not do that on Windows, but I would rather fix the issue upstream)

@mstorsjo Thanks for your feedback. Might I ask how you tested this? Did you simply compiled clang as well and run the testsuite?

I tested compiling a minimal trivial C file with clang:

$ cat ssp.c
void other(void* ptr);
void func(void) {
	char c;
	other(&c);
}
$ clang -target x86_64-w64-mingw32 -S -o - ssp.c -O2 -fstack-protector-strong

It can also easily be reproduced with plain IR and llc:

$ cat ssp.ll
define dso_local void @func() sspstrong {
entry:
  %c = alloca i8, align 1
  call void @llvm.lifetime.start.p0i8(i64 1, i8* nonnull %c) 
  call void @other(i8* nonnull %c) 
  call void @llvm.lifetime.end.p0i8(i64 1, i8* nonnull %c) 
  ret void
}
declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture) 
declare dso_local void @other(i8*) 
declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture) 
$ llc -mtriple x86_64-w64-mingw32 ssp.ll

Yes this should only be an issue when StackProtectStrong is set on functions, which Julia does in certain build configurations (we could also not do that on Windows, but I would rather fix the issue upstream)

Ok, well if you are setting this you will need to have GCC's libssp available for linking as well (I presume you have that and that's what you intend?), since the example above should produce references to __stack_chk_guard and __stack_chk_fail if it didn't error out during compilation.

vchuravy updated this revision to Diff 139001.Mar 19 2018, 1:53 PM

Start adding test for ssp and sspstrong for mingw32

Hm I can't reproduce the failure you are seeing. I added your example as a test case and started extending the tests in Codegen/X86/stack-protector.ll to also cover mingw (there still lots of cases to go there).

mstorsjo accepted this revision.Mar 20 2018, 12:10 AM

Hm I can't reproduce the failure you are seeing.

Sorry about that - I tested it with a build tree that was a couple weeks old. For some reason I got that failure there, but it does indeed seem to work with the latest trunk version. No idea what changed inbetween.

I added your example as a test case and started extending the tests in Codegen/X86/stack-protector.ll to also cover mingw (there still lots of cases to go there).

Thanks, this looks great!

Do you have commit access, or do you want me to commit it for you?

This revision is now accepted and ready to land.Mar 20 2018, 12:10 AM

One small detail actually; you might want to check for isWindowsItaniumEnvironment() as well, since afaik that environment is supposed to link to MSVC libraries. So either Subtarget.getTargetTriple().isWindowsMSVCEnvironment() || Subtarget.getTargetTriple().isWindowsItaniumEnvironment() or Subtarget.getTargetTriple().isOSMSVCRT() && !Subtarget.getTargetTriple().isWindowsGNUEnvironment().

Sorry about that - I tested it with a build tree that was a couple weeks old. For some reason I got that failure there, but it does indeed seem to work with the latest trunk version. No idea what changed inbetween.

No worries!

Do you have commit access, or do you want me to commit it for you?

I don't have commit access so it would be great if you could land this.

You are right about Itanium. I will switch to not isWindowsGNUEnvironment() for the test and add a minimal test case for IA64.

You are right about Itanium. I will switch to not isWindowsGNUEnvironment() for the test and add a minimal test case for IA64.

isWindowsItaniumEnvironment() isn't about IA64, but about the itanium C++ ABI (the one used on all other platforms), contrary to the MSVC C++ ABI. So the windows Itanium environment is like MSVC except that it uses a C++ ABI similar to what mingw and other platforms uses.

vchuravy updated this revision to Diff 139158.Mar 20 2018, 10:29 AM
  • update for windows-itanium and add tests

Is the x86_64-pc-windows-itanium the right platform triple?

I adapted the test to check that windows-itanium and windows-msvc both behave like msvc.

LGTM. Do @compnerd or @smeenai have anything to add, or shall I commit it?

Seems fine to me, since windows-itanium is unaffected.

This revision was automatically updated to reflect the committed changes.