Skip to content

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

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.

Sign up for GitHub

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

Merged
dmah42 merged 1 commit into from
Jan 9, 2025
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/sysinfo.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@dmah42 dmah42 Jan 9, 2025

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 •
edited
Loading

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

@dmah42 dmah42 Jan 9, 2025

Choose a reason for hiding this comment

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

that's fair :)

"/proc/cpuinfo, custom support may need to be added.\n";
/* There is at least one CPU which we run on. */
num_cpus = 1;
}
return num_cpus;
}
Expand Down
Loading