1415697 - Build Failing: CheckedInt conflicting decleration

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[jlogandavison]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170928210252

Steps to reproduce:

A while ago I pulled the mozilla-central repo and built it successfully.

I recently pulled the latest source from the mozilla-central repo (75c04adb414b) and the android build appears to be failing now.

I did a clean build. Here's the mozconfig:

# Build Firefox for Android:
ac_add_options --enable-application=mobile/android
ac_add_options --target=arm-linux-androideabi

# With the following Android SDK and NDK:
ac_add_options --with-android-sdk="/home/jack/.mozbuild/android-sdk-linux"
ac_add_options --with-android-ndk="/home/jack/.mozbuild/android-ndk-r11c"

# Enable debug options
ac_add_options --enable-debug
ac_add_options --enable-debug-symbols

I'm building on a debian machine.


Actual results:

The build fails with this error message:

 2:43.28 In file included from /<HOMEDIR>/mozilla-workspace/bug1180865/obj-arm-linux-androideabi/js/src/Unified_cpp_js_src42.cpp:29:
 2:43.28 /<HOMEDIR>/mozilla-workspace/bug1180865/js/src/wasm/WasmValidate.cpp:32:16: error: target of using declaration conflicts with declaration already in scope
 2:43.28 using mozilla::CheckedInt;
 2:43.28                ^
 2:43.29 /<HOMEDIR>/mozilla-workspace/bug1180865/obj-arm-linux-androideabi/dist/include/mozilla/CheckedInt.h:571:16: note: target of using declaration
 2:43.29   friend class CheckedInt;
 2:43.29                ^
 2:43.29 /<HOMEDIR>/mozilla-workspace/bug1180865/js/src/wasm/WasmTextToBinary.cpp:43:16: note: conflicting declaration
 2:43.29 using mozilla::CheckedInt;
 2:43.29                ^
 2:44.90 1 error generated.
 2:44.92 /<HOMEDIR>/mozilla-workspace/bug1180865/config/rules.mk:1028: recipe for target 'Unified_cpp_js_src42.o' failed

(If I comment the "using mozilla::CheckedInt;" line from WasmTextToBinary.cpp then the build completes)

(The desktop-linux build completes successfully without modifying any files)


Expected results:

The build should have completed.

Comment 1

[jlogandavison]

Sorry, correction. I commented the line "using mozilla::CheckedInt;" from WasmValidate.cpp in order to get the build working. NOT WasmTextToBinary.cpp

Comment 2

[Jason Orendorff [:jorendorff]]

Benjamin, could you take a look?

This has to be unified builds biting us, since the two `using` declarations occur in different .cpp files. It also probably explains why it's platform-specific.

But I still don't understand why the declarations conflict. There's only one mozilla::CheckedInt. Right?

Cc-ing some people who understand C++ better than I do.

Comment 3

[Jason Orendorff [:jorendorff]]

Forgot to say: Thanks for the bug report, jlogandavison. Bear with us; C++ is weird and the way we use it is weird.

Comment 4

[Botond Ballo [:botond]]

Reduced code example that triggers the error:

#include "mozilla/CheckedInt.h"
using mozilla::CheckedInt;

void foo() {
    CheckedInt<uint32_t> index;
}
    
namespace js {
namespace jit {

using mozilla::CheckedInt;

} // namespace jit
} // namespace js

using namespace js::jit;
using mozilla::CheckedInt;

Comment 5

[Botond Ballo [:botond]]

Further reduced to:


namespace mozilla {
    template <typename T>
    class CheckedInt {
        template <typename U>
        friend class CheckedInt;
    };
}

using mozilla::CheckedInt;

void foo() {
    CheckedInt<int> index;
}
    
namespace js {
namespace jit {

using mozilla::CheckedInt;

} // namespace jit
} // namespace js

using namespace js::jit;
using mozilla::CheckedInt;


Only the Android compiler (which is "3.8.243773") gives an error about this. My desktop version of clang 3.8 (which is "3.8.1-24") does not. Could be a clang bug?

Comment 6

[Botond Ballo [:botond]]

In any case, several tweaks fix the problem:

  - Placing the "using mozilla::CheckedInt" that's inside js::jit
    in the global scope instead.

    I believe the offending using-declaration comes from
    js/src/jit/shared/Assembler-shared.h.

  - Getting rid of the "using namespace js::jit", replacing it
    with targeted using-declarations instead.

    The use of using-directives is error-prone and discouraged
    anyways...

Comment 7

[Botond Ballo [:botond]]

Attachment: Bug 1415697 - Move a using-declaration in Assembler-shared.h to global scope.

This works around a clang compiler error on clang that is likely caused
by a clang bug.

Review commit: https://reviewboard.mozilla.org/r/198684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198684/

Comment 8

[Botond Ballo [:botond]]

I went with the first option because "using namespace js::jit" appears in many places in the WASM code.

jlogandavidson, can you verify that this patch allows your build to succeed?

Comment 9

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0b98afa80841541c464a683e46ce7559382ff4e

Comment 10

[jlogandavison]

I've just applied the patch and tried a clean build.

Finished without errors, looks good :)

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8927373 [details]
Bug 1415697 - Move a using-declaration in Assembler-shared.h to global scope.

https://reviewboard.mozilla.org/r/198684/#review204086

Thanks for investigating!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2a9944436adc
Move a using-declaration in Assembler-shared.h to global scope. r=bbouvier

Comment 13

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/2a9944436adc

Comment 14

[Julien Cristau [:jcristau]]

Does this need uplift?

Comment 15

[Botond Ballo [:botond]]

Not unless someone runs into this compiler error on the beta branch. (If that happens, we can uplift at that time; this is a virtually no risk, compile-time only patch.)