Closed Bug 1415697 Opened 2 years ago Closed 2 years ago

Build Failing: CheckedInt conflicting decleration

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: jlogandavison, Assigned: botond)

Details

Attachments

(1 file)

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.
Sorry, correction. I commented the line "using mozilla::CheckedInt;" from WasmValidate.cpp in order to get the build working. NOT WasmTextToBinary.cpp
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.
Flags: needinfo?(bbouvier)
Priority: -- → P1
Forgot to say: Thanks for the bug report, jlogandavison. Bear with us; C++ is weird and the way we use it is weird.
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;
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?
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...
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?
Flags: needinfo?(jlogandavison)
Assignee: nobody → botond
I've just applied the patch and tried a clean build.

Finished without errors, looks good :)
Flags: needinfo?(jlogandavison)
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!
Attachment #8927373 - Flags: review?(bbouvier) → review+
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a9944436adc
Move a using-declaration in Assembler-shared.h to global scope. r=bbouvier
https://hg.mozilla.org/mozilla-central/rev/2a9944436adc
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Does this need uplift?
Flags: needinfo?(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.)
Flags: needinfo?(botond)
You need to log in before you can comment on or make changes to this bug.