Prevent use of uninitialized scalar Parameters in JIT code (#7847, partial) #7853

steven-johnson · 2023-09-18T22:25:43Z

No description provided.

…rtial)

test/correctness/func_clone.cpp

TH3CHARLie · 2023-09-19T18:18:22Z

src/Parameter.cpp

@@ -20,6 +20,7 @@ struct ParameterContents {
    std::vector<BufferConstraint> buffer_constraints;
    Expr scalar_default, scalar_min, scalar_max, scalar_estimate;
    const bool is_buffer;
+    bool data_ever_set = false;


this is the place we realize whenever we add sth to the frontend IR, serialization needs to be updated at the same time

I don't understand

data_ever_set looks like a field that needs to be consistent across serialization (i.e. A parameter that has a data set should also be known as data-ever-set after serialization roundtrip). In this case, we need to add this field to the serialization implementation to properly serialize it.

Hmm... didn't think about that.

I'm actually kinda unsure as to whether we should be saving the scalar value of the Param (if any) to the Serialization or not -- I know we do now (and it's required for the serialization-round-trip-via-JIT hack), but conceptually, it seems like the wrong thing to do.

A Parameter (either scalar or buffer) is conceptually just a placeholder with required-type information. Is it really desirable (in the general sense) to save whatever happens to be stuff in there?

it does seem weird to me at first as well, had to admit it.

But now I treat this new field in the PR same/similar to the purpose of defined, then it makes more sense

steven-johnson · 2023-09-25T20:04:16Z

PTAL -- this change ballooned quite a bit but I think this is all better now:

Use std::optional<halide_scalar_value_t> instead of uint64-and-bool for the scalar data field.
Mark the Flatbuffer field as = null so that the API for it uses std::optional as well. (Note that there is no binary format change here since all Flatbuffer fields are optional by default.)
Make the recently-added-for-deserialization ctors for Parameter private, with friend access for Deserializer; also remove the redundant is_buffer arguments since each of them has only one legal value anyway (and delete the Python wrappers for them entirely)
Tighten the implementation of Parameter:scalar_expr() and Parameter::scalar()

PTAL

abadams · 2023-09-25T20:32:31Z

src/Pipeline.cpp

@@ -808,6 +825,11 @@ void Pipeline::prepare_jit_call_arguments(RealizationArg &outputs, const Target
                                          bool is_bounds_inference, JITCallArgs &args_result) {
    user_assert(defined()) << "Can't realize an undefined Pipeline\n";

+    FindVariablesWithUnsetParams find_unset;


I think this code is a hot path, so we can't do IR traversal here.

Blarg. That's no good. I'm open for better suggestions.

abadams · 2023-09-25T20:34:32Z

src/Pipeline.cpp

+
+    void visit(const Variable *op) override {
+        if (op->param.defined() && !op->param.is_buffer() && op->param.name() != "__user_context") {
+            user_assert(op->param.has_scalar_value())


I don't think this error message is right. Won't it trigger on all attempts to realize a pipeline with unset params, regardless of whether or not you're in a generator? I think the much more likely scenario is someone using the JIT forgetting to set the value of a Param. Also, you can absolutely compile_to_callable without setting the scalar params.

Won't it trigger on all attempts to realize a pipeline with unset params, regardless of whether or not you're in a generator?

Yes. That's a feature, not a bug.

I don't think this error message is right.

Yeah, this is a bit confusing -- IIRC the reason for wording it this way is that the "other" cases (realize, callable) end up being caught via read_only_scalar_address() noting the invalid value and failing (apparently they don't add the interposed Variable, I don't recall why, it's been a few days).

steven-johnson · 2023-09-26T18:05:32Z

OK, so upon further review:

FindVariablesWithUnsetParams was added to try to address the case of using compile_to_callable() inside a Generator, but I think it's basically impossible to (mis)use compile_to_callable() in this way inside a Generator; you'd have to construct your own Argument directly to use in compile_to_callable() (since Generator Input fields don't expose any conversion to Argument or Param or Parameter)... and even then, there isn't a way to invoke the Callable with the Input, because the Callable requires a float argument (an Expr won't do). So I don't think we even need that traverser (or the uninitialized_param_3 test) at all.

…, partial) (halide#7853) * Prevent use of uninitialized scalar Parameters in JIT code (halide#7847, partial) * Fix broken tests * Update Parameter.h * Update func_clone.cpp * Fix Generators too * Fixes * Update InferArguments.cpp * Fixes * pacify clang-tidy * fixes

Prevent use of uninitialized scalar Parameters in JIT code (#7847, pa…

9495153

…rtial)

steven-johnson requested review from abadams and zvookin September 18, 2023 22:25

steven-johnson mentioned this pull request Sep 18, 2023

Calling realize() within an AOT Generator is problematic #7847

Open

steven-johnson added 2 commits September 18, 2023 16:46

Fix broken tests

c8bb1af

Update Parameter.h

a60eb99

abadams reviewed Sep 19, 2023

View reviewed changes

test/correctness/func_clone.cpp Show resolved Hide resolved

Update func_clone.cpp

699ec87

TH3CHARLie reviewed Sep 19, 2023

View reviewed changes

steven-johnson added 3 commits September 19, 2023 11:34

Fix Generators too

296a58a

Merge branch 'main' into srj/uninit-scalar-param

113c4d3

Fixes

2fa1c73

Update InferArguments.cpp

3963c5e

abadams reviewed Sep 25, 2023

View reviewed changes

steven-johnson added 2 commits September 25, 2023 15:25

Fixes

57838ba

pacify clang-tidy

b914ccd

fixes

70135b0

abadams approved these changes Sep 26, 2023

View reviewed changes

steven-johnson merged commit 9f96b25 into main Sep 27, 2023
3 checks passed

steven-johnson deleted the srj/uninit-scalar-param branch September 27, 2023 01:55

BrewTestBot mentioned this pull request Feb 2, 2024

halide 17.0.0 Homebrew/homebrew-core#161602

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent use of uninitialized scalar Parameters in JIT code (#7847, partial) #7853

Prevent use of uninitialized scalar Parameters in JIT code (#7847, partial) #7853

steven-johnson commented Sep 18, 2023

TH3CHARLie Sep 19, 2023

steven-johnson Sep 19, 2023

TH3CHARLie Sep 19, 2023

steven-johnson Sep 19, 2023

TH3CHARLie Sep 19, 2023

steven-johnson commented Sep 25, 2023

abadams Sep 25, 2023

steven-johnson Sep 25, 2023

abadams Sep 25, 2023

steven-johnson Sep 25, 2023

steven-johnson commented Sep 26, 2023

Prevent use of uninitialized scalar Parameters in JIT code (#7847, partial) #7853

Prevent use of uninitialized scalar Parameters in JIT code (#7847, partial) #7853

Conversation

steven-johnson commented Sep 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-johnson commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-johnson commented Sep 26, 2023