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
How about a test case?
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.
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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
19 | - // without actually using memory. 20 | - void *synth_base = reinterpret_cast<void*>(0x08000000); 21 | const size_t synth_size = 1024*1024; 22 | - Arena b(synth_base, synth_size, 16); 23 | + std::unique_ptr<char[]> synth_base(new char[synth_size]()); 24 | + Arena b(synth_base.get(), synth_size, 16);
Using a fake address here was on purpose, it would catch accesses to the underlying memory by causing a segfault. Also it leaks a MB of memory now.
Using a fake address here was on purpose, it would catch accesses to the underlying memory by causing a segfault.
Ah, that wasn't so clear to me from the code comment. Mission accomplished, I suppose. :) Since the access occurs when calling walk, those calls aren't very useful in this test given they cause a segfault.
Also it leaks a MB of memory now.
Won't the memory be deleted when the unique_ptr goes out of scope since I'm using get and not release?
That said, I see the fake address is used later in the test, so perhaps it would be better to leave it as it was and remove the calls to walk given the segfault. Not sure what value walk has if never called though. Thoughts?
I'm slightly confused. I don't think walk is supposed to access the contents of the memory, it is supposed to be a debugging function that prints the allocation metadata, addresses and such, which is intentionally stored outside of the arena (to save on scarce page-locked memory). If it accesses the underlying memory, or even dereferences the pointer, that seems like a bug to me.
I'm not sure why, but Inserting base into std::cout inside printchunk causes the segfault when executed with make CPPFLAGS=-DARENA_DEBUG check:
Entering test module "Bitcoin Core Test Suite"
test/allocator_tests.cpp(14): Entering test suite "allocator_tests"
test/allocator_tests.cpp(16): Entering test case "arena_tests"
0xunknown location(0): fatal error: in "allocator_tests/arena_tests": memory access violation at address: 0x080ffc10: no mapping at fault address
test/allocator_tests.cpp(16): last checkpoint: "arena_tests" entry.
Test is aborted
test/allocator_tests.cpp(16): Leaving test case "arena_tests"; testing time: 8196us
Test is aborted
test/allocator_tests.cpp(14): Leaving test suite "allocator_tests"; testing time: 8231us
Test is aborted
The segfault does not occur if I remove base from being inserted into the stream.
Thanks for catching this and fixing the build with -DARENA_DEBUG, not 100% sure about the build system and test changes.
<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase
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.
Concept ACK
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
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.
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.
ACK 30fb598737f6efb7802d707a1fa989872e7f8b7b
I like how minimal this change has become.
ACK 30fb598737f6efb7802d707a1fa989872e7f8b7b - 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