Fix dead-store in intersect_line_with_prism slist dedup#83
Open
Luochenghuang wants to merge 1 commit into
Open
Fix dead-store in intersect_line_with_prism slist dedup#83Luochenghuang wants to merge 1 commit into
Luochenghuang wants to merge 1 commit into
Conversation
The duplicate-removal step at the end of intersect_line_with_prism wrote deduped values into a stack VLA slist_unique and then did 'slist = slist_unique;' before returning the new count. But slist is a pass-by-value pointer parameter, so this assignment only rebinds the local variable — the caller's buffer is never touched, and slist_unique is destroyed when the function returns. The caller then read the first num_unique_elements entries of the sorted-but-undeduped buffer rather than the actual deduped values. In intersect_line_segment_with_prism this corrupts the inside/outside parity walk: a near-duplicate that the dedup loop intended to drop remains in the buffer (producing a phantom thin 'inside' sliver of width ~1e-3*|s|), and a real far-end crossing is silently dropped because the caller stops at the smaller returned count. Worst case error is on the order of the prism diameter when an exit crossing is lost and parity inverts for the remainder of the segment. The bug only triggers when the qsort'd slist contains two values within 1e-3*|s| of each other, which happens for edge-grazing rays, rays through or near a shared vertex, and near-tangent rays where floating-point noise produces paired hits. test-prism's random-ray distribution does not statistically hit the 1e-3 window often enough to flag this in the existing comparison-vs-block tests, which is why the bug has lived in tree across PR NanoComp#75 (which only changed slist sizing/fallback, not the dedup logic). Fix is one line: replace the dead pointer reassignment with a memcpy back into the caller's buffer. Algorithm, tolerance, and comparison semantics are all preserved — the dead store just becomes the write-back that the original code obviously intended. <string.h> is already included; memcpy is used elsewhere in the file.
Collaborator
|
The underlying problem here is that The code double slist_unique[num_vertices+2];
slist_unique[0] = slist[0];
for (nv = 1; nv < num_intersections; nv++) {
if (fabs(slist[nv] - slist[nv-1]) > duplicate_tolerance*fabs(slist[nv])) {
slist_unique[num_unique_elements] = slist[nv];
num_unique_elements++;
}
}
memcpy(slist, slist_unique, num_unique_elements * sizeof(double));can be replaced with: int iv = num_intersections < 1 ? 0 : 1; /* min(1, num_intersections) */
for (nv = 1; nv < num_intersections; nv++) {
if (fabs(slist[nv] - slist[nv-1]) > duplicate_tolerance*fabs(slist[nv])) {
slist[iv++] = slist[nv];
}
}
num_intersections = iv; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #82