Skip to content

Conversation

Empact
Copy link
Contributor

@Empact Empact commented Jan 11, 2019

SmartOS FD_ZERO is implemented in a way that requires
an external declaration of memcpy. We can not simply
include cstring in the existing file because
sanity_test_memcpy is attempting to replace memcpy.

Instead split glibc_sanity into fdelt and memcpy files,
and include in glibc_sanity/fdelt.cpp.

Fixes #13581, see also #13619

@Empact Empact force-pushed the glibc-sanity-string branch 2 times, most recently from 333ede2 to effb4fd Compare January 11, 2019 02:43
@laanwj
Copy link
Member

laanwj commented Jan 11, 2019

Concept ACK

@laanwj
Copy link
Member

laanwj commented Jan 16, 2019

@theuni can you take a look please

@Empact Empact force-pushed the glibc-sanity-string branch from effb4fd to 68a8df0 Compare January 16, 2019 20:12
@bitcoin bitcoin deleted a comment from DrahtBot Feb 14, 2019
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 14, 2019

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

Conflicts

No conflicts as of last run.

@fanquake
Copy link
Member

Concept ACK

@Empact Empact force-pushed the glibc-sanity-string branch from 68a8df0 to fd3e6e3 Compare February 25, 2019 11:13
@Empact Empact force-pushed the glibc-sanity-string branch 3 times, most recently from bd0bfed to fa364d5 Compare March 9, 2019 17:42
@Empact
Copy link
Contributor Author

Empact commented Mar 9, 2019

Made an effort to pare this down significantly. Now only one new file, and the necessity of the cstring include is determined in configure.

@Empact Empact force-pushed the glibc-sanity-string branch 3 times, most recently from 0afb772 to 5a37824 Compare March 9, 2019 19:10
@Empact Empact force-pushed the glibc-sanity-string branch from 5a37824 to b86bb6f Compare April 3, 2019 05:23
Empact added 2 commits April 13, 2019 20:21
SmartOS FD_ZERO is implemented in a way that requires
an external declaration of memcpy. We can not simply
include cstring in the existing file because
sanity_test_memcpy is attempting to replace memcpy, but we can do
so here, now that the fdelt test is split out.
@Empact Empact force-pushed the glibc-sanity-string branch from b86bb6f to b4fd0ca Compare April 14, 2019 03:22
@Empact
Copy link
Contributor Author

Empact commented Apr 14, 2019

@MarcoFalke realized that in attempting to rebase earlier I unintentionally overwrote my later implementation (#15146 (comment)) with the earlier. Restored the earlier implementation - would need gitian again.

@bitcoin bitcoin deleted a comment from DrahtBot Apr 15, 2019
@laanwj
Copy link
Member

laanwj commented Jun 7, 2019

Code review an lightly tested (but not on SmartOS) ACK b4fd0ca

@maflcko maflcko added this to the 0.19.0 milestone Jun 7, 2019
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 9, 2019

Gitian builds for commit 5d2ccf0 (master):

Gitian builds for commit bb4d0dd (master and this pull):

@DrahtBot
Copy link
Contributor

Gitian builds for commit 8a503a6 (master):

Gitian builds for commit 6ba1950 (master and this pull):

@laanwj laanwj changed the title Solve SmartOS FD_ZERO build issue build: Solve SmartOS FD_ZERO build issue Sep 18, 2019
laanwj added a commit that referenced this pull request Sep 18, 2019
b4fd0ca Include cstring for sanity_test_fdelt if required (Ben Woosley)
7fb886b [moveonly] Split glibc sanity_test_fdelt out (Ben Woosley)

Pull request description:

  SmartOS FD_ZERO is implemented in a way that requires
  an external declaration of memcpy. We can not simply
  include cstring in the existing file because
  sanity_test_memcpy is attempting to replace memcpy.

  Instead split glibc_sanity into fdelt and memcpy files,
  and include <cstring> in glibc_sanity/fdelt.cpp.

  Fixes #13581, see also #13619

ACKs for top commit:
  laanwj:
    Code review an lightly tested (but not on SmartOS) ACK b4fd0ca

Tree-SHA512: 231306da291ad9eca8ba91bea1e9c27b6c2e96e484d1602e1c2cf27761202f9287ce0bc19fefd000943d2b449d0e5929cd39e2f7e09cf930d89fa520228ccbec
@laanwj laanwj merged commit b4fd0ca into bitcoin:master Sep 18, 2019
@Empact Empact deleted the glibc-sanity-string branch September 18, 2019 19:38
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 23, 2019
b4fd0ca Include cstring for sanity_test_fdelt if required (Ben Woosley)
7fb886b [moveonly] Split glibc sanity_test_fdelt out (Ben Woosley)

Pull request description:

  SmartOS FD_ZERO is implemented in a way that requires
  an external declaration of memcpy. We can not simply
  include cstring in the existing file because
  sanity_test_memcpy is attempting to replace memcpy.

  Instead split glibc_sanity into fdelt and memcpy files,
  and include <cstring> in glibc_sanity/fdelt.cpp.

  Fixes bitcoin#13581, see also bitcoin#13619

ACKs for top commit:
  laanwj:
    Code review an lightly tested (but not on SmartOS) ACK b4fd0ca

Tree-SHA512: 231306da291ad9eca8ba91bea1e9c27b6c2e96e484d1602e1c2cf27761202f9287ce0bc19fefd000943d2b449d0e5929cd39e2f7e09cf930d89fa520228ccbec
@bitcoin bitcoin deleted a comment from DrahtBot Oct 13, 2019
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jun 4, 2020
Summary:
```The return type of fdelt_chk changed from unsigned long int to long
int in glibc 2.16. See this commit. Now that we require glibc >=2.17 we
can remove our back-compat code.
[...]
```

Note: we require glibc >= 2.19 so this also applies to us.

Backport of core [[bitcoin/bitcoin#15146 | PR15146]] and [[bitcoin/bitcoin#18862 | PR18862]].
[[bitcoin/bitcoin#15146 | PR15146]] fix an edge case from the sanity check which is totally removed
by the second commit of [[bitcoin/bitcoin#18862 | PR18862]]. The diff is then mostly a backport of
the first commit from [[bitcoin/bitcoin#18862 | PR18862]]:
bitcoin/bitcoin@8bf1540

Test Plan: Run the gitian builds.

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6342
kwvg added a commit to kwvg/dash that referenced this pull request Dec 4, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Dec 8, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Dec 13, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Dec 13, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
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.

Can't compile bitcoin for SmartOS - recipe for target 'compat/libbitcoin_util_a-glibc_sanity.o' failed
5 participants