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

Fix runtime crash when parsing /proc/cpuinfo fails #1900

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

hdeller
Copy link
Contributor

@hdeller hdeller commented Jan 8, 2025

The testcase fails on sparc64, because the parsing of /proc/cpuinfo fails and thus currently returns "0" CPUs which finally leads to division-by-zero faults in the tests.

Fix the issue by returning at least "1" CPU which allows the tests to run. A error message will be printed in any case.

Long-term the code should be fixed to parse the cpuinfo output on sparch which looks like this:
...
type : sun4v
ncpus probed : 48
ncpus active : 48

The testcase fails on sparc64, because the parsing of /proc/cpuinfo
fails and thus currently returns "0" CPUs which finally leads
to division-by-zero faults in the tests.

Fix the issue by returning at least "1" CPU which allows the
tests to run. A error message will be printed in any case.

Long-term the code should be fixed to parse the cpuinfo output
on sparch which looks like this:
...
type            : sun4v
ncpus probed    : 48
ncpus active    : 48
@hdeller
Copy link
Contributor Author

hdeller commented Jan 8, 2025

A failing log on a sparc64 machine can be found here:
https://buildd.debian.org/status/fetch.php?pkg=benchmark&arch=sparc64&ver=1.8.4-1%7Eexp1&stamp=1719401179&raw=0

Currently I suggest to apply this trivial patch as-is to get rid of the problem for now.

In the long run I see two options:
a) Either add code to parse /proc/cpuinfo on sparc (as written in the commit message), or
b) drop all code which parses /proc/cpuinfo, and instead switch to calling "sysconf(_SC_NPROCESSORS_ONLN)" (see man-page).

I suggest to implement b).

@@ -561,10 +561,12 @@ int GetNumCPUsImpl() {
}

int GetNumCPUs() {
const int num_cpus = GetNumCPUsImpl();
int num_cpus = GetNumCPUsImpl();
if (num_cpus < 1) {
std::cerr << "Unable to extract number of CPUs. If your platform uses "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this instead be an assert that would require platform support to be added?

Copy link
Contributor Author

@hdeller hdeller Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally don't like an asserts, unless there is a really CRITICAL problem.
Not being able to correctly detect the number of CPUs is not at all a critical problem.
Currently it prints that the number of CPUs could not be determined at that someone may take a look at it.
With my addition it will then will return 1 CPU (which is of course somewhat true since we wouldn't be here if there was no CPU, although maybe not the perfect answer).

(... or even better use sysconf(_SC_NPROCESSORS_ONLN) in an upcoming patch.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's fair :)

@dmah42 dmah42 merged commit 39be87d into google:main Jan 9, 2025
75 of 86 checks passed
@hdeller hdeller deleted the sparc-crash-fix branch January 9, 2025 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants