Skip to content

Conversation

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Feb 27, 2022

This seems to be what is needed to get i686-linux-gnu Guix builds working.

Since we don't officially provide i686-linux-gnu release binaries, I'm not sure if this is wanted for Core. Considering the need to reintroduce --enable-glibc-back-compat, I would guess we don't want it. But at the same time, our Guix stuff presently has incomplete/broken i686 support, so maybe others might disagree and think it's worth keeping. (I'm keeping it in Knots for now, so might as well offer it for Core here too.)

Note that for __divmoddi4, I have reintroduced it with a very simple (copyright-immune?) replacement similar to the one used by LLVM. Prior versions (0.17-0.21) using it had copied GPL 3 code from GCC.

Previous versions had also used this __divmoddi4 substitution concept for ARM. As far as I can tell, it was never needed for ARM. While ARM uses a similar method, the equivalent libgcc functions it calls appear to be older than on x86-32.

Also note we cannot use the "build with an older library" trick for __divmoddi4 because it is part of GCC, not glibc, and we require a fairly new GCC to build at all, at this point.

@laanwj
Copy link
Member

laanwj commented Feb 28, 2022

I'm ok with doing the minimal here, but a reminder that supporting i686 is outside the goals of this project. So NACK on reintroducing --enable-glibc-back-compat. It's ugly, outside scope of maintenance, and as you said yourself on IRC, introducing parts of libgcc into our codebase is legally questionable (dunno about other libraries that look like it like LLVM's).

Edit: also as said on IRC, with gitian it was the best we could do, as we were bound to ubuntu's toolchains, with guix we control the entire build environment including toolchains so could patch those if necessary. There's no excuse for having something like --enable-glibc-back-compat in the leaf bitcoin-core build.

But a more trivial solution is still preferred. Would bumping the minimum libc version for i686 help?

return False
return True

def control_flow_instruction_for(binary):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK on the security check changes

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 28, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

GCC 7 introduced the __divmoddi4 function required for x86-32 builds.

This shifts the minimum required Debian version to 10, and Ubuntu to 18.04.
No change to CentOS support (8).
@luke-jr
Copy link
Member Author

luke-jr commented Feb 28, 2022

re foreign code: this version is very simple (likely too simple for copyright), but in the worst case is licensed under MIT the same as our code. see LLVM code here. (But the other reasons not to reintroduce glibc-back-compat still hold)

re patching the toolchain: I'm not aware of any easy patch to make GCC 7+ not require this. Maybe we could include __divmoddi4 in crti.o or something, but that feels ugly too. In theory -static-libgcc should do the job - but I don't know what other ramifications that might have.

re bumping the minimum libc version for i686: this doesn't involve libc, but we could presumably bump the minimum libgcc version. This would lose support for Debian stretch (supported under June), requiring Debian 10 (buster, oldstable, released 2019 July). It would lose Ubuntu xenial (supported until 2026), requiring at least bionic (released 2018). CentOS gained a new enough version with CentOS 8 (released 2019 September); we already don't support older versions of CentOS. FWIW, I also have a device that requires Linux 2.6.32, which holds it back to glibc 2.25, but has fully updated to GCC 11 with no problems.

Dropped the glibc-back-compat commits and replaced them with one bumping the libgcc requirement enforced by symbol-check.

@fanquake
Copy link
Member

Concept ~0.

Since we don't officially provide i686-linux-gnu release binaries, I'm not sure if this is wanted for Core.

I agree. While the changes here are simpler than when first proposed, reintroducing --enable-glibc-back-compat would have been a NACK, as-is this would be the same as us re-introducing support in our build infrastructure for 32-bit windows builds. It seems odd for us to introduce a bunch of code that isn't going to be exercised / tested (by us), to support binaries we don't release.

@hebasto
Copy link
Member

hebasto commented Mar 22, 2022

Since we don't officially provide i686-linux-gnu release binaries, I'm not sure if this is wanted for Core.

Concept ~0. Bitcoin Core does not need these changes.

Otherwise, code changes (c76ac9d) look correct.

@fanquake
Copy link
Member

Going to close this then. If you would like to provide Guix built i686-linux-gnubinaries for Knots, you can maintain these changes there.

@fanquake fanquake closed this Mar 22, 2022
fanquake added a commit to bitcoin-core/gui that referenced this pull request Mar 24, 2022
…6-linux-gnu` host

97af652 guix: Drop code for the unsupported `i686-linux-gnu` host (Hennadii Stepanov)

Pull request description:

  Now GUIX build for the `i686-linux-gnu` host is broken, and [there are no plans to re-add it](bitcoin/bitcoin#24448).

ACKs for top commit:
  fanquake:
    ACK 97af652

Tree-SHA512: 968181aff65e607a7c1a1b06ac7dfd79f6e2ce49b3c4c3828def020e925769fdbab1859d37ea924ded7632405b30539ac3ec81ac714cb9a01a2f7d5c93301dd9
dekm pushed a commit to unigrid-project/daemon that referenced this pull request Oct 27, 2022
…gnu` host

97af652 guix: Drop code for the unsupported `i686-linux-gnu` host (Hennadii Stepanov)

Pull request description:

  Now GUIX build for the `i686-linux-gnu` host is broken, and [there are no plans to re-add it](bitcoin#24448).

ACKs for top commit:
  fanquake:
    ACK 97af652

Tree-SHA512: 968181aff65e607a7c1a1b06ac7dfd79f6e2ce49b3c4c3828def020e925769fdbab1859d37ea924ded7632405b30539ac3ec81ac714cb9a01a2f7d5c93301dd9
@bitcoin bitcoin locked and limited conversation to collaborators Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants