1233768 - ctypes/conversion-finalizer.js fails when compiled with clang-cl

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Nathan Froyd [:froydnj]]

I don't know whether this is a miscompilation of some sort by clang-cl, a genuine problem in the JIT, or some build configury weirdness.  Filing it here so it doesn't get forgotten.

Comment 1

[Jan de Mooij [:jandem]]

Nathan, are we still seeing this?

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> Nathan, are we still seeing this?

I do have this on my radar, but I don't have an answer right now.  clang-cl has been crashing as of late trying to compile firefox, so I haven't been able to get to a shell build and answer this question.  Leaving ni? open so I don't forget.

Comment 3

[Nathan Froyd [:froydnj]]

I am still seeing this failure with LLVM r285003 and clang r285001.

Comment 4

[(Away)]

This test passes for me with r286174 in both 32-bit and 64-bit builds.

Comment 5

[(Away)]

ctypes/conversion-finalizer.js is failing once again with clang trunk from somewhere just after the 7.0.0 bump.

Win32 only. With or without lld.

Comment 6

[(Away)]

Attachment: WIP

Taskcluster has been very uncooperative today, and I had to debug this without symbols. So I'm in the dark on some of the details here.

The jit-test crashes in the CRT's memset on a bogus address that is actually an x86 instruction. I used the Time Travel Debugger to trace this value back to a function whose disassembly seems to match ffi_closure_SYSV_inner: https://searchfox.org/mozilla-central/rev/b24e6342d744c5a83fab5c15972e11eeb69d68e6/js/src/ctypes/libffi/src/x86/ffi.c#490

While pushing the args to `closure->fun`, the value that is supposed to be `*respp` is actually in the form of an `ffi_closure` -- so likely it's `closure` and our stack is off by one word.

I'm guessing that this `regparm(1)` business is actually getting honored by clang-cl, but the caller (some asm thunk?) sees that it's built under _MSC_VER and assumes regparm doesn't work.

I disabled regparm for clang-cl and it fixed the test. I don't know if this is the best fix. Maybe we should just drop all attributes in clang-cl and use exclusively the _MSC_VER-specific pragmas, i.e. revert this earlier patch? https://hg.mozilla.org/integration/mozilla-inbound/rev/5c06b086e8b0#l3.12

Comment 7

[(Away)]

> I'm guessing that this `regparm(1)` business is actually getting honored by
> clang-cl, but the caller (some asm thunk?) sees that it's built under
> _MSC_VER and assumes regparm doesn't work.

I finally managed to do a local build, and indeed the above is exactly what happened.

https://dxr.mozilla.org/mozilla-central/rev/351c75ab74c9a83db5c0662ba271b49479adb1f1/js/src/ctypes/libffi/src/x86/win32.S#221,223-224

0:000> dv /V
@eax              @eax                      closure = 0x205a00e0
004fef68          @ebp+0x0008                 respp = 0x205a00e0
004fef6c          @ebp+0x000c                  args = 0x004fef84

Comment 8

[(Away)]

Attachment: (re)disable attributes under clang-cl

This bug is yet another piece of fallout from clang-cl behaving like both _MSC_VER and __clang__.

Nathan, what do you think of (re)disabling these attributes? I admit it's kinda yucky in that it would prevent future attributes that may be interesting. OTOH, given this code's sensitivity to things like calling conventions, I like being able to say "the compiler should act like (only) MSVC here".

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to David Major [:dmajor] from comment #8)
> Nathan, what do you think of (re)disabling these attributes? I admit it's
> kinda yucky in that it would prevent future attributes that may be
> interesting. OTOH, given this code's sensitivity to things like calling
> conventions, I like being able to say "the compiler should act like (only)
> MSVC here".

I don't think disabling these attributes is a bad thing, but I would really like to know what changed in the compiler to make things start failing again.  Want to bisect the compiler a little, or even just scan commits?

If regparm(1) keeps getting enabled by accident, perhaps we should inform upstream so they can put some regression tests in place to make sure the compiler DTRT?

Comment 10

[(Away)]

I went all the way back to clang r286174 and indeed it seems to be pulling `closure` and `respp` from the right places on the stack, i.e. ignoring regparm(1).

I didn't see anything standing out in the history of files mentioning regparm.

While in the abstract, bisecting this seems like a worthy thing to do, in practice I can't afford to devote that much time to this.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8940299 [details] [diff] [review]
(re)disable attributes under clang-cl

Review of attachment 8940299 [details] [diff] [review]:
-----------------------------------------------------------------

OK, let's do this.  r=me with the changes below.

::: js/src/ctypes/libffi-patches/02-clang-cl.patch
@@ -35,5 @@
> - 
> - #ifndef LIBFFI_ASM
> - 
> --#ifdef _MSC_VER
> -+#if defined(_MSC_VER) && !defined(__clang__)

Please add a comment here about why we're disabling attributes for clang-cl, since clang-cl supports attributes.  And make sure that comment makes it in to ffi.h.in as well.  Thanks.

Comment 12

[(Away)]

Attachment: regparm

I've held off on landing this because I didn't feel very good about it.

It occurred to me that we could disable only regparm, WDYT?

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8960716 [details] [diff] [review]
regparm

Review of attachment 8960716 [details] [diff] [review]:
-----------------------------------------------------------------

I like this a little better.  I am slightly nervous about defining away regparm, as I could imagine very peculiar errors resulting from that, but defining away __attribute__ is probably even worse, and we've been OK so far!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f0acb32263c
Disable regparm under clang-cl in libffi. r=froydnj

Comment 15

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/9f0acb32263c