MAC OSX Intel LLVM Assembler bug (causes Vorbis OGG loader to crash)

Question

I had mysterious bug in loading Vorbis Ogg files on Mac OSX. The first file is loaded correctly, the second crashes in some code that indicates the file is corrupted, the same happens even if I load the same exact file twice.

After long hours of deep debugging inside Vorbis I found out that the bug is caused by the system function "pow" (double power of) returning a (nan) for a completely valid input, and that happens only on the second call to (ov_read), on the first call the same exact values passed to "pow" returns valid result.

8 hours later and lots of Intel x87 documentation reading I found the problem. Long story short there is a function deep inside vorbis "vorbis_ftoi " that uses this assembly code:

__asm__("fistl %0": "=m"(i) : "t"(f));

Which should push and pop on the Intel FPU Stack. However on LLVM it generates this code:

fld    QWORD PTR [ebp-0x20]
fist   DWORD PTR [ebp-0x14]

Which pushes on the stack but never pops causing an FPU stack overflow. And that's obviously a bug in LLVM

The proper code generated by GCC looks like this:

fld    QWORD PTR [ebp-0x20]
fist   DWORD PTR [ebp-0xc]
fstp   st(0)        // pops off the stack

I wasted a day and a half and some bytes of my brian learning some garbage (x87 Instruction Set and Registers) on this, so I though I would share it.

Auday

Answer 1

Simpler patch, has affect only when compiling with llvm:

--- Xiph\vorbis\os.h    Mon Mar 28 08:42:43 2011
+++ Xiph\vorbis\os.h    Thu Feb 02 14:20:27 2012
@@ -81,7 +81,7 @@


 /* Special i386 GCC implementation */
-#if defined(__i386__) && defined(__GNUC__) && !defined(__BEOS__)
+#if defined(__i386__) && defined(__GNUC__) && !defined(__BEOS__) && !defined(__llvm__)
 #  define VORBIS_FPU_CONTROL
 /* both GCC and MSVC are kinda stupid about rounding/casting to int.
    Because of encapsulation constraints (GCC can't see inside the asm

Unfortunately, I don't have enough reputation to vote up the OP, but know that I'm grateful for your find. Thank you.

Answer 2

Excellent! Thank you. Another solution is to simply remove the asm altogether. Here is a patch:

--- lib/os.h 2011-11-13 20:36:24.000000000 -0500
+++ lib/os.h        2011-11-15 18:45:00.000000000 -0500
@@ -93,27 +93,16 @@
 typedef ogg_int16_t vorbis_fpu_control;

 static inline void vorbis_fpu_setround(vorbis_fpu_control *fpu){
-  ogg_int16_t ret;
-  ogg_int16_t temp;
-  __asm__ __volatile__("fnstcw %0\n\t"
-          "movw %0,%%dx\n\t"
-          "andw $62463,%%dx\n\t"
-          "movw %%dx,%1\n\t"
-          "fldcw %1\n\t":"=m"(ret):"m"(temp): "dx");
-  *fpu=ret;
 }

 static inline void vorbis_fpu_restore(vorbis_fpu_control fpu){
-  __asm__ __volatile__("fldcw %0":: "m"(fpu));
 }

 /* assumes the FPU is in round mode! */
 static inline int vorbis_ftoi(double f){  /* yes, double!  Otherwise,
                                              we get extra fst/fld to
                                              truncate precision */
-  int i;
-  __asm__("fistl %0": "=m"(i) : "t"(f));
-  return(i);
+    return (int)floor(f+.5);
 }
 #endif /* Special i386 GCC implementation */