google / benchmark

A microbenchmark support library
Apache License 2.0
8.61k stars 1.57k forks source link

pfm: Use a more standard CMake approach for finding libpfm #1628

Closed chipot closed 1 year ago

chipot commented 1 year ago

This PR is trying to address https://github.com/google/benchmark/issues/1627

I reworked FindPFM.cmake to use find_library and find_path instead of check_library_exists and check_include_file. The main problem with these two is that they expect CFLAGS and LDFLAGS to be set correctly before the call.

The module now creates an IMPORTED target PFM::libpfm and we can link to it as expected.

google-cla[bot] commented 1 year ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

dmah42 commented 1 year ago

fantastic! i wonder if this is why the CI bots fail the pfm tests... cc @macandy13

dmah42 commented 1 year ago

CI failures are unrelated

dmah42 commented 1 year ago

thanks!

macandy13 commented 1 year ago

fantastic! i wonder if this is why the CI bots fail the pfm tests... cc @macandy13

Hm, I don't see test failures related to pfm. I also think the pfm tests are never actually run on the CI machines, because they do not support performance counters at all (which is what I tried in one of the recent prs but sadly had to remove from the pr again).

Am I missing something?

dmah42 commented 1 year ago

i was suggesting that the current version of this code that doesn't do a great job of finding pfm might be the reason why the CI machines can't run the tests. maybe with this change they can?

macandy13 commented 1 year ago

When I tried the tests, the problem was not that libpfm wasn't found (I was using bazel for doing this), it was that the underlying VM does not report any performance counters at all. The pfm tests actually expect that the system at least reports a few standard ones.

Do you have a link to the error you were seeing?

dmah42 commented 1 year ago

i didn't check any errors, i was just being optimistic.

macandy13 commented 1 year ago

Ok. For the record, this is the failure I was seeing when trying to enable pfm CI tests: https://github.com/google/benchmark/actions/runs/5484545652/jobs/9992228532#step:5:2830

[ RUN      ] PerfCountersTest.OneCounter
***WARNING** Failed to get a file descriptor for performance counter CYCLES. Ignoring
test/perf_counters_gtest.cc:36: Failure
Expected equality of these values:
  PerfCounters::Create({kGenericPerfEvent1}).num_counters()
    Which is: 0
  1
[  FAILED  ] PerfCountersTest.OneCounter (0 ms)