Fix memory violation when processing cmd line args in the driver

[aprokop]

No description provided.

[aprokop]

This may not fix all the problems, looking further.

[aprokop]

More investigations show that this only happens when you have more than one benchmark arguments. For example, --benchmark_filter=OpenMP* --benchmark_color=false. The presence of our arguments does not seem to matter.

[aprokop]

OK. So, the problem is these two functions:

  int &argc() { return _argc; }                                                                                                                               
  char **argv() { return _argv.data(); }   

What happens is that internal members of CmdLineArgs are changed during benchmark parsing. Benchmark decreases argc, and (probably) puts nullptr into the argv members (or simply frees them). But it does not change argv size as it does not know about it. Thus, you will get double delete[] corresponding to removed benchmark options.

A clear indicator of this was the fact that _argc and _argv.size() were different when passing benchmark options to the executable.

[dalg24]

@aprokop thanks for the explanation

Please add a comment to capture that and add the curly braces you removed from the for-loop back

[aprokop]

Will do. It's funny that the second patch was exactly the right fix for this.

[masterleinad]

This is the guilty code: https://github.com/google/benchmark/blob/f92903cc5338daed898242f22015d8426c065770/src/benchmark.cc#L456-L458
I still don't understand why that doesn't create a memory leak, though.

[masterleinad]

Running

 valgrind --leak-check=full examples/bvh_driver/ArborX_BoundingVolumeHierarchy.exe --benchmark_filter=Serial --values=10 --benchmark_color=false

I still get a memory leak:

==8214== 
==8214== HEAP SUMMARY:
==8214==     in use at exit: 370 bytes in 6 blocks
==8214==   total heap usage: 249,106 allocs, 249,100 frees, 78,973,521 bytes allocated
==8214== 
==8214== 50 bytes in 2 blocks are definitely lost in loss record 3 of 5
==8214==    at 0x4C3089F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8214==    by 0x11BC6D: CmdLineArgs::CmdLineArgs(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, char const*) (bvh_driver.cpp:267)
==8214==    by 0x117BDC: main (bvh_driver.cpp:323)
==8214== 
==8214== LEAK SUMMARY:
==8214==    definitely lost: 50 bytes in 2 blocks
==8214==    indirectly lost: 0 bytes in 0 blocks
==8214==      possibly lost: 0 bytes in 0 blocks
==8214==    still reachable: 320 bytes in 4 blocks
==8214==         suppressed: 0 bytes in 0 blocks
==8214== Reachable blocks (those to which a pointer was found) are not shown.
==8214== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==8214== 
==8214== For counts of detected and suppressed errors, rerun with: -v
==8214== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

[aprokop]

@masterleinad You are right, it makes no sense.

[aprokop]

So it seems what actually happens is benchmark overwriting the addresses and they never get deallocated. So, we actually need to store an extra array (or return a copy for values).