1233732 - disable MacroAssembler.h macro magic for clang-cl

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nathan Froyd [:froydnj]]

clang-cl attempts to emulate MSVC's handling of __VA_ARGS__, but doesn't
quite get it right:

https://llvm.org/bugs/show_bug.cgi?id=25875

As a result of this, compiling files that #include MacroAssembler.h with
clang-cl result in fallbacks to MSVC.  Since falling back to MSVC is
non-ideal (and also causes problems around things like linking function
template instantiations), let's disable MacroAssembler.h's macro magic
for the time being.  Ideally, the problem will get fixed upstream
soon (even though it looks somewhat complicated); in the meantime,
fixing this issue lets forward progress be made when compiling Gecko
with clang-cl.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: disable MacroAssembler.h macro magic for clang-cl

It's worth nothing that we get significantly farther in Firefox startup
(crashing somewhere in JIT code, instead of crashing somewhere when JSContexts
are created) with this patch.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8700032 [details] [diff] [review]
disable MacroAssembler.h macro magic for clang-cl

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

::: js/src/jit/MacroAssembler.h
@@ +171,5 @@
>  
> +// clang-cl doesn't exactly follow MSVC's custom rules for handling
> +// __VA_ARGS__ in macros, so avoid using this macro there.
> +#if defined(_MSC_VER) && defined(__clang__)
> +# define DEFINED_ON(...) /* nothing */

This might have a side-effect of leaving undefined symbols on the macro assembler, is that acceptable?
Can we guard on a specific version of clang?

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8700032 [details] [diff] [review]
disable MacroAssembler.h macro magic for clang-cl

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

::: js/src/jit/MacroAssembler.h
@@ +170,5 @@
>        (MOZ_FOR_EACH(DEFINED_ON_FWDARCH, (), ArchList)))
>  
> +// clang-cl doesn't exactly follow MSVC's custom rules for handling
> +// __VA_ARGS__ in macros, so avoid using this macro there.
> +#if defined(_MSC_VER) && defined(__clang__)

If we are fine about having undefined symbols, then …
Guard on clang version, and add a link to llvm bug tracker, such that we can bump the version, or remove it later.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> Comment on attachment 8700032 [details] [diff] [review]
> disable MacroAssembler.h macro magic for clang-cl
> 
> Review of attachment 8700032 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/MacroAssembler.h
> @@ +170,5 @@
> >        (MOZ_FOR_EACH(DEFINED_ON_FWDARCH, (), ArchList)))
> >  
> > +// clang-cl doesn't exactly follow MSVC's custom rules for handling
> > +// __VA_ARGS__ in macros, so avoid using this macro there.
> > +#if defined(_MSC_VER) && defined(__clang__)
> 
> If we are fine about having undefined symbols, then …

Why would there be undefined symbols?  Presumably nothing is calling those functions, so there's nothing for the compiler to emit.

> Guard on clang version, and add a link to llvm bug tracker, such that we can
> bump the version, or remove it later.

What do you mean "guard on clang version" here?  There's no good way to check for "all versions until it gets fixed", is there?

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> (In reply to Nicolas B. Pierron [:nbp] from comment #3)
> > Comment on attachment 8700032 [details] [diff] [review]
> > disable MacroAssembler.h macro magic for clang-cl
> > 
> > Review of attachment 8700032 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/jit/MacroAssembler.h
> > @@ +170,5 @@
> > >        (MOZ_FOR_EACH(DEFINED_ON_FWDARCH, (), ArchList)))
> > >  
> > > +// clang-cl doesn't exactly follow MSVC's custom rules for handling
> > > +// __VA_ARGS__ in macros, so avoid using this macro there.
> > > +#if defined(_MSC_VER) && defined(__clang__)
> > 
> > If we are fine about having undefined symbols, then …
> 
> Why would there be undefined symbols?  Presumably nothing is calling those
> functions, so there's nothing for the compiler to emit.

Indeed, none of the functions are virtual, there should be no use of any symbol.

> > Guard on clang version, and add a link to llvm bug tracker, such that we can
> > bump the version, or remove it later.
> 
> What do you mean "guard on clang version" here?  There's no good way to
> check for "all versions until it gets fixed", is there?

I know we do something similar for pgo on MSVC and for features on GCC, where we check the major & minor version of the compiler.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> > > Guard on clang version, and add a link to llvm bug tracker, such that we can
> > > bump the version, or remove it later.
> > 
> > What do you mean "guard on clang version" here?  There's no good way to
> > check for "all versions until it gets fixed", is there?
> 
> I know we do something similar for pgo on MSVC and for features on GCC,
> where we check the major & minor version of the compiler.

Sure, but that's for things like, "hey this was busted in older versions, but it's fixed in a known later version".  This is a "hey, this is busted and we don't know if/when it's getting fixed" and we generally don't have version checks for those (compare disabling of PGO for functions and/or files across the tree).

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f47aa2b7f3f4

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f47aa2b7f3f4

Comment 9

[(no longer active)]

I fixed the LLVM bug so this is no longer needed.  I will back out this patch.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4892358dbf20

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4892358dbf20