Skip to content

Add inline, remove unused exception variable name, safe float computation#324

Open
mnorris11 wants to merge 1 commit intointel:mainfrom
mnorris11:main
Open

Add inline, remove unused exception variable name, safe float computation#324
mnorris11 wants to merge 1 commit intointel:mainfrom
mnorris11:main

Conversation

@mnorris11
Copy link
Copy Markdown

@mnorris11 mnorris11 commented Apr 29, 2026

This is a patch to get it to work internally. It would be easier if things are in SVS, so next version upgrade we don't need to apply this patch.

Reason for changes:

  1. inlining: This is from -Werror,-Winline-namespace-reopened-noninline.
  2. Unused exception parameter: -Werror,-Wunused-exception-parameter.
  3. Float change: acccording to AI: the shift 125 - e goes out of range for uint32_t when e is outside [102, 112]. The result is multiplied by zero so it's "dead" at runtime, but the shift itself is still UB. UBSAN catches it and 5 FP16 tests fail: WriteAndReadIndexSVSFP16, VamanaFP16TrainSaveLoadAndAdd, WriteAndReadStaticIVFFP16, WriteAndReadIndexSVSIVFFP16, IVFFP16TrainAndAdd.
  4. index.h Thread clamping: per AI: "compute_centroid_distances (in common.h:675) partitions centroids across threads using StaticPartition(num_centroids). If you have more threads than centroids, some threads get an empty partition. The callback then calls batch.start() and batch.size() on those empty ranges, leading to OOB access into centroids.get_datum() and matmul_results[tid]. The fix clamps the thread pool size to at most the number of centroids at construction time, so every thread gets at least one centroid to work on."
  5. sorted_buffer.h change: " back() returns candidates_[size_ - 1]. When the buffer is empty (size_ == 0), that underflows to candidates_[SIZE_MAX] — an OOB read. The original code evaluated back() before checking full(), so on an empty buffer it would access garbage memory before full() returned false and short-circuited the &&. Swapping the operands ensures full() is checked first; if the buffer isn't full, back() is never called. Classic short-circuit evaluation fix."

Copy link
Copy Markdown
Member

@rfsaliev rfsaliev left a comment

Choose a reason for hiding this comment

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

Thank you @mnorris11 , for the contribution.
There are some suggestions, proposals and comments

Comment thread include/svs/lib/float16.h
static_cast<uint32_t>(e > 112) * ((((e - 112) << 10) & 0x7C00) | m >> 13) |
static_cast<uint32_t>((e < 113) && (e > 101)) *
((((0x007FF000 + m) >> (125 - e)) + 1) >> 1) |
((((0x007FF000 + m) >> (125 - safe_e)) + 1) >> 1) |
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm, in case when (e < 101 || e > 113), this component is always 0:

return (b & 0x80000000) >> 16 |
           ... |
           static_cast<uint32_t>((e < 113) && (e > 101)) *
               ((((0x007FF000 + m) >> (125 - safe_e)) + 1) >> 1) |

// equal to:
return (b & 0x80000000) >> 16 |
           ... |
           static_cast<uint32_t>(e > 101 && e < 113) *
               ((((0x007FF000 + m) >> (125 - safe_e)) + 1) >> 1) |
// equal to:
return (b & 0x80000000) >> 16 |
           ... |
           static_cast<uint32_t>(false) *
               ((((0x007FF000 + m) >> (125 - safe_e)) + 1) >> 1) |
//equal to:
return (b & 0x80000000) >> 16 |
           ... |
           0 *
               ((((0x007FF000 + m) >> (125 - safe_e)) + 1) >> 1) |

try {
unsafe_assign(fn);
} catch (const ThreadCrashedError& err) {
} catch (const ThreadCrashedError&) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
} catch (const ThreadCrashedError&) {
} catch (const ThreadCrashedError& SVS_UNUSED(err)) {

Comment on lines +858 to +861
throw ANNEXCEPTION(
"Expected to get an exception from a crashed thread but no "
"exception was thrown!"
);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please, use SVS formatting rules.

Comment on lines 33 to +35
namespace svs {
namespace runtime {
namespace v0 {
inline namespace v0 {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMHO, this change will negatively affect usability and maintainability for further API major versions.
Instead, a macro can be used here which value defined in version.h depending on SVS_RUNTIME_API_VERSION, e.g.:

#if (SVS_RUNTIME_API_VERSION == 0)
...
#define SVS_API_V0 inline namespace v0
...
#endif

With usage aka:

Suggested change
namespace svs {
namespace runtime {
namespace v0 {
inline namespace v0 {
namespace svs {
namespace runtime {
SVS_API_V0 {
...
} // SVS_API_V0

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