1805809 - relocation R_386_GOTOFF against undefined hidden symbol `tabs_4d51_TabsBridgedEngine_reset' can not be used when making a shared object

Status: RESOLVED FIXED

(Toolkit :: UniFFI Bindings, defect)

Description

[Mike Hommey [:glandium]]

On some platforms (happens on Debian i386 and s390x), the build fails with an error like:

relocation R_386_GOTOFF against undefined hidden symbol `tabs_4d51_TabsBridgedEngine_reset' can not be used when making a shared object
/usr/bin/ld: final link failed: bad value

The problem on i386 is that tabs.uniffi.rs doesn't contains tabs_4d51_ prefixes, but tabs_1c79_.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1791851

:skhamis, since you are the author of the regressor, bug 1791851, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 2

[Mike Hommey [:glandium]]

There are apparently two problems, one immediate and one theoretical:

• The hash used in the ffi function prefixes depends on the in-memory properties of ComponentInterface's types.type_definitions.
• The uniffi_meta::checksum function relies on DefaultHasher, which is not guaranteed to stay the same across versions of rust, so the rust version used to pre-generate toolkit/components/uniffi-js/UniFFIGeneratedScaffolding.cpp might not generate the same hash as the rust version that builds tree.

Comment 3

[Mike Hommey [:glandium]]

• The hash used in the ffi function prefixes depends on the in-memory properties of ComponentInterface's types.type_definitions.

Mmmm that's actually not it. types is not hashed. Its content was the only difference in the output of dbg!() for the hashed value...

Comment 4

[Mark Hammond [:markh] [:mhammond]]

This almost looks like another manifestation of bug 1797714 (but Debian i386 being impacted gives me pause)

Comment 5

[Mike Hommey [:glandium]]

Oh, the reason it happens on Debian but not on our CI (after all we do have 32-bits Linux builds) is that the Debian build is not cross-compiled, so it's using a x86 rust compiler. I can totally reproduce off the application-services repo on x86_64 after installing a i686 rust compiler (e.g. rustup default stable-i686-unknown-linux-gnu) and removing rust-toolchain.toml (and checking the generated tabs.uniffi.rs).

After digging a little, it turns out the difference comes from the impl Hash for [T], which is called for each Vec, and which does a hasher.write_length_prefix(slice.len()), which translates to hasher.write_usize(slice.len()), which translates to hasher.write(&slice.len().to_ne_bytes()), which will do feed the hasher with 4 bytes on 32-bits and 8 bytes on 64-bits, and with different bytes order depending on the endian, so it is in fact, the same as bug 1797714.

The lesson here is that std::hash::Hash is not meant to be used the way uniffi uses it.

Comment 6

[Mike Hommey [:glandium]]

(And the executive summary of the issue is that Firefox fails to build on any platform that is not little-endian 64-bits)

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mike Hommey [:glandium] from comment #5)

The lesson here is that std::hash::Hash is not meant to be used the way uniffi uses it.

Yeah - https://github.com/mozilla/uniffi-rs/issues/1393. In the meantime, we decided to prioritize doing the generation at build-time because that would side-step the issue in most cases and also fix other pain points with having the generated code checked in. If that turns out to be tricky we intend fixing it in uniffi first.

The other reason we side-stepped that is because ideally we'd get rid of that hash entirely - in the meantime, uniffi kinda assumes that the uniffi bindgen utility will generally be built using the same compiler that will be used to build the generated code, but I'm not sure whether the above is an example of when this will not be true - ie, it sounds like we might need to fix it in uniffi anyway?

(Removing ni from Sammy as I think we understand what's going on here)

Comment 8

[Mike Hommey [:glandium]]

IIRC, generating the code at build time would be very difficult because of how the integration between C++ and rust code in the build system works. I'm looking at a quick-and-dirty stable hash for Debian.

Comment 9

[Mike Hommey [:glandium]]

I'm testing applying this on Debian: https://github.com/mozilla/uniffi-rs/compare/main...glandium:uniffi-rs:checksum

Comment 11

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #9)

I'm testing applying this on Debian: https://github.com/mozilla/uniffi-rs/compare/main...glandium:uniffi-rs:checksum

Confirmed to work on 32-bits and big-endian. I'll polish it and send a PR.

Comment 12

[Mike Hommey [:glandium]]

PR: https://github.com/mozilla/uniffi-rs/pull/1427

Comment 13

[Mike Hommey [:glandium]]

Attachment: Bug 1805809 - Update uniffi to 0.21.1 for more determinism across platforms.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7aa21aa29c9
Update uniffi to 0.21.1 for more determinism across platforms. r=janerik,supply-chain-reviewers

Comment 15

[Noemi Erli[:noemi_erli]]

Backed out 2 changesets (Bug 1804360, Bug 1805809) for causing build bustages CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&resultStatus=testfailed%2Cbusted%2Cexception&revision=c7aa21aa29c998c87ec19c64b36501978ed55239&selectedTaskRun=VsunbqfXS76EOLJoQy8wBQ.0

Log: https://treeherder.mozilla.org/logviewer?job_id=399934643&repo=autoland&lineNumber=70834

Backout: https://hg.mozilla.org/integration/autoland/rev/37e8f20bbb82f3a3f85833d84be4a28393cefbe0

Comment 16

[Mike Hommey [:glandium]]

Duh of course it failed, the checksum changes based on the uniffi version, so we need an update in the generated code in tree.

Comment 17

[Mike Hommey [:glandium]]

Attachment: Bug 1805809 - Update uniffi to 0.21.1 for more determinism across platforms.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d62469a1741
Update uniffi to 0.21.1 for more determinism across platforms. r=janerik,supply-chain-reviewers

Comment 19

[Sandor Molnar[:smolnar]]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 1806321.
https://hg.mozilla.org/mozilla-central/rev/370b88940241e124a82dee16c6fce90ee25af36b

Comment 21

[tt_1]

hey everyone, thanks for the fix.

Is it possible to backport it to the beta branch?

Never noticed, as I do my test rides of nightly with a cross compiler setup, hence it got away

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c3dbe28af6a
Update uniffi to 0.21.1 for more determinism across platforms. r=janerik,supply-chain-reviewers

Comment 23

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/0c3dbe28af6a

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 9308676 [details]
Bug 1805809 - Update uniffi to 0.21.1 for more determinism across platforms.

Beta/Release Uplift Approval Request

• User impact if declined: Build failures on 32-bits or big-endian hosts.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Build-time only, no functional change, and if something goes wrong, it does so visibly at build time.
• String changes made/needed: N/A
• Is Android affected?: No

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 9308676 [details]
Bug 1805809 - Update uniffi to 0.21.1 for more determinism across platforms.

Beta/Release Uplift Approval Request

• User impact if declined: If a dot release comes out, it would be good to have this in it too. See beta approval.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed:
• Is Android affected?: No

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9308676 [details]
Bug 1805809 - Update uniffi to 0.21.1 for more determinism across platforms.

Approved for Desktop 109.0b6 and Mobile 109.0b3.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/42d532d3de54

Comment 29

[Dianna Smith [:diannaS]]

Comment on attachment 9308676 [details]
Bug 1805809 - Update uniffi to 0.21.1 for more determinism across platforms.

Approved for 108.0.2

Comment 30

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-release/rev/b1388c0d0072