Skip to content

Commit 6cfa972

Browse files
AlexMWellsAlexMWells
authored
fix: false positives when utilizing OSL's "debug_uninit" feature (#1947)
It was checking a "while" loops condition variable before the condition had executed causing false positive. It was checking all elements of arrays causing false positives when only a subset of the array is actually being read from in "pointcloud_get", "spline", "splineinverse" calls. Implementation detail: in batched mode, to handle a varying count or knot count parameters a new version of osl_uninit_check_values_offset was added. Fix message formatting for batched debug_uninit messages which was printing out pointer to the string instead of the strings themselves. debug-uninit in testsuite was update to check for false positives for while(condition), spline, and splineinverse as well as correctly identifying uninitialized use of partially initialized arrays with spline and splineinverse. Adding test for pointcloud_get was omitted due to local build configuration. --------- Signed-off-by: Alex M. Wells <[email protected]>
1 parent 43f649b commit 6cfa972

File tree

7 files changed

+219
-40
lines changed

7 files changed

+219
-40
lines changed

‎src/liboslexec/batched_llvm_instance.cpp‎

Lines changed: 86 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,9 @@ static ustring op_aref("aref");
138138
static ustring op_compref("compref");
139139
static ustring op_mxcompref("mxcompref");
140140
static ustring op_useparam("useparam");
141+
static ustring op_pointcloud_get("pointcloud_get");
142+
static ustring op_spline("spline");
143+
static ustring op_splineinverse("splineinverse");
141144
static ustring unknown_shader_group_name("<Unknown Shader Group Name>");
142145

143146

@@ -1620,7 +1623,8 @@ BatchedBackendLLVM::llvm_generate_debug_uninit(const Opcode& op)
16201623
}
16211624

16221625
llvm::Value* ncheck = ll.constant(int(t.numelements() * t.aggregate));
1623-
llvm::Value* offset = ll.constant(0);
1626+
llvm::Value* varying_nchecks = nullptr;
1627+
llvm::Value* offset = ll.constant(0);
16241628
BatchedBackendLLVM::TempScope temp_scope(*this);
16251629
llvm::Value* loc_varying_offsets = nullptr;
16261630

@@ -1701,6 +1705,31 @@ BatchedBackendLLVM::llvm_generate_debug_uninit(const Opcode& op)
17011705
ll.op_store(comp, loc_varying_offsets);
17021706
}
17031707
ncheck = ll.constant(1);
1708+
} else if (op.opname() == op_pointcloud_get && i == 2) {
1709+
// int pointcloud_get (string ptcname, int indices[], int count, string attr, type data[])
1710+
// will only read indices[0..count-1], so limit the check to count
1711+
OSL_ASSERT(3 < op.nargs());
1712+
Symbol& count_sym = *opargsym(op, 3);
1713+
if (count_sym.is_uniform()) {
1714+
ncheck = llvm_load_value(count_sym);
1715+
} else {
1716+
// Don't think we can have a uniform indices array that
1717+
// has a varying index count
1718+
if (!sym.is_uniform()) {
1719+
varying_nchecks = ll.void_ptr(llvm_get_pointer(count_sym));
1720+
}
1721+
}
1722+
} else if (((op.opname() == op_spline)
1723+
|| (op.opname() == op_splineinverse))
1724+
&& i == 4) {
1725+
// If an explicit knot count was provided to spline|splineinverse we should
1726+
// limit our check of knot values to that count
1727+
bool has_knot_count = (op.nargs() == 5);
1728+
if (has_knot_count) {
1729+
Symbol& knot_count_sym = *opargsym(op, 3);
1730+
OSL_ASSERT(knot_count_sym.is_uniform());
1731+
ncheck = llvm_load_value(knot_count_sym);
1732+
}
17041733
}
17051734

17061735
if (loc_varying_offsets != nullptr) {
@@ -1724,15 +1753,15 @@ BatchedBackendLLVM::llvm_generate_debug_uninit(const Opcode& op)
17241753
if (sym.is_uniform()) {
17251754
ll.call_function(build_name(
17261755
FuncSpec("uninit_check_values_offset")
1727-
.arg_uniform(TypeDesc::PTR)
1728-
.arg_varying(TypeInt)
1756+
.arg_uniform(TypeDesc::PTR) // vals
1757+
.arg_varying(TypeInt) // firstcheck
17291758
.mask()),
17301759
args);
17311760
} else {
17321761
ll.call_function(build_name(
17331762
FuncSpec("uninit_check_values_offset")
1734-
.arg_varying(TypeDesc::PTR)
1735-
.arg_varying(TypeInt)
1763+
.arg_varying(TypeDesc::PTR) // vals
1764+
.arg_varying(TypeInt) // firstcheck
17361765
.mask()),
17371766
args);
17381767
}
@@ -1756,33 +1785,60 @@ BatchedBackendLLVM::llvm_generate_debug_uninit(const Opcode& op)
17561785
ncheck };
17571786
ll.call_function(build_name(
17581787
FuncSpec("uninit_check_values_offset")
1759-
.arg_uniform(TypeDesc::PTR)
1760-
.arg_uniform(TypeInt)),
1788+
.arg_uniform(TypeDesc::PTR) // vals
1789+
.arg_uniform(TypeInt)), // firstcheck
17611790
args);
17621791
} else {
1763-
llvm::Value* args[]
1764-
= { ll.mask_as_int(ll.current_mask()),
1765-
ll.constant(t),
1766-
llvm_void_ptr(sym),
1767-
sg_void_ptr(),
1768-
ll.constant(op.sourcefile()),
1769-
ll.constant(op.sourceline()),
1770-
ll.constant(group().name()),
1771-
ll.constant(layer()),
1772-
ll.constant(inst()->layername()),
1773-
ll.constant(inst()->shadername().c_str()),
1774-
ll.constant(int(&op - &inst()->ops()[0])),
1775-
ll.constant(op.opname()),
1776-
ll.constant(i),
1777-
ll.constant(sym.unmangled()),
1778-
offset,
1779-
ncheck };
1780-
ll.call_function(build_name(
1781-
FuncSpec("uninit_check_values_offset")
1782-
.arg_varying(TypeDesc::PTR)
1783-
.arg_uniform(TypeInt)
1784-
.mask()),
1785-
args);
1792+
if (varying_nchecks != nullptr) {
1793+
llvm::Value* args[]
1794+
= { ll.mask_as_int(ll.current_mask()),
1795+
ll.constant(t),
1796+
llvm_void_ptr(sym),
1797+
sg_void_ptr(),
1798+
ll.constant(op.sourcefile()),
1799+
ll.constant(op.sourceline()),
1800+
ll.constant(group().name()),
1801+
ll.constant(layer()),
1802+
ll.constant(inst()->layername()),
1803+
ll.constant(inst()->shadername().c_str()),
1804+
ll.constant(int(&op - &inst()->ops()[0])),
1805+
ll.constant(op.opname()),
1806+
ll.constant(i),
1807+
ll.constant(sym.unmangled()),
1808+
offset,
1809+
varying_nchecks };
1810+
ll.call_function(
1811+
build_name(FuncSpec("uninit_check_values_offset")
1812+
.arg_varying(TypeDesc::PTR) // vals
1813+
.arg_uniform(TypeInt) // firstcheck
1814+
.arg_varying(TypeInt) // nchecks
1815+
.mask()),
1816+
args);
1817+
} else {
1818+
llvm::Value* args[]
1819+
= { ll.mask_as_int(ll.current_mask()),
1820+
ll.constant(t),
1821+
llvm_void_ptr(sym),
1822+
sg_void_ptr(),
1823+
ll.constant(op.sourcefile()),
1824+
ll.constant(op.sourceline()),
1825+
ll.constant(group().name()),
1826+
ll.constant(layer()),
1827+
ll.constant(inst()->layername()),
1828+
ll.constant(inst()->shadername().c_str()),
1829+
ll.constant(int(&op - &inst()->ops()[0])),
1830+
ll.constant(op.opname()),
1831+
ll.constant(i),
1832+
ll.constant(sym.unmangled()),
1833+
offset,
1834+
ncheck };
1835+
ll.call_function(
1836+
build_name(FuncSpec("uninit_check_values_offset")
1837+
.arg_varying(TypeDesc::PTR) // vals
1838+
.arg_uniform(TypeInt) // firstcheck
1839+
.mask()),
1840+
args);
1841+
}
17861842
}
17871843
}
17881844
}

‎src/liboslexec/builtindecl_wide_xmacro.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -373,6 +373,8 @@ DECL(__OSL_MASKED_OP(range_check), "xXiisXsisiss")
373373
DECL(__OSL_OP2(uninit_check_values_offset, X, i), "xLXXsisissisisii")
374374
DECL(__OSL_MASKED_OP2(uninit_check_values_offset, X, Wi), "xiLXXsisissisisXi")
375375
DECL(__OSL_MASKED_OP2(uninit_check_values_offset, WX, i), "xiLXXsisissisisii")
376+
DECL(__OSL_MASKED_OP3(uninit_check_values_offset, WX, i, Wi),
377+
"xiLXXsisissisisiX")
376378
DECL(__OSL_MASKED_OP2(uninit_check_values_offset, WX, Wi), "xiLXXsisissisisXi")
377379

378380
DECL(__OSL_OP1(get_attribute, s), "iXissiiXXi")

‎src/liboslexec/llvm_instance.cpp‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,9 @@ static ustring op_aref("aref");
143143
static ustring op_compref("compref");
144144
static ustring op_mxcompref("mxcompref");
145145
static ustring op_useparam("useparam");
146+
static ustring op_pointcloud_get("pointcloud_get");
147+
static ustring op_spline("spline");
148+
static ustring op_splineinverse("splineinverse");
146149
static ustring unknown_shader_group_name("<Unknown Shader Group Name>");
147150

148151

@@ -1135,8 +1138,10 @@ BackendLLVM::llvm_generate_debug_uninit(const Opcode& op)
11351138
// don't generate uninit test code for it.
11361139
continue;
11371140
}
1138-
if (op.opname() == Strings::op_dowhile && i == 0) {
1139-
// The first argument of 'dowhile' is the condition temp, but
1141+
if (((op.opname() == Strings::op_dowhile)
1142+
|| (op.opname() == Strings::op_while))
1143+
&& i == 0) {
1144+
// The first argument of 'dowhile' or "while" is the condition temp, but
11401145
// most likely its initializer has not run yet. Unless there is
11411146
// no "condition" code block, in that case we should still test
11421147
// it for uninit.
@@ -1185,6 +1190,20 @@ BackendLLVM::llvm_generate_debug_uninit(const Opcode& op)
11851190
comp = ll.op_add(comp, col_ind);
11861191
offset = comp;
11871192
ncheck = ll.constant(1);
1193+
} else if (op.opname() == op_pointcloud_get && i == 2) {
1194+
// int pointcloud_get (string ptcname, int indices[], int count, string attr, type data[])
1195+
// will only read indices[0..count-1], so limit the check to count
1196+
OSL_ASSERT(3 < op.nargs());
1197+
ncheck = llvm_load_value(*opargsym(op, 3));
1198+
} else if (((op.opname() == op_spline)
1199+
|| (op.opname() == op_splineinverse))
1200+
&& i == 4) {
1201+
// If an explicit knot count was provided to spline|splineinverse we should
1202+
// limit our check of knot values to that count
1203+
bool has_knot_count = (op.nargs() == 5);
1204+
if (has_knot_count) {
1205+
ncheck = llvm_load_value(*opargsym(op, 3));
1206+
}
11881207
}
11891208

11901209
llvm::Value* args[] = { ll.constant(t),

‎src/liboslexec/wide/wide_shadingsys.cpp‎

Lines changed: 82 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -169,10 +169,10 @@ __OSL_OP2(uninit_check_values_offset, X,
169169
ustring layername = USTR(layername_);
170170
ctx->errorfmt(
171171
"Detected possible use of uninitialized value in {} {} at {}:{} (group {}, layer {} {}, shader {}, op {} '{}', arg {})",
172-
typedesc, symbolname, sourcefile, sourceline,
172+
typedesc, USTR(symbolname), USTR(sourcefile), sourceline,
173173
groupname.empty() ? "<unnamed group>" : groupname.c_str(), layer,
174174
layername.empty() ? "<unnamed layer>" : layername.c_str(),
175-
shadername, opnum, opname, argnum);
175+
USTR(shadername), opnum, USTR(opname), argnum);
176176
}
177177
}
178178

@@ -233,10 +233,82 @@ __OSL_MASKED_OP2(uninit_check_values_offset, WX,
233233
ustring layername = USTR(layername_);
234234
ctx->errorfmt(
235235
"Detected possible use of uninitialized value in {} {} at {}:{} (group {}, layer {} {}, shader {}, op {} '{}', arg {}) for lanes({:x}) of batch",
236-
typedesc, symbolname, sourcefile, sourceline,
236+
typedesc, USTR(symbolname), USTR(sourcefile), sourceline,
237237
groupname.empty() ? "<unnamed group>" : groupname.c_str(), layer,
238238
layername.empty() ? "<unnamed layer>" : layername.c_str(),
239-
shadername, opnum, opname, argnum, lanes_uninit.value());
239+
USTR(shadername), opnum, USTR(opname), argnum,
240+
lanes_uninit.value());
241+
}
242+
}
243+
244+
245+
246+
// Many parameters, but the 3 parameters used in the function name
247+
// correspond to: "vals", "firstcheck", and "nchecks"
248+
OSL_BATCHOP void
249+
__OSL_MASKED_OP3(uninit_check_values_offset, WX, i,
250+
Wi)(int mask_value, long long typedesc_, void* vals_,
251+
void* bsg_, ustring_pod sourcefile, int sourceline,
252+
ustring_pod groupname_, int layer, ustring_pod layername_,
253+
ustring_pod shadername, int opnum, ustring_pod opname,
254+
int argnum, ustring_pod symbolname, int firstcheck,
255+
int* nchecks_)
256+
{
257+
TypeDesc typedesc = TYPEDESC(typedesc_);
258+
auto* bsg = reinterpret_cast<BatchedShaderGlobals*>(bsg_);
259+
ShadingContext* ctx = bsg->uniform.context;
260+
const Mask mask(mask_value);
261+
262+
Mask lanes_uninit(false);
263+
264+
if (typedesc.basetype == TypeDesc::FLOAT) {
265+
float* vals = (float*)vals_;
266+
mask.foreach ([=, &lanes_uninit](ActiveLane lane) -> void {
267+
int nchecks = nchecks_[lane];
268+
for (int c = firstcheck, e = firstcheck + nchecks; c < e; ++c) {
269+
if (!std::isfinite(vals[c * __OSL_WIDTH + lane])) {
270+
lanes_uninit.set_on(lane);
271+
vals[c * __OSL_WIDTH + lane] = 0;
272+
}
273+
}
274+
});
275+
}
276+
if (typedesc.basetype == TypeDesc::INT) {
277+
int* vals = (int*)vals_;
278+
mask.foreach ([=, &lanes_uninit](ActiveLane lane) -> void {
279+
int nchecks = nchecks_[lane];
280+
for (int c = firstcheck, e = firstcheck + nchecks; c < e; ++c) {
281+
if (vals[c * __OSL_WIDTH + lane]
282+
== std::numeric_limits<int>::min()) {
283+
lanes_uninit.set_on(lane);
284+
vals[c * __OSL_WIDTH + lane] = 0;
285+
}
286+
}
287+
});
288+
}
289+
if (typedesc.basetype == TypeDesc::STRING) {
290+
ustring* vals = (ustring*)vals_;
291+
mask.foreach ([=, &lanes_uninit](ActiveLane lane) -> void {
292+
int nchecks = nchecks_[lane];
293+
for (int c = firstcheck, e = firstcheck + nchecks; c < e; ++c) {
294+
if (vals[c * __OSL_WIDTH + lane]
295+
== Strings::uninitialized_string) {
296+
lanes_uninit.set_on(lane);
297+
vals[c * __OSL_WIDTH + lane] = ustring();
298+
}
299+
}
300+
});
301+
}
302+
if (lanes_uninit.any_on()) {
303+
ustring groupname = USTR(groupname_);
304+
ustring layername = USTR(layername_);
305+
ctx->errorfmt(
306+
"Detected possible use of uninitialized value in {} {} at {}:{} (group {}, layer {} {}, shader {}, op {} '{}', arg {}) for lanes({:x}) of batch",
307+
typedesc, USTR(symbolname), USTR(sourcefile), sourceline,
308+
groupname.empty() ? "<unnamed group>" : groupname.c_str(), layer,
309+
layername.empty() ? "<unnamed layer>" : layername.c_str(),
310+
USTR(shadername), opnum, USTR(opname), argnum,
311+
lanes_uninit.value());
240312
}
241313
}
242314

@@ -299,10 +371,11 @@ __OSL_MASKED_OP2(uninit_check_values_offset, X,
299371
ustring layername = USTR(layername_);
300372
ctx->errorfmt(
301373
"Detected possible use of uninitialized value in {} {} at {}:{} (group {}, layer {} {}, shader {}, op {} '{}', arg {}) for lanes({:x}) of batch",
302-
typedesc, symbolname, sourcefile, sourceline,
374+
typedesc, USTR(symbolname), USTR(sourcefile), sourceline,
303375
groupname.empty() ? "<unnamed group>" : groupname.c_str(), layer,
304376
layername.empty() ? "<unnamed layer>" : layername.c_str(),
305-
shadername, opnum, opname, argnum, lanes_uninit.value());
377+
USTR(shadername), opnum, USTR(opname), argnum,
378+
lanes_uninit.value());
306379
}
307380
}
308381

@@ -367,10 +440,11 @@ __OSL_MASKED_OP2(uninit_check_values_offset, WX,
367440
ustring layername = USTR(layername_);
368441
ctx->errorfmt(
369442
"Detected possible use of uninitialized value in {} {} at {}:{} (group {}, layer {} {}, shader {}, op {} '{}', arg {}) for lanes({:x}) of batch",
370-
typedesc, symbolname, sourcefile, sourceline,
443+
typedesc, USTR(symbolname), USTR(sourcefile), sourceline,
371444
groupname.empty() ? "<unnamed group>" : groupname.c_str(), layer,
372445
layername.empty() ? "<unnamed layer>" : layername.c_str(),
373-
shadername, opnum, opname, argnum, lanes_uninit.value());
446+
USTR(shadername), opnum, USTR(opname), argnum,
447+
lanes_uninit.value());
374448
}
375449
}
376450

‎testsuite/debug-uninit/ref/out-opt2.txt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,3 +8,5 @@ ERROR: [RendererServices::texture] ImageInput::create() called with no filename
88
ERROR: Detected possible use of uninitialized value in float[3] A at test.osl:22 (group unnamed_group_1, layer 0 test_0, shader test, op 7 'aref', arg 1)
99
ERROR: Detected possible use of uninitialized value in color C at test.osl:28 (group unnamed_group_1, layer 0 test_0, shader test, op 12 'compref', arg 1)
1010
ERROR: Detected possible use of uninitialized value in matrix M at test.osl:34 (group unnamed_group_1, layer 0 test_0, shader test, op 17 'mxcompref', arg 1)
11+
ERROR: Detected possible use of uninitialized value in float[20] knots3 at test.osl:54 (group unnamed_group_1, layer 0 test_0, shader test, op 44 'spline', arg 4)
12+
ERROR: Detected possible use of uninitialized value in float[20] knots4 at test.osl:55 (group unnamed_group_1, layer 0 test_0, shader test, op 46 'splineinverse', arg 4)

‎testsuite/debug-uninit/ref/out.txt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,3 +8,5 @@ ERROR: [RendererServices::texture] ImageInput::create() called with no filename
88
ERROR: Detected possible use of uninitialized value in float[3] A at test.osl:22 (group unnamed_group_1, layer 0 test_0, shader test, op 12 'aref', arg 1)
99
ERROR: Detected possible use of uninitialized value in color C at test.osl:28 (group unnamed_group_1, layer 0 test_0, shader test, op 17 'compref', arg 1)
1010
ERROR: Detected possible use of uninitialized value in matrix M at test.osl:34 (group unnamed_group_1, layer 0 test_0, shader test, op 22 'mxcompref', arg 1)
11+
ERROR: Detected possible use of uninitialized value in float[20] knots3 at test.osl:54 (group unnamed_group_1, layer 0 test_0, shader test, op 49 'spline', arg 4)
12+
ERROR: Detected possible use of uninitialized value in float[20] knots4 at test.osl:55 (group unnamed_group_1, layer 0 test_0, shader test, op 51 'splineinverse', arg 4)

‎testsuite/debug-uninit/test.osl‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,5 +33,29 @@ test (output color Cout = 0)
3333
x += M[1][2]; // NOT an error
3434
x += M[0][0]; // An error
3535
}
36+
float knots[20];
37+
float knots2[20];
38+
float knots3[20];
39+
float knots4[20];
40+
int index=0;
41+
// Ensure no false positive for temporary condition variable
42+
while (index < 10) {
43+
knots[index] = x*index;
44+
knots2[index] = (1.0-x)*index;
45+
knots3[index] = 2*x*index;
46+
knots4[index] = 2*(1.0-x)*index;
47+
++index;
48+
}
49+
// Ensure no false positive for accessing initialized portion of a partially initialized array
50+
x += spline ("linear", x, 10, knots);
51+
x += splineinverse ("linear", x, 10, knots2);
52+
53+
// Accessing uninitialized portion of a partially initialized array
54+
x += spline ("linear", x, 15, knots3);
55+
x += splineinverse ("linear", x, 15, knots4);
56+
57+
// pointcloud_get could also have false postive and read partially initialized array
58+
// leaving pointcloud_get out, as not all builds are configured for pointcloud support
59+
3660
Cout[0] += x; // force x results to not be optimized away
3761
}

0 commit comments

Comments
 (0)