Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util: Fix compilation errors in support/lockedpool.cpp #16161

Merged
merged 3 commits into from
Nov 20, 2019

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Jun 6, 2019

Changes in #12048 cause a compilation error in Arena::walk() when
ARENA_DEBUG is defined. Specifically, Arena's chunks_free map was
changed to have a different value type.

Additionally, missing includes cause other compilation errors when
ARENA_DEBUG is defined.

Reproduced with:

make CPPFLAGS=-DARENA_DEBUG

@maflcko maflcko changed the title Trivial: Fix compilation errors in support/lockedpool.cpp debug: Fix compilation errors in support/lockedpool.cpp Jun 7, 2019
@maflcko maflcko changed the title debug: Fix compilation errors in support/lockedpool.cpp util: Fix compilation errors in support/lockedpool.cpp Jun 7, 2019
@Empact
Copy link
Member

Empact commented Jun 10, 2019

How about a test case?

@jkczyz
Copy link
Contributor Author

jkczyz commented Jun 10, 2019

@Empact Would it be sufficient to remove #ifdef ARENA_DEBUG from this code to ensure the compilation error is caught in the future? Not sure if this goes against conventions for debug code.

@Empact
Copy link
Member

Empact commented Jun 10, 2019

No, the guards are there for a reason, just would be nice to exercise the guarded code at some point so such a failure is caught in the future.

The guiding principle is: “test anything that might fail and everything that does”.

May not be practical, but one possibility is treating one of the Travis test runs with ARENA_DEBUG.

@jkczyz
Copy link
Contributor Author

jkczyz commented Jun 11, 2019

I defined ARENA_DEBUG for a couple environments. Wasn't sure which environments, so I just added it to ones that were already setting CPPFLAGS. The Travis CI build failures seem unrelated.

I had to update the Arena's test to use allocated memory to prevent a segfault. Figured this may have been needed as I observed it locally.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 12, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17517 (ci: Bump to clang-8 for asan build to avoid segfaults on ppc64le by MarcoFalke)
  • #16834 (Fetch Headers over DNS by TheBlueMatt)
  • #16762 (Rust-based Backup over-REST block downloader by TheBlueMatt)
  • #16195 (util: Use void* throughout support/lockedpool.h by jkczyz)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@laanwj laanwj added the Tests label Jun 17, 2019
@laanwj
Copy link
Member

laanwj commented Jun 17, 2019

Thanks for catching this and fixing the build with -DARENA_DEBUG, not 100% sure about the build system and test changes.

@DrahtBot
Copy link
Contributor

Needs rebase

@fanquake
Copy link
Member

fanquake commented Nov 16, 2019

Concept ACK. @jkczyz could you rebase this?

Compiling on macOS with ARENA_DEBUG:

  CXX      support/libbitcoin_util_a-lockedpool.o
support/lockedpool.cpp:141:10: error: no member named 'cout' in namespace 'std'
    std::cout <<
    ~~~~~^
support/lockedpool.cpp:142:22: error: no member named 'hex' in namespace 'std'
        "0x" << std::hex << std::setw(16) << std::setfill('0') << base <<
                ~~~~~^
support/lockedpool.cpp:142:34: error: no member named 'setw' in namespace 'std'
        "0x" << std::hex << std::setw(16) << std::setfill('0') << base <<
                            ~~~~~^
support/lockedpool.cpp:142:51: error: no member named 'setfill' in namespace 'std'
        "0x" << std::hex << std::setw(16) << std::setfill('0') << base <<
                                             ~~~~~^
support/lockedpool.cpp:143:23: error: no member named 'hex' in namespace 'std'
        " 0x" << std::hex << std::setw(16) << std::setfill('0') << sz <<
                 ~~~~~^
support/lockedpool.cpp:143:35: error: no member named 'setw' in namespace 'std'
        " 0x" << std::hex << std::setw(16) << std::setfill('0') << sz <<
                             ~~~~~^
support/lockedpool.cpp:143:52: error: no member named 'setfill' in namespace 'std'
        " 0x" << std::hex << std::setw(16) << std::setfill('0') << sz <<
                                              ~~~~~^
support/lockedpool.cpp:144:31: error: no member named 'endl' in namespace 'std'
        " 0x" << used << std::endl;
                         ~~~~~^
support/lockedpool.cpp:150:10: error: no member named 'cout' in namespace 'std'
    std::cout << std::endl;
    ~~~~~^
support/lockedpool.cpp:150:23: error: no member named 'endl' in namespace 'std'
    std::cout << std::endl;
                 ~~~~~^
support/lockedpool.cpp:152:9: error: no matching function for call to 'printchunk'
        printchunk(chunk.first, chunk.second, false);
        ^~~~~~~~~~
support/lockedpool.cpp:140:13: note: candidate function not viable: no known conversion from 'const std::__1::__map_const_iterator<std::__1::__tree_const_iterator<std::__1::__value_type<unsigned long, char *>, std::__1::__tree_node<std::__1::__value_type<unsigned long, char *>, void *> *, long> >' to 'size_t' (aka 'unsigned long') for 2nd argument
static void printchunk(char* base, size_t sz, bool used) {
            ^
support/lockedpool.cpp:153:10: error: no member named 'cout' in namespace 'std'
    std::cout << std::endl;
    ~~~~~^
support/lockedpool.cpp:153:23: error: no member named 'endl' in namespace 'std'
    std::cout << std::endl;
                 ~~~~~^
13 errors generated.

@Empact
Copy link
Member

Empact commented Nov 16, 2019

Concept ACK

Changes in bitcoin#12048 cause a compilation error in Arena::walk() when
ARENA_DEBUG is defined. Specifically, Arena's chunks_free map was
changed to have a different value type.

Additionally, missing includes cause other compilation errors when
ARENA_DEBUG is defined.

Reproduced with:

make CPPFLAGS=-DARENA_DEBUG
The definition and uses of Arena::walk() are compiled only if
ARENA_DEBUG is defined. Configure Travis to define ARENA_DEBUG so
compilation errors do not go unnoticed.
@jkczyz
Copy link
Contributor Author

jkczyz commented Nov 16, 2019

Concept ACK. @jkczyz could you rebase this?

Done.

The test uses reinterpret_cast<void*> on unallocated memory. Using this
memory in printchunk as char* causes a segfault, so have printchunk take
void* instead.
@laanwj
Copy link
Member

laanwj commented Nov 20, 2019

ACK 30fb598

I like how minimal this change has become.

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK 30fb598 - thanks for following up jkczyz.

src/test/test_bitcoin --run_test=allocator_tests
Running 3 test cases...
0x00000000x80ffc10 0x00000000000003f0 0x1

0x00000000x8000000 0x00000000000ffc10 0x0


0x00000000x8000000 0x0000000000100000 0x0

0x00000000x80ffc80 0x0000000000000200 0x1
0x00000000x80ffe80 0x0000000000000100 0x1
0x00000000x80fff80 0x0000000000000080 0x1

0x00000000x8000000 0x00000000000ffc80 0x0

0x00000000x80ffc80 0x0000000000000200 0x1
0x00000000x80ffe80 0x0000000000000100 0x1

0x00000000x80fff80 0x0000000000000080 0x0
0x00000000x8000000 0x00000000000ffc80 0x0

0x00000000x80fff80 0x0000000000000080 0x1
0x00000000x80ffc80 0x0000000000000200 0x1

0x00000000x80ffe80 0x0000000000000100 0x0
0x00000000x8000000 0x00000000000ffc80 0x0


0x00000000x8000000 0x0000000000100000 0x0

fanquake added a commit that referenced this pull request Nov 20, 2019
30fb598 Fix segfault in allocator_tests/arena_tests (Jeffrey Czyz)
15c84f5 Define ARENA_DEBUG in Travis test runs (Jeffrey Czyz)
ad71548 Fix compilation errors in support/lockedpool.cpp (Jeffrey Czyz)

Pull request description:

  Changes in #12048 cause a compilation error in Arena::walk() when
  ARENA_DEBUG is defined. Specifically, Arena's chunks_free map was
  changed to have a different value type.

  Additionally, missing includes cause other compilation errors when
  ARENA_DEBUG is defined.

  Reproduced with:

  make CPPFLAGS=-DARENA_DEBUG

ACKs for top commit:
  laanwj:
    ACK 30fb598
  fanquake:
    ACK 30fb598 - thanks for following up jkczyz.

Tree-SHA512: 4eec368a4e9c67e4e2a27bc05608a807c2892d50c60d06ed21490cd274c0369f9671bc05b3006acc2a193316caf4896454c9c299603bfed29bd488f1987ec446
@fanquake fanquake merged commit 30fb598 into bitcoin:master Nov 20, 2019
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 20, 2019
…ol.cpp

30fb598 Fix segfault in allocator_tests/arena_tests (Jeffrey Czyz)
15c84f5 Define ARENA_DEBUG in Travis test runs (Jeffrey Czyz)
ad71548 Fix compilation errors in support/lockedpool.cpp (Jeffrey Czyz)

Pull request description:

  Changes in bitcoin#12048 cause a compilation error in Arena::walk() when
  ARENA_DEBUG is defined. Specifically, Arena's chunks_free map was
  changed to have a different value type.

  Additionally, missing includes cause other compilation errors when
  ARENA_DEBUG is defined.

  Reproduced with:

  make CPPFLAGS=-DARENA_DEBUG

ACKs for top commit:
  laanwj:
    ACK 30fb598
  fanquake:
    ACK 30fb598 - thanks for following up jkczyz.

Tree-SHA512: 4eec368a4e9c67e4e2a27bc05608a807c2892d50c60d06ed21490cd274c0369f9671bc05b3006acc2a193316caf4896454c9c299603bfed29bd488f1987ec446
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 9, 2020
Summary:
PR12048:
5fbf7c4 fix nits: variable naming, typos (Martin Ankerl)
1e0ee90 Use best-fit strategy in Arena, now O(log(n)) instead O(n) (Martin Ankerl)

Pull request description:

  This replaces the first-fit algorithm used in the Arena with a best-fit. According to "Dynamic Storage Allocation: A Survey and Critical Review", Wilson et. al. 1995, http://www.scs.stanford.edu/14wi-cs140/sched/readings/wilson.pdf, both startegies work well in practice.

  The advantage of using best-fit is that we can switch the O(n) allocation to O(log(n)). Additionally, some previously O(log(n)) operations are now O(1) operations by using hash maps. The end effect is that the benchmark runs about 2.5 times faster on my machine:

      # Benchmark, evals, iterations, total, min, max, median
      old: BenchLockedPool, 5, 530, 5.25749, 0.00196938, 0.00199755, 0.00198172
      new: BenchLockedPool, 5, 1300, 5.11313, 0.000781493, 0.000793314, 0.00078606

  I've run all unit tests and benchmarks, and increased the number of iterations so that BenchLockedPool takes about 5 seconds again.

Tree-SHA512: 6551e384671f93f10c60df530a29a1954bd265cc305411f665a8756525e5afe2873a8032c797d00b6e8c07e16d9827465d0b662875433147381474a44119ccce

Also included PR16161 which fixes a bug introduced by PR12048:
  Changes in #12048 cause a compilation error in Arena::walk() when
  ARENA_DEBUG is defined. Specifically, Arena's chunks_free map was
  changed to have a different value type.

  Additionally, missing includes cause other compilation errors when
  ARENA_DEBUG is defined.

  Second commit fixes segfault in allocator_tests/arena_tests
  The test uses reinterpret_cast<void*> on unallocated memory. Using this
  memory in printchunk as char* causes a segfault, so have printchunk take
  void* instead.

  Reproduced with:

  make CPPFLAGS=-DARENA_DEBUG

Backport of Core PR12048 and PR16161
bitcoin/bitcoin#12048
bitcoin/bitcoin#16161

Reviewer note: the last commit in PR16161 is TRAVIS related and was therefore modified to our code base.

Test Plan:
  make check
  test_runner.py
  ./bitcoin-qt -> settings -> Encrypt Wallet -> set password and/or change password if already encrypted
  ./bench_bitcoin
old: BenchLockedPool, 5, 530, 5.15069, 0.00192059, 0.00199956, 0.00193796
new: BenchLockedPool, 5, 1300, 4.44411, 0.00067821, 0.000698535, 0.000678916

  ../configure CPPFLAGS=-DARENA_DEBUG
  make check
  test_runner.py
  ABC_BUILD_NAME=build-asan build-configurations.sh

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3898
zkbot added a commit to zcash/zcash that referenced this pull request Sep 29, 2020
Locked memory manager

Add a pool for locked memory chunks, replacing `LockedPageManager`.

Cherry-picked from the following upstream PRs:
- bitcoin/bitcoin#8321
- bitcoin/bitcoin#8753
- bitcoin/bitcoin#9063
- bitcoin/bitcoin#9070
- bitcoin/bitcoin#11385
- bitcoin/bitcoin#12048
  - Excludes change to benchmark.
- bitcoin/bitcoin#15117
- bitcoin/bitcoin#16161
  - Excludes Travis CI changes.
  - Includes change from bitcoin/bitcoin#13163
- bitcoin/bitcoin#15600
- bitcoin/bitcoin#18443
- Assorted small changes from:
  - bitcoin/bitcoin#9233
  - bitcoin/bitcoin#10483
  - bitcoin/bitcoin#10645
  - bitcoin/bitcoin#10969
  - bitcoin/bitcoin#11351
- bitcoin/bitcoin#19111
  - Excludes change to `src/rpc/server.cpp`
- bitcoin/bitcoin#9804
  - Only the commit for `src/key.cpp`
- bitcoin/bitcoin#9598
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 22, 2020
…ol.cpp

30fb598 Fix segfault in allocator_tests/arena_tests (Jeffrey Czyz)
15c84f5 Define ARENA_DEBUG in Travis test runs (Jeffrey Czyz)
ad71548 Fix compilation errors in support/lockedpool.cpp (Jeffrey Czyz)

Pull request description:

  Changes in bitcoin#12048 cause a compilation error in Arena::walk() when
  ARENA_DEBUG is defined. Specifically, Arena's chunks_free map was
  changed to have a different value type.

  Additionally, missing includes cause other compilation errors when
  ARENA_DEBUG is defined.

  Reproduced with:

  make CPPFLAGS=-DARENA_DEBUG

ACKs for top commit:
  laanwj:
    ACK 30fb598
  fanquake:
    ACK 30fb598 - thanks for following up jkczyz.

Tree-SHA512: 4eec368a4e9c67e4e2a27bc05608a807c2892d50c60d06ed21490cd274c0369f9671bc05b3006acc2a193316caf4896454c9c299603bfed29bd488f1987ec446
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 27, 2020
…ol.cpp

30fb598 Fix segfault in allocator_tests/arena_tests (Jeffrey Czyz)
15c84f5 Define ARENA_DEBUG in Travis test runs (Jeffrey Czyz)
ad71548 Fix compilation errors in support/lockedpool.cpp (Jeffrey Czyz)

Pull request description:

  Changes in bitcoin#12048 cause a compilation error in Arena::walk() when
  ARENA_DEBUG is defined. Specifically, Arena's chunks_free map was
  changed to have a different value type.

  Additionally, missing includes cause other compilation errors when
  ARENA_DEBUG is defined.

  Reproduced with:

  make CPPFLAGS=-DARENA_DEBUG

ACKs for top commit:
  laanwj:
    ACK 30fb598
  fanquake:
    ACK 30fb598 - thanks for following up jkczyz.

Tree-SHA512: 4eec368a4e9c67e4e2a27bc05608a807c2892d50c60d06ed21490cd274c0369f9671bc05b3006acc2a193316caf4896454c9c299603bfed29bd488f1987ec446
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
…ol.cpp

30fb598 Fix segfault in allocator_tests/arena_tests (Jeffrey Czyz)
15c84f5 Define ARENA_DEBUG in Travis test runs (Jeffrey Czyz)
ad71548 Fix compilation errors in support/lockedpool.cpp (Jeffrey Czyz)

Pull request description:

  Changes in bitcoin#12048 cause a compilation error in Arena::walk() when
  ARENA_DEBUG is defined. Specifically, Arena's chunks_free map was
  changed to have a different value type.

  Additionally, missing includes cause other compilation errors when
  ARENA_DEBUG is defined.

  Reproduced with:

  make CPPFLAGS=-DARENA_DEBUG

ACKs for top commit:
  laanwj:
    ACK 30fb598
  fanquake:
    ACK 30fb598 - thanks for following up jkczyz.

Tree-SHA512: 4eec368a4e9c67e4e2a27bc05608a807c2892d50c60d06ed21490cd274c0369f9671bc05b3006acc2a193316caf4896454c9c299603bfed29bd488f1987ec446
furszy added a commit to PIVX-Project/PIVX that referenced this pull request Sep 11, 2021
9295568 Fix segfault in allocator_tests/arena_tests (Jeffrey Czyz)
f7d88f5 Define ARENA_DEBUG in GA test runs (furszy)
cae2e13 Fix compilation errors in support/lockedpool.cpp (Jeffrey Czyz)
d19359f fix nits: variable naming, typos (Martin Ankerl)
5cbd967 Use best-fit strategy in Arena, now O(log(n)) instead O(n) (Martin Ankerl)
1ccd62e Fix out-of-bounds write in case of failing mmap(...) in PosixLockedPageAllocator::AllocateLocked (practicalswift)
7ad7157 LockedPool: avoid quadratic-time allocation (Kaz Wesley)
39e1b3b LockedPool: fix explosion for illegal-sized alloc (Kaz Wesley)
adb1f1c LockedPool: test handling of invalid allocations (Kaz Wesley)
626b865 Do not shadow variable, use deprecated MAP_ANON if MAP_ANONYMOUS is not defined. (Pavel Janík)

Pull request description:

  Follow up to #2276. Completing the locked memory manager todo list.

  Back porting:
  * bitcoin#9063.
  * bitcoin#9070.
  * bitcoin#12048.
  * bitcoin#15117.
  * bitcoin#16161.

ACKs for top commit:
  random-zebra:
    ACK 9295568
  Fuzzbawls:
    ACK 9295568

Tree-SHA512: 0d14c7e8231386ee32f1fefac08de9ee278c02d6b0e5172c055851d33c803b0be3398fc5173457e539179712a13d4736033a926f318e576789b4d57b67eb0596
@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.

5 participants