Fix for Bug 5053

[swaroop.sridhar]

Summary

Fix for LLI failure on Windows\X86: http://llvm.org/bugs/show_bug.cgi?id=5053

The here problem is that LLI.exe crashes on Windows\X86 when single precession floating point intrinsics like the following are used: acos, asin, atan, atan2, ceil, copysign, cos, cosh, exp, floor, fmin, fmax, fmod, log, pow, sin, sinh, sqrt, tan, tanh

The above intrinsics are defined as inline-expansions in math.h, and are not exported by msvcr120.dll (Win32 API GetProcAddress returns null).

For an FREM instruction, the JIT compiler generates a call to a stub for the fmodf() intrinsic, and adds a relocation to fixup at load time. The loader searches the libraries for the function, but fails because the symbol is not exported. So, the call target remains NULL and the execution crashes.

Since the math functions are loaded at JIT/runtime, the JIT can patch CALL instruction directly instead of the searching the libraries’ exported symbols.
However, this fix caused build failures due to unresolved symbols like _fmodf at link time.

Therefore, the current fix defines helper functions in the Runtime link/load library to perform the above operations. The address of these helper functions are used to patch up the CALL instruction at load time.

[swaroop.sridhar]

Added test case to CodeGen\X86

[rnk]

Your test case already passes for me. Can you elaborate on when this fails and in what environment, i.e. VS version, LLVM version, MCJIT vs interpreter, etc?

lib/Support/Windows/DynamicLibrary.inc

138

This should either be SYMBOL1 or the define should drop the trailing 1.

lib/Support/Windows/explicit_symbols.inc

74

What version of VS are you testing with? I don't believe this will compile on VS 2012, because it only has _copysign. See the 2012 MSDN docs vs the 2013 docs:
http://msdn.microsoft.com/en-us/library/0yafk1hc(v=vs.110).aspx
http://msdn.microsoft.com/en-us/library/0yafk1hc(v=vs.120).aspx

Furthermore, with VS2013, I think we can simply use the EXPLICIT_SYMBOL mechanism instead of a new approach.

test/CodeGen/X86/win_frem.ll

1 ↗

(On Diff #14327)

With the fix committed, this should be "used to crash" instead of "crashes".

Also, this test is an execution test that belongs in test/ExecutionEngine instead of test/CodeGen/X86.

[swaroop.sridhar]

Thanks a lot for the review Reid.

1. I'm using VS2013, LLVM 3.5, and not using MCJIT to repro this failure.
2. I've updated the test case to look like the actual test reported in Bug 5053. The problem should repro now, can you please try again?
3. I've fixed the issues you've identified. -> Fixed the wording in the comments for the test case -> Made copysignf VS2013 only
4. In DynamicLibrary.inc, I've simplified the implementation of the inline_ *() functions. Now, inline_fmodf(x,y) simply calls fmodf(x,y) instead of performing (float)fmod(x,y)

I've attached a new patch for with the updates. Please take a look at it.

Thanks,
Swaroop.

• inline-frem.patch6 KBDownload

[swaroop.sridhar]

Addressed Code review feedback from Reid. Updated the diff in the revision, instead of attaching a patch in my earlier comment.
[Changes are
1.I'm using VS2013, LLVM 3.5, and not using MCJIT to repro this failure.
2.I've updated the test case to look like the actual test reported in Bug 5053. The problem should repro now, can you please try again?
3.I've fixed the issues you've identified. -> Fixed the wording in the comments for the test case -> Made copysignf VS2013 only
4.In DynamicLibrary.inc, I've simplified the implementation of the inline_ *() functions. Now, inline_fmodf(x,y) simply calls fmodf(x,y) instead of performing (float)fmod(x,y)]

[lhames]

Hi Swaroop,

Copied from the bug report:

The old JIT is deprecated in 3.5, and has been removed entirely on the development branch of LLVM. Could you try this with MCJIT and see if your problem reproduces there?

Just run 'lli -use-mcjit testcase.ll' on 3.5, or simply 'lli testcase.ll' on the development branch where MCJIT is the default.

Cheers,
Lang.

[swaroop.sridhar]

Hi Lang,

Is the MCJit supported on Windows?

I tried the following using lli MCJIT (on the development branch of LLVM), but LLI failed with the following error:

Debug\bin\lli.exe llvm\test\ExecutionEngine\hello.ll
LLVM ERROR: Incompatible object format!
Stack dump:
0. Program arguments: Debug\bin\lli.exe D:\enlist\JIT\llvm\test\ExecutionEngine\hello.ll

Thanks,
Swaroop.

[swaroop.sridhar]

I verified that the same failure repros on Windows when using MCJit, and that the patch fixes it.
I've updated the test case with the target triple "windows-msvc-elf" so that it works on MCJit.
Here's the new patch:

inline-frem2.patch6 KBDownload

lib/Support/Windows/DynamicLibrary.inc

138

Fixed this, thanks.

lib/Support/Windows/explicit_symbols.inc

74

I've changed this to copysignf to be VS2013 only, thanks.

test/CodeGen/X86/win_frem.ll

1 ↗

(On Diff #14327)

Fixed the comment wording.

[rnk]

Looks pretty good. Do you want me to tweak the test case and commit this for you?

I checked the test case and it fails for me today, so this fixes a real bug.

test/ExecutionEngine/win-frem.ll

1

These tests run on all platforms supported by the JIT, so I'd just name it "frem.ll". Windows just happened to be the problematic one.

5

Use %lli, which expands with the appropriate -mtriple flag. You can also FileCheck the result.

7–8

Get rid of this so we can be target neutral.

15

main returns i32?

[swaroop.sridhar]

OK Thanks Reid, would be great if you can get this fix checked in.

Thanks,
Swaroop.

[swaroop.sridhar]

This fix is already checked in by rnk

lib/Support/Windows/DynamicLibrary.inc

   extern "C" { extern void *SYM; }
 #define EXPLICIT_SYMBOL2(SYMFROM, SYMTO) EXPLICIT_SYMBOL(SYMTO)
 
+#ifdef _M_IX86
+// Win32 on x86 implements certain single-precision math functions as macros.
+// These functions are not exported by the DLL, but will still be needed 
+// for symbol-resolution by the JIT loader. Therefore, this Support libray 
+// provides helper functions with the same implementation.
+
+#define INLINE_DEF_SYMBOL1(TYP, SYM)                                      \
+  extern "C" {                                                                 \
+    TYP inline_##SYM(TYP _X) { return SYM(_X); }                               \
+  }
+#define INLINE_DEF_SYMBOL2(TYP, SYM)                                      \
+  extern "C" {                                                                 \
+    TYP inline_##SYM(TYP _X, TYP _Y) { return SYM(_X, _Y); }                   \
+  }
+#endif
+
 #include "explicit_symbols.inc"
 
 #undef EXPLICIT_SYMBOL
 #undef EXPLICIT_SYMBOL2
+#undef INLINE_DEF_SYMBOL1
+#undef INLINE_DEF_SYMBOL2
 
 void* DynamicLibrary::SearchForAddressOfSymbol(const char* symbolName) {
   SmartScopedLock<true> Lock(*SymbolsMutex);

   #define EXPLICIT_SYMBOL2(SYMFROM, SYMTO)        \
     if (!strcmp(symbolName, #SYMFROM)) return (void*)&SYMTO;
 
+  #ifdef _M_IX86
+  #define INLINE_DEF_SYMBOL1(TYP, SYM)             \
+    if (!strcmp(symbolName, #SYM)) return (void*)&inline_##SYM;
+  #define INLINE_DEF_SYMBOL2(TYP, SYM) INLINE_DEF_SYMBOL1(TYP, SYM)
+  #endif 
+
   {
     #include "explicit_symbols.inc"
   }
 
   #undef EXPLICIT_SYMBOL
   #undef EXPLICIT_SYMBOL2
+  #undef INLINE_DEF_SYMBOL1
+  #undef INLINE_DEF_SYMBOL2
 
   return 0;
 }
 
-
 void *DynamicLibrary::getAddressOfSymbol(const char *symbolName) {
   if (!isValid())
     return NULL;

   return (void *)(intptr_t)GetProcAddress((HMODULE)Data, symbolName);
 }
 
-
 }

lib/Support/Windows/explicit_symbols.inc

 /* msvcrt */
 #if defined(_MSC_VER)
   EXPLICIT_SYMBOL2(alloca, _alloca_probe)
+
+#ifdef _M_IX86
+#define INLINE_DEF_FLOAT_SYMBOL(SYM, ARGC) INLINE_DEF_SYMBOL##ARGC(float, SYM)
+  INLINE_DEF_FLOAT_SYMBOL(acosf, 1)
+  INLINE_DEF_FLOAT_SYMBOL(asinf, 1)
+  INLINE_DEF_FLOAT_SYMBOL(atanf, 1)
+  INLINE_DEF_FLOAT_SYMBOL(atan2f, 2)
+  INLINE_DEF_FLOAT_SYMBOL(ceilf, 1)
+#if (_MSC_VER==1800)
+  INLINE_DEF_FLOAT_SYMBOL(copysignf, 2)
+#endif
+  INLINE_DEF_FLOAT_SYMBOL(cosf, 1)
+  INLINE_DEF_FLOAT_SYMBOL(coshf, 1)
+  INLINE_DEF_FLOAT_SYMBOL(expf, 1)
+  INLINE_DEF_FLOAT_SYMBOL(floorf, 1)
+  INLINE_DEF_FLOAT_SYMBOL(fminf, 2)
+  INLINE_DEF_FLOAT_SYMBOL(fmaxf, 2)
+  INLINE_DEF_FLOAT_SYMBOL(fmodf, 2)
+  INLINE_DEF_FLOAT_SYMBOL(logf, 1)
+  INLINE_DEF_FLOAT_SYMBOL(powf, 2)
+  INLINE_DEF_FLOAT_SYMBOL(sinf, 1)
+  INLINE_DEF_FLOAT_SYMBOL(sinhf, 1)
+  INLINE_DEF_FLOAT_SYMBOL(sqrtf, 1)
+  INLINE_DEF_FLOAT_SYMBOL(tanf, 1)
+  INLINE_DEF_FLOAT_SYMBOL(tanhf, 1)
+#undef INLINE_DEF_FLOAT_SYMBOL
+#endif
+
 #endif

test/ExecutionEngine/win-frem.ll

+; LLI.exe used to crash on Windows\X86 when certain single precession 
+; floating point intrinsics (defined as macros) are used.
+; This unit test guards against the failure.
+;
+; RUN: lli %s > /dev/null
+
+target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-f80:32:32"
+target triple = "i686-pc-win32"
+
+@flt = internal global float 12.0e+0
+@str = internal constant [18 x i8] c"Double value:%f \0A\00"
+
+declare i32 @printf(i8* nocapture, ...) nounwind
+
+define i16 @main() {
+  %flt = load float* @flt
+  %float2 = frem float %flt, 5.0
+  %double1 = fpext float %float2 to double
+  %yo = call i32(i8*, ...)* @printf( i8* getelementptr ([18 x i8]* @str, i32 0, i64 0), double %double1 )
+
+  ret i16 0
+}