fix gcc7.5 compile error

[tpoisonooo]

Context

I tried to compile google/uVkCompute with GCC7.5, got some error:

...
uVkCompute/third_party/benchmark/include/benchmark/benchmark.h:1204:41: error: field 
‘benchmark::RegisterBenchmark(const char*, Lambda&&, Args&& ...) [with Lambda = void (&)(benchmark::State&, 
uvkc::vulkan::Device*, uvkc::benchmark::LatencyMeasureMode, const double*, const unsigned int*, long unsigned int, 
long unsigned int, int, double*); Args = {uvkc::vulkan::Device*&, uvkc::benchmark::LatencyMeasureMode&, const 
double*&, const unsigned int* const&, long unsigned int, long unsigned int&, long unsigned int, double*&}]::
<lambda(benchmark::State&)>::<fn capture>’ invalidly declared function type

...

The key problem is construct lambda in gcc7.5, please build this snippet and have a try:

#include <iostream>

template <class Lambda>
void RegisterBenchmark(Lambda&& fn) {
  fprintf(stdout, "first version\n");
}

template <class Lambda, class... Args>
void RegisterBenchmark(Lambda&& fn, Args&&... args) {

  // no auto decay in g++7.5, crash !!!
  auto fff = [=](){ fn(args...); };

  fff = [=, gn=std::forward<Lambda>(fn)](){ gn(args...); };
  fff = [=, gn=std::decay_t<Lambda>(fn)](){ gn(args...); };
  RegisterBenchmark(std::move(fff)); 
}


int func(int a) {
  fprintf(stdout, "%d\n", a);
}

int main() {
RegisterBenchmark(func, 12);
return 0;
}

auto fff = [=](){ fn(args...); }; is fine in clang/gcc11, but does not work in gcc7.5.

please check

• https://stackoverflow.com/questions/34815698/c11-passing-function-as-lambda-parameter
• Build Fail on Jetson Nano uVkCompute#15

[LebedevRI]

Can you point me at the test within this repo that fails when you build the benchmark itself with GCC7.5?
If none, please add one.

[tpoisonooo]

Can you point me at the test within this repo that fails when you build the benchmark itself with GCC7.5? If none, please add one.

I have build google/benchmark with commit-id 0d98dba (uVKCompute use it) and 60b16f1 (the newest one), no error happens.

[tpoisonooo]

+_+

[LebedevRI]

Can you point me at the test within this repo that fails when you build the benchmark itself with GCC7.5? If none, please add one.

I have build google/benchmark with commit-id 0d98dba (uVKCompute use it) and 60b16f1 (the newest one), no error happens.

Then please add a test that fails without this fix :)

[tpoisonooo]

Can you point me at the test within this repo that fails when you build the benchmark itself with GCC7.5? If none, please add one.

I have build google/benchmark with commit-id 0d98dba (uVKCompute use it) and 60b16f1 (the newest one), no error happens.

Then please add a test that fails without this fix :)

Thanks for your guidance, testcase added, RegisterBenchmark a static function with GCC7.5 can reproduce it.

It seems that lambda is not compatible with compiler, use std::bind could fix it, no need BENCHMARK_HAS_CXX14.

According to absl-cpp avoid std::bind, google dev prefer absl::bind ? I am not sure, if absl::bind is a must, pls let me know.

[LebedevRI]

Can you point me at the test within this repo that fails when you build the benchmark itself with GCC7.5? If none, please add one.

I have build google/benchmark with commit-id 0d98dba (uVKCompute use it) and 60b16f1 (the newest one), no error happens.

Then please add a test that fails without this fix :)

Thanks for your guidance, testcase added, RegisterBenchmark a static function with GCC7.5 can reproduce it.

It seems that lambda is not compatible with compiler, use std::bind could fix it, no need BENCHMARK_HAS_CXX14.

According to absl-cpp avoid std::bind, google dev prefer absl::bind ? I am not sure, if absl::bind is must, pls let me know.

I'm not sure why what is the problem with just defining the macro?

[tpoisonooo]

Ah...which "macro" ? no macro now.

I have added static void StaticFunction(::benchmark::State& state, int) {} to reproduce the problem and use std::bind to fix it, please check the code.

[LebedevRI]

I believe the previous fix is much less controversial.

[tpoisonooo]

I believe the previous fix is much less controversial.

Ah I got it ... I have tried to add BENCHMARK_HAS_CXX14 in benchmark.h

#if __cplusplus >= 201402L || (defined(_MSVC_LANG) && _MSVC_LANG >= 201402L)
#define BENCHMARK_HAS_CXX14
#endif
...
#if BENCHMARCK_HAS_CXX14
...
#elif BENCHMARCK_HAS_CXX11
...
#endif

But the default compile option is std=c++11, so RegisterBenchmark a static function still failed.

$ cd benchmark/build
$ make VERBOSE=1
....  -std=c++11 ....

That means GCC7.5 -std=c++11 could not use lamda, use #if BENCHMARK_HAS_CXX14 or not does not make any sense...

[LebedevRI]

Err, i'm being stupid again.
Of course, the check shouldn't be for the C++17, it should be for GCC version :/
So roughly i think it should be

#if defined(BENCHMARK_GCC_VERSION) && BENCHMARK_GCC_VERSION < 800
<the code with explicit move>
#else
<original code>
#endif

[tpoisonooo]

for clang/gcc11, it's fine;
for c++14/c++17, explicit move is fine;

for c++11, lamda not support capture initializer, for example this code not work:

int value1=1;
auto x = [=, value2=value1](){...}

[tpoisonooo]

That is why I use std::bind.

[tpoisonooo]

Err... is this PR acceptable ? @LebedevRI

[dmah42]

ping for @LebedevRI to comment :)

[LebedevRI]

I think there may be a translation issue here.

This sounds like a GCC7 bug, so the fix should not affect anything else.
Roughly, if you change the diff to the following, i'd stamp:

template <class Lambda, class... Args>
internal::Benchmark* RegisterBenchmark(const char* name, Lambda&& fn,
                                       Args&&... args) {
+#if !defined(__GNUC__) || (defined(BENCHMARK_GCC_VERSION) && BENCHMARK_GCC_VERSION >= 800) || __cplusplus >= 201402L
  return benchmark::RegisterBenchmark(
      name, [=](benchmark::State& st) { fn(st, args...); });

+#else
+  // Workaround GCC7.5 C++ bug, see https://github.com/google/benchmark/pull/1379.
+  auto func = std::bind(std::forward<Lambda>(fn), std::placeholders::_1,
+                        std::forward<Args>(args)...);
+  return benchmark::RegisterBenchmark(name,
+                                      [=](benchmark::State& st) { func(st); });
+#endif
}