potswa / cxx_function

Prototype for new std::function features, compatible with C++11.
MIT License
46 stars 7 forks source link

Assertion failed: (pool[ 2 ] == op.state.capacity() + 1 + sizeof (stateful_op)), function main, file scoped_allocator.cpp, line 106. #14

Open graphicsMan opened 6 years ago

graphicsMan commented 6 years ago

When running the scoped_allocator test with -O3 on my mac, I get the stated assertion. My g++ appears to be clang++ masquerading as g++, version as follows:

$ g++ --version Configured with: --prefix=/Applications/Xcode_9.3.0_fb.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1 Apple LLVM version 9.1.0 (clang-902.0.39.1) Target: x86_64-apple-darwin17.7.0 Thread model: posix InstalledDir: /Applications/Xcode_9.3.0_fb.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

potswa commented 6 years ago

OK, poking around on Wandbox, I find that its prerelease Clang 8 and Clang 7.0.0 compilers pass the test with -O0 through -O3, but Clang 6.0.1 fails the test at -O3 after exhibiting memory corruption. (All the failures are the same assertion that you mentioned. So far, I'm testing without your change. These results suggest you didn't break anything :) .)

GCC does worse. Version 6.3.0 passes at -O0, -O2, and -O3, but fails at -O1. Versions 7.3.0 and 8.1.0 pass at -O0 and fail at any optimization level. GCC 8.2.0 and 9.0 prerelease consistently run the test wrong; this appears to be a change in the library behavior.

Clang never reports the warning that you mention, but it is reported by GCC since 8.1.0. So the result of g++ -v is confusing, because it indicates that Clang is emulating an older GCC (4.2.1), when your results look more like a genuine and newer GCC. Unfortunately I do not have an up-to-date macOS or XCode to dig deeper, but maybe these details are irrelevant…

TL;DR: I need to work on GCC compatibility, but you should still get good results from Clang.

graphicsMan commented 6 years ago

I've been using unique_function without issue (after my change), but unless unique_function uses scoped_allocator, I haven't been using it.

potswa commented 6 years ago

The most likely source of UB in the library is this memcpy. The scoped_allocator test is just giving it a bit of exercise. It's possible that the UB is coming from a bug in the scoped_allocator, but until that's proven, I'd assume that scoped_allocator is only setting up an alias analysis mistake in the compiler, which could possibly affect your code as well.

potswa commented 6 years ago

No worries, though, none of this counts against your pull request. I'm just getting a handle on the current state of the library, so bear with me.

graphicsMan commented 6 years ago

I actually was able to find some stack use after scope issues with address sanitizer. I wasn't able to find anything with the undefined behavior sanitizer. Here's a simple makefile you can put in the test directory:

objects = align constructible ctorthrow overlambda usernew badcopy container heap ptm swap volatile callmove convnull nocopy recover target_access scoped_allocator all: $(objects)

CXXFLAGS=-Og -std=c++14 -I../ -Wall -fsanitize=address

$(objects): %: %.cpp clang++ $(CXXFLAGS) -o $@.exe $< ./$@.exe

potswa commented 6 years ago

Fixed by 0a7c31055.

potswa commented 6 years ago

This fix solves the issue on older compilers. It does not fix recent GCC.

graphicsMan commented 6 years ago

Thanks David. Is it a solution or a workaround? I actually can't distinguish a difference between the old code and the new code.

potswa commented 6 years ago

There are two separate causes: An issue with the lifetime extension rules of older compilers, and (I suspect) an issue in newer GCC libstdc++. So if you are using GCC, you'll still see this issue.

I guess it's better to leave this ticket open until we know exactly what's happening with GCC. Historically, allocators don't get tested very well in standard libraries and bugs are common. To anyone reading this open issue, if you are using scoped_allocator_adaptor, try Clang instead.