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

Build error: read-only reference ‘value’ used as ‘asm’ output #2343

Closed
eyalroz opened this issue Oct 3, 2023 · 10 comments · Fixed by #2354
Closed

Build error: read-only reference ‘value’ used as ‘asm’ output #2343

eyalroz opened this issue Oct 3, 2023 · 10 comments · Fixed by #2354
Assignees
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@eyalroz
Copy link

eyalroz commented Oct 3, 2023

Describe your environment

  • SLES 15 SP 1
  • g++ 7.4.1
  • Kernel 4.12.14-195-default
  • googletest-1.14.0 manually downloaded built and installed under /opt/gtest
  • Google benchmark 1.8.3 manually downloaded built and installed under /opt/benchmark
  • CMake 3.27.1 manually downloaded and installed under
  • main branch, commit 0803e6a .

Steps to reproduce

  • Used CMake to configure
  • Used CMake to build

What is the expected behavior?

Build should have concluded successfully

What is the actual behavior?

During build, got the error:

In file included from /path/to/src/opentelemetry-cpp/sdk/test/trace/sampler_benchmark.cc:17:0:
/opt/benchmark/include/benchmark/benchmark.h: In instantiation of ‘typename std::enable_if<((! std::is_trivially_copyable<_Tp>::value) || (sizeof (Tp) > sizeof (Tp*)))>::type benchmark::DoNotOptimize(Tp&&) [with Tp = opentelemetry::v1::sdk::trace::ParentBasedSampler; typename std::enable_if<((! std::is_trivially_copyable<_Tp>::value) || (sizeof (Tp) > sizeof (Tp*)))>::type = void]’:
/path/to/src/opentelemetry-cpp/sdk/test/trace/sampler_benchmark.cc:49:85:   required from here
/opt/benchmark/include/benchmark/benchmark.h:540:46: error: read-only reference ‘value’ used as ‘asm’ output
   asm volatile("" : "+m"(value) : : "memory");
                                              ^
/opt/benchmark/include/benchmark/benchmark.h: In instantiation of ‘typename std::enable_if<((! std::is_trivially_copyable<_Tp>::value) || (sizeof (Tp) > sizeof (Tp*)))>::type benchmark::DoNotOptimize(Tp&&) [with Tp = opentelemetry::v1::sdk::trace::TraceIdRatioBasedSampler; typename std::enable_if<((! std::is_trivially_copyable<_Tp>::value) || (sizeof (Tp) > sizeof (Tp*)))>::type = void]’:
/path/to/src/opentelemetry-cpp/sdk/test/trace/sampler_benchmark.cc:58:60:   required from here
/opt/benchmark/include/benchmark/benchmark.h:540:46: error: read-only reference ‘value’ used as ‘asm’ output

Additional context

N/A

@eyalroz eyalroz added the bug Something isn't working label Oct 3, 2023
@github-actions github-actions bot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Oct 3, 2023
@marcalff
Copy link
Member

marcalff commented Oct 3, 2023

@eyalroz
Copy link
Author

eyalroz commented Oct 3, 2023

@marcalff : So, is this also a simple matter of calling the right overload? Or perhaps const-casting? Anyway, I would gladly try to build from a bugfix branch.

@marcalff
Copy link
Member

marcalff commented Oct 3, 2023

First, please clarify which version of opentelemetry-cpp is used.

Regarding the error seen, the content of /opt/benchmark/include/benchmark/benchmark.h:540 is

https://github.com/google/benchmark/blob/main/include/benchmark/benchmark.h#L535-L541

template <class Tp>
inline BENCHMARK_ALWAYS_INLINE
    typename std::enable_if<!std::is_trivially_copyable<Tp>::value ||
                            (sizeof(Tp) > sizeof(Tp*))>::type
    DoNotOptimize(Tp&& value) {
  asm volatile("" : "+m"(value) : : "memory");
}

This sounds like a bug in google benchmark itself: if the std::enable_if condition is satisfied, then the method implemented inline should compile, and it does not.

Now, this header files has a few ifdef, in particular with BENCHMARK_HAS_CXX11 and friends.

Please clarify, and double check, which c++ options are used for this build (C++11/C++14/C++17/etc).

@psmoot
Copy link

psmoot commented Oct 4, 2023

I encountered this issue too.

I'm using OpenSUSE Leap 15.4 with gcc 7.5.0. I cloned the opentelemtry-cli repo at commit 0eaa794.

I tried this twice, once setting -DCMAKE_CXX_STANDARD=17 and once -DCMAKE_CXX_STANDARD=14. I had the same issue without setting the C++ standard at all. I'd let you know the versions of gtest and benchmark but TBH, I don't know how to figure those out.

@eyalroz
Copy link
Author

eyalroz commented Oct 4, 2023

@marcalff : Ah, yes, sorry. I followed the INSTALL.md instructions and used the main branch. See edit of initial comment.

@marcalff marcalff self-assigned this Oct 6, 2023
@marcalff
Copy link
Member

marcalff commented Oct 9, 2023

After investigations, it turns out that:

  • building with both benchmark 1.8.2 and benchmark 1.8.3 fails.
  • building with both opentelemetry-cpp 1.11.0 and main (0eaa794) fails

It seems this bug has been present for a long time, this is not a recent regression (opentelemetry-cpp changes to abandon C++11, or recent changes in benchmark).

Failed to build with:

  • g++-12 (SUSE Linux) 12.3.0 (verified)
  • g++-7 (SUSE Linux) 7.5.0 (verified)
  • g++ 7.4.1 (reported)

@marcalff
Copy link
Member

marcalff commented Oct 9, 2023

Filed upstream as:

as this looks like a problem in the benchmark header files with the work around implemented for GCC.

marcalff added a commit to marcalff/opentelemetry-cpp that referenced this issue Oct 9, 2023
@psmoot
Copy link

psmoot commented Oct 9, 2023

@marcalff : Can you suggest a workaround while an actual fix is in progress?

For my purposes, I could skip benchmarking entirely. I just need it to work, working fast can be deferred.

I'm also quite rusty with CMake so short, simple instructions would be greatly appreciated.

@marcalff
Copy link
Member

marcalff commented Oct 9, 2023

@psmoot

An obvious work around is to skip benchmarks entirely, which requires no code change:

cmake -DWITH_BENCHMARK=OFF

@eyalroz
Copy link
Author

eyalroz commented Oct 9, 2023

@marcalff : Thanks for that. Just remember that what's obvious for project developers is not always obvious to users, so it's a good thing to point this kind of stuff out when bugs are filed.

@marcalff marcalff added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants