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.

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

index: Fix volk_get_index #519

Closed
wants to merge 3 commits into from
Closed

index: Fix volk_get_index #519

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c452aec
index: Fix volk_get_index
jdemel Sep 26, 2021
8fe7ed8
index: Restructure volk_get_index
jdemel Oct 2, 2021
c1ed567
index: Revert to !strncmp
jdemel Oct 2, 2021
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
42 changes: 32 additions & 10 deletions lib/volk_rank_archs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* -*- c++ -*- */
/*
* Copyright 2011-2012 Free Software Foundation, Inc.
* Copyright 2011-2012, 2021 Free Software Foundation, Inc.
*
* This file is part of VOLK
*
Expand All @@ -15,21 +15,43 @@
#include <volk/volk_prefs.h>
#include <volk_rank_archs.h>

int volk_get_index(const char* impl_names[], // list of implementations by name
const size_t n_impls, // number of implementations available
const char* impl_name // the implementation name to find
)
// This function is supposed to be private to this compilation unit.
int volk_search_index(const char* impl_names[],
const size_t n_impls,
const char* impl_name)
{
unsigned int i;
for (i = 0; i < n_impls; i++) {
if (!strncmp(impl_names[i], impl_name, 20)) {
if (!strncmp(impl_names[i], impl_name, 42)) {
jdemel marked this conversation as resolved.
Show resolved Hide resolved
return i;
}
}
// TODO return -1;
// something terrible should happen here
fprintf(stderr, "Volk warning: no arch found, returning generic impl\n");
return volk_get_index(impl_names, n_impls, "generic"); // but we'll fake it for now
return -1; // Indicate we couldn't find anything!
}

int volk_get_index(const char* impl_names[], // list of implementations by name
const size_t n_impls, // number of implementations available
const char* impl_name // the implementation name to find
)
{
/*
* We follow a 3 step process.
* 1. Search for the requested impl. Return index if found.
* 2. Search for the generic impl. Return as fail-safe.
* 3. Return -1 to indicate an error. Caller needs to handle this case.
*/
int idx = volk_search_index(impl_names, n_impls, impl_name);
if (idx >= 0) {
return idx;
}

idx = volk_search_index(impl_names, n_impls, "generic");
if (idx >= 0) {
fprintf(stderr, "Volk warning: no arch found, returning generic impl\n");
} else {
fprintf(stderr, "Volk ERROR: no arch found. generic impl missing!\n");
}
return idx;
Copy link
Member

Choose a reason for hiding this comment

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

we're just bubbling up the -1 here in case of error, but we need to handle that. Otherwise, we'll jump into some random memory location when the consumer program actually calls the kernel. That's worse than an infinite loop.

So, this is half the fix so far.

Copy link
Member

Choose a reason for hiding this comment

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

at the very least, __init_${kern.name} needs to check for -1 and abort in that case. Failing early instead of wandering into random memory, where we become a stability or security issue, is necessary. Otherwise, we're just making bad code worse!

Copy link
Member

@marcusmueller marcusmueller Oct 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

also note that __init_.. casts the int that volk_rank_archs returns into a size_t, which is unsigned, and potentially of a different size!

}

int volk_rank_archs(const char* kern_name, // name of the kernel to rank
Expand Down