1032930 - --disable-ion build broken: js/src/vm/HelperThreads.cpp:571:14: error: invalid use of incomplete type 'class js::jit::IonBuilder'

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan Beich]

In file included from js/src/Unified_cpp_js_src5.cpp:2:
js/src/vm/HelperThreads.cpp:571:14: error: member access into
      incomplete type 'jit::IonBuilder'
    if (first->optimizationInfo().level() != second->optimizationInfo().level())
             ^
js/src/vm/HelperThreads.h:31:9: note: forward declaration of
      'js::jit::IonBuilder'
  class IonBuilder;
        ^
In file included from js/src/Unified_cpp_js_src5.cpp:2:
js/src/vm/HelperThreads.cpp:572:21: error: member access into
      incomplete type 'jit::IonBuilder'
        return first->optimizationInfo().level() < second->optimizationInfo(...
                    ^
js/src/vm/HelperThreads.h:31:9: note: forward declaration of
      'js::jit::IonBuilder'
  class IonBuilder;
        ^
In file included from js/src/Unified_cpp_js_src5.cpp:2:
js/src/vm/HelperThreads.cpp:575:14: error: member access into
      incomplete type 'jit::IonBuilder'
    if (first->script()->hasIonScript() != second->script()->hasIonScript())
             ^
js/src/vm/HelperThreads.h:31:9: note: forward declaration of
      'js::jit::IonBuilder'
  class IonBuilder;
        ^
In file included from js/src/Unified_cpp_js_src5.cpp:2:
js/src/vm/HelperThreads.cpp:576:22: error: member access into
      incomplete type 'jit::IonBuilder'
        return !first->script()->hasIonScript();
                     ^
js/src/vm/HelperThreads.h:31:9: note: forward declaration of
      'js::jit::IonBuilder'
  class IonBuilder;
        ^
In file included from js/src/Unified_cpp_js_src5.cpp:2:
js/src/vm/HelperThreads.cpp:579:17: error: member access into
      incomplete type 'jit::IonBuilder'
    return first->script()->getUseCount() > second->script()->getUseCount();
                ^
js/src/vm/HelperThreads.h:31:9: note: forward declaration of
      'js::jit::IonBuilder'
  class IonBuilder;
        ^
5 errors generated.

http://mozillaproject.osuosl.org:8010/builders/runtests/builds/1689/steps/shell/logs/stdio

Comment 1

[Jan Beich]

Attachment: fix, always |true|

Minimalistic change. I actually don't know which value it should return. The Ion snippet just before says

    // A script without an IonScript has precedence on one with.
    if (first->script()->hasIonScript() != second->script()->hasIonScript())
        return !first->script()->hasIonScript();

but what if not a single script has IonScript? Which should take precedence then?

Comment 2

[Jan Beich]

Attachment: fix, always |true|

-U8 this time. git-bz-moz is broken atm.

Comment 3

[Jan Beich]

Attachment: fix, always |true|

... or not. Old cookies. Sorry, for the noise.

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 8448874 [details] [diff] [review]
fix, always |true|

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

::: js/src/vm/HelperThreads.cpp
@@ +577,5 @@
>          return !first->script()->hasIonScript();
>  
>      // A higher useCount indicates a higher priority.
>      return first->script()->getUseCount() > second->script()->getUseCount();
> +#else

Can you add a MOZ_ASSUME_UNREACHABLE here?

Comment 5

[Jan Beich]

Attachment: fix, v2

https://tbpl.mozilla.org/?tree=Try&rev=7e1fafebdbd5

Comment 6

[Hannes Verschore [:h4writer]]

Comment on attachment 8449524 [details] [diff] [review]
fix, v2

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

::: js/src/vm/HelperThreads.cpp
@@ +570,5 @@
>  
>      // A higher useCount indicates a higher priority.
>      return first->script()->getUseCount() > second->script()->getUseCount();
> +#else
> +    MOZ_ASSUME_UNREACHABLE("Cannot infer priority without Ion");

I think you still need the "return true;" after this line. Just to silence some compilers.

Comment 7

[Jan Beich]

(In reply to Hannes Verschore [:h4writer] from comment #6)
> > +#else
> > +    MOZ_ASSUME_UNREACHABLE("Cannot infer priority without Ion");
> 
> I think you still need the "return true;" after this line. Just to silence
> some compilers.

Even non-JS_THREADSAFE code in the same file suggests otherwise. And having |return true| may violate implicit style or just being less popular.

# have |return| in stubs
$ pcregrep -lM '^{\n\s*MOZ_ASSUME_UNREACHABLE.*\n\s*return.*\n}' js/src/**/*.cpp
js/src/jit/Recover.cpp
js/src/jit/arm/Assembler-arm.cpp
js/src/vm/ForkJoin.cpp

# omit |return| in stubs
$ pcregrep -lM '^{\n\s*MOZ_ASSUME_UNREACHABLE.*\n}' js/src/**/*.cpp
js/src/builtin/Intl.cpp
js/src/jit/Recover.cpp
js/src/jit/arm/Assembler-arm.cpp
js/src/jit/arm/CodeGenerator-arm.cpp
js/src/jit/arm/Lowering-arm.cpp
js/src/jit/arm/MacroAssembler-arm.cpp
js/src/jit/mips/CodeGenerator-mips.cpp
js/src/jit/mips/Lowering-mips.cpp
js/src/jit/x64/CodeGenerator-x64.cpp
js/src/jit/x64/Lowering-x64.cpp
js/src/jit/x64/Trampoline-x64.cpp
js/src/jsproxy.cpp
js/src/vm/ForkJoin.cpp
js/src/vm/GlobalObject.cpp
js/src/vm/HelperThreads.cpp
js/src/vm/PosixNSPR.cpp

Comment 8

[Hannes Verschore [:h4writer]]

I wouldn't trust those greps. A "void" function doesn't need a "return". Also in most cases they are used with switches for the default case. So there is no "return" there either.

But there is indeed a precedent in vm/HelperThreads.cpp. (even multiple). So feel free to ignore the "add return true;" comment.

Comment 9

[Jan Beich]

(In reply to Hannes Verschore [:h4writer] from comment #8)
> I wouldn't trust those greps. A "void" function doesn't need a "return".

Adding |(?!void)| doesn't change the number of files lacking |return|. I didn't add because type signature sometimes spans multiple lines and have comments.

/* static */ void
foo(int a,
    int b,
    int c)
{
}

> Also in most cases they are used with switches for the default case.
> So there is no "return" there either.

Already excluded by |^{| in the regexp.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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