From 51491a57eea766c12c8056e57ae30930b6baa6a0 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Fri, 12 Nov 2021 09:33:21 -0800 Subject: [PATCH 1/4] [hannk] Pacify clang-tidy --- apps/hannk/interpreter/model.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/apps/hannk/interpreter/model.h b/apps/hannk/interpreter/model.h index 56c6c550bcd0..a6896a5b3303 100644 --- a/apps/hannk/interpreter/model.h +++ b/apps/hannk/interpreter/model.h @@ -306,7 +306,12 @@ class Op { // by making the virtual dispatches private, and add inline wrappers that // enforce a sane calling convention. static inline OpPtr mutate(OpPtr op, OpMutator *m) { - return op->mutate_impl(m, std::move(op)); + // clang-tidy will complain if we just do `op->mutate_impl(m, move(op))` + // because the order of evaluation between the move and the invocation is + // undefined; while that's true, we know that op will remain valid throughout + // this sequence, so we decompose into a couple of steps here just to pacify clang-tidy. + Op *op_ptr = op.get(); + return op_ptr->mutate_impl(m, std::move(op)); } virtual void dump(std::ostream &os, int indent = 0) const; From 9ed07a70b4a656790236a5ff6966155df823a319 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Fri, 12 Nov 2021 10:11:53 -0800 Subject: [PATCH 2/4] One more ASAN fix We must use use_global_gc = false to work properly with the JIT --- src/CodeGen_LLVM.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/CodeGen_LLVM.cpp b/src/CodeGen_LLVM.cpp index e4411faaf08a..17e836799043 100644 --- a/src/CodeGen_LLVM.cpp +++ b/src/CodeGen_LLVM.cpp @@ -1150,7 +1150,7 @@ void CodeGen_LLVM::optimize_module() { #if LLVM_VERSION >= 140 AddressSanitizerOptions asan_options; // default values are good... asan_options.UseAfterScope = true; // ...except this one - constexpr bool use_global_gc = true; + constexpr bool use_global_gc = false; constexpr bool use_odr_indicator = true; constexpr auto destructor_kind = AsanDtorKind::Global; mpm.addPass(ModuleAddressSanitizerPass( From a6ccea44500d4857fbcbf9ae6bf7652902c5918e Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Fri, 12 Nov 2021 10:12:29 -0800 Subject: [PATCH 3/4] Revert "One more ASAN fix" This reverts commit 9ed07a70b4a656790236a5ff6966155df823a319. --- src/CodeGen_LLVM.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/CodeGen_LLVM.cpp b/src/CodeGen_LLVM.cpp index 17e836799043..e4411faaf08a 100644 --- a/src/CodeGen_LLVM.cpp +++ b/src/CodeGen_LLVM.cpp @@ -1150,7 +1150,7 @@ void CodeGen_LLVM::optimize_module() { #if LLVM_VERSION >= 140 AddressSanitizerOptions asan_options; // default values are good... asan_options.UseAfterScope = true; // ...except this one - constexpr bool use_global_gc = false; + constexpr bool use_global_gc = true; constexpr bool use_odr_indicator = true; constexpr auto destructor_kind = AsanDtorKind::Global; mpm.addPass(ModuleAddressSanitizerPass( From 8a363749a461ecd8eb0911e0b9a005fb771f769b Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Fri, 12 Nov 2021 10:42:05 -0800 Subject: [PATCH 4/4] Rework Op::mutate() to avoid UB --- apps/hannk/interpreter/model.h | 21 +++++++------------ apps/hannk/interpreter/ops.cpp | 16 +++++++------- apps/hannk/interpreter/ops.h | 38 +++++++++++++++++----------------- 3 files changed, 36 insertions(+), 39 deletions(-) diff --git a/apps/hannk/interpreter/model.h b/apps/hannk/interpreter/model.h index a6896a5b3303..2f78e40ed99a 100644 --- a/apps/hannk/interpreter/model.h +++ b/apps/hannk/interpreter/model.h @@ -300,18 +300,13 @@ class Op { // // Note that this is a static method because we need to pass the op in question // via unique_ptr (since the callee needs to take ownership); we also - // need to use the 'naked' pointer to dispatch a virtual method, which - // is weird-looking and could lead to wrong behavior if you passed different - // objects. To avoid this, we restrict access to the dispatching code - // by making the virtual dispatches private, and add inline wrappers that - // enforce a sane calling convention. + // need to use the 'naked' pointer to dispatch a virtual method which returns a function pointer, + // to avoid any possible UB from order-of-operations (e.g., op->mutate_impl(std::move(op)), which + // has undefined order wrt the move vs the virtual lookup). + using OpMutatorFn = OpPtr (*)(OpPtr op, OpMutator *m); static inline OpPtr mutate(OpPtr op, OpMutator *m) { - // clang-tidy will complain if we just do `op->mutate_impl(m, move(op))` - // because the order of evaluation between the move and the invocation is - // undefined; while that's true, we know that op will remain valid throughout - // this sequence, so we decompose into a couple of steps here just to pacify clang-tidy. - Op *op_ptr = op.get(); - return op_ptr->mutate_impl(m, std::move(op)); + OpMutatorFn mutate_fn = op->mutate_impl(); + return mutate_fn(std::move(op), m); } virtual void dump(std::ostream &os, int indent = 0) const; @@ -353,7 +348,7 @@ class Op { private: virtual void accept_impl(OpVisitor *v) const = 0; - virtual OpPtr mutate_impl(OpMutator *m, OpPtr op) = 0; + virtual OpMutatorFn mutate_impl() const = 0; }; class OpGroup : public Op { @@ -406,7 +401,7 @@ class OpGroup : public Op { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; } // namespace hannk diff --git a/apps/hannk/interpreter/ops.cpp b/apps/hannk/interpreter/ops.cpp index 0915dcb3d736..3f6d03c5dc65 100644 --- a/apps/hannk/interpreter/ops.cpp +++ b/apps/hannk/interpreter/ops.cpp @@ -1876,13 +1876,15 @@ void UpsampleChannelsOp::execute() { << "Unsupported UpsampleChannels op for types " << in->type() << ", " << out->type(); } -#define ACCEPT_AND_MUTATE_IMPL(OP) \ - void OP::accept_impl(OpVisitor *v) const { \ - v->visit(this); \ - } \ - OpPtr OP::mutate_impl(OpMutator *m, OpPtr op) { \ - std::unique_ptr o(static_cast(op.release())); \ - return m->visit(std::move(o)); \ +#define ACCEPT_AND_MUTATE_IMPL(OP) \ + void OP::accept_impl(OpVisitor *v) const { \ + v->visit(this); \ + } \ + Op::OpMutatorFn OP::mutate_impl() const { \ + return [](OpPtr op, OpMutator *m) -> OpPtr { \ + std::unique_ptr o(static_cast(op.release())); \ + return m->visit(std::move(o)); \ + }; \ } ACCEPT_AND_MUTATE_IMPL(BinaryOp) diff --git a/apps/hannk/interpreter/ops.h b/apps/hannk/interpreter/ops.h index 594816481306..e20d2c5ab71c 100644 --- a/apps/hannk/interpreter/ops.h +++ b/apps/hannk/interpreter/ops.h @@ -63,7 +63,7 @@ class BinaryOp : public ElementwiseOp { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class ConcatenationOp : public Op { @@ -92,7 +92,7 @@ class ConcatenationOp : public Op { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class ConvOp : public Op { @@ -148,7 +148,7 @@ class ConvOp : public Op { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class DepthwiseConv2DOp : public Op { @@ -207,7 +207,7 @@ class DepthwiseConv2DOp : public Op { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class ElementwiseProgramOp : public ElementwiseOp { @@ -230,7 +230,7 @@ class ElementwiseProgramOp : public ElementwiseOp { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class GatherOp : public Op { @@ -252,7 +252,7 @@ class GatherOp : public Op { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class L2NormalizationOp : public Op { @@ -273,7 +273,7 @@ class L2NormalizationOp : public Op { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class PadOp : public Op { @@ -299,7 +299,7 @@ class PadOp : public Op { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class Pool2DOp : public Op { @@ -347,7 +347,7 @@ class Pool2DOp : public Op { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class ReductionOp : public Op { @@ -378,7 +378,7 @@ class ReductionOp : public Op { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class ReshapeOp : public Op { @@ -402,7 +402,7 @@ class ReshapeOp : public Op { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class ShapeOp : public Op { @@ -421,7 +421,7 @@ class ShapeOp : public Op { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class SoftmaxOp : public Op { @@ -443,7 +443,7 @@ class SoftmaxOp : public Op { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class SpaceDepthOp : public Op { @@ -464,7 +464,7 @@ class SpaceDepthOp : public Op { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class SplitOp : public Op { @@ -493,7 +493,7 @@ class SplitOp : public Op { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class TileConvFilterOp : public Op { @@ -512,7 +512,7 @@ class TileConvFilterOp : public Op { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class TransposeOp : public Op { @@ -531,7 +531,7 @@ class TransposeOp : public Op { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class UnaryOp : public ElementwiseOp { @@ -564,7 +564,7 @@ class UnaryOp : public ElementwiseOp { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class UpsampleChannelsOp : public Op { @@ -585,7 +585,7 @@ class UpsampleChannelsOp : public Op { private: void accept_impl(OpVisitor *v) const override; - OpPtr mutate_impl(OpMutator *m, OpPtr op) override; + OpMutatorFn mutate_impl() const override; }; class OpVisitor {