Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[hannk] Pacify clang-tidy #6412

Merged
merged 4 commits into from
Nov 12, 2021
Merged

[hannk] Pacify clang-tidy #6412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
51491a5
[hannk] Pacify clang-tidy
steven-johnson Nov 12, 2021
9ed07a7
One more ASAN fix
steven-johnson Nov 12, 2021
a6ccea4
Revert "One more ASAN fix"
steven-johnson Nov 12, 2021
8a36374
Rework Op::mutate() to avoid UB
steven-johnson Nov 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions apps/hannk/interpreter/model.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,13 +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) {
return op->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;
Expand Down Expand Up @@ -348,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 {
Expand Down Expand Up @@ -401,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
Expand Down
16 changes: 9 additions & 7 deletions apps/hannk/interpreter/ops.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<OP> o(static_cast<OP *>(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<OP> o(static_cast<OP *>(op.release())); \
return m->visit(std::move(o)); \
}; \
}

ACCEPT_AND_MUTATE_IMPL(BinaryOp)
Expand Down
38 changes: 19 additions & 19 deletions apps/hannk/interpreter/ops.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down