Commit 4f4da1f8 authored by titzer's avatar titzer Committed by Commit bot

[wasm] Refactoring: move bytecode operands into wasm-decoder-impl.h

R=bradnelson@chromium.org,clemensh@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2682943007
Cr-Commit-Position: refs/heads/master@{#43077}
parent 53a74a64
......@@ -1798,6 +1798,7 @@ v8_source_set("v8_base") {
"src/vm-state-inl.h",
"src/vm-state.h",
"src/wasm/decoder.h",
"src/wasm/function-body-decoder-impl.h",
"src/wasm/function-body-decoder.cc",
"src/wasm/function-body-decoder.h",
"src/wasm/leb-helper.h",
......
......@@ -1304,6 +1304,7 @@
'wasm/decoder.h',
'wasm/function-body-decoder.cc',
'wasm/function-body-decoder.h',
'wasm/function-body-decoder-impl.h',
'wasm/leb-helper.h',
'wasm/managed.h',
'wasm/module-decoder.cc',
......
// Copyright 2017 the V8 project authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef V8_WASM_FUNCTION_BODY_DECODER_IMPL_H_
#define V8_WASM_FUNCTION_BODY_DECODER_IMPL_H_
#include "src/wasm/decoder.h"
#include "src/wasm/wasm-opcodes.h"
namespace v8 {
namespace internal {
namespace wasm {
struct WasmGlobal;
// Helpers for decoding different kinds of operands which follow bytecodes.
struct LocalIndexOperand {
uint32_t index;
ValueType type;
unsigned length;
inline LocalIndexOperand(Decoder* decoder, const byte* pc) {
index = decoder->checked_read_u32v(pc, 1, &length, "local index");
type = kWasmStmt;
}
};
struct ImmI32Operand {
int32_t value;
unsigned length;
inline ImmI32Operand(Decoder* decoder, const byte* pc) {
value = decoder->checked_read_i32v(pc, 1, &length, "immi32");
}
};
struct ImmI64Operand {
int64_t value;
unsigned length;
inline ImmI64Operand(Decoder* decoder, const byte* pc) {
value = decoder->checked_read_i64v(pc, 1, &length, "immi64");
}
};
struct ImmF32Operand {
float value;
unsigned length;
inline ImmF32Operand(Decoder* decoder, const byte* pc) {
// Avoid bit_cast because it might not preserve the signalling bit of a NaN.
uint32_t tmp = decoder->checked_read_u32(pc, 1, "immf32");
memcpy(&value, &tmp, sizeof(value));
length = 4;
}
};
struct ImmF64Operand {
double value;
unsigned length;
inline ImmF64Operand(Decoder* decoder, const byte* pc) {
// Avoid bit_cast because it might not preserve the signalling bit of a NaN.
uint64_t tmp = decoder->checked_read_u64(pc, 1, "immf64");
memcpy(&value, &tmp, sizeof(value));
length = 8;
}
};
struct GlobalIndexOperand {
uint32_t index;
ValueType type;
const WasmGlobal* global;
unsigned length;
inline GlobalIndexOperand(Decoder* decoder, const byte* pc) {
index = decoder->checked_read_u32v(pc, 1, &length, "global index");
global = nullptr;
type = kWasmStmt;
}
};
struct BlockTypeOperand {
uint32_t arity;
const byte* types; // pointer to encoded types for the block.
unsigned length;
inline BlockTypeOperand(Decoder* decoder, const byte* pc) {
uint8_t val = decoder->checked_read_u8(pc, 1, "block type");
ValueType type = kWasmStmt;
length = 1;
arity = 0;
types = nullptr;
if (decode_local_type(val, &type)) {
arity = type == kWasmStmt ? 0 : 1;
types = pc + 1;
} else {
// Handle multi-value blocks.
if (!FLAG_wasm_mv_prototype) {
decoder->error(pc, pc + 1, "invalid block arity > 1");
return;
}
if (val != kMultivalBlock) {
decoder->error(pc, pc + 1, "invalid block type");
return;
}
// Decode and check the types vector of the block.
unsigned len = 0;
uint32_t count = decoder->checked_read_u32v(pc, 2, &len, "block arity");
// {count} is encoded as {arity-2}, so that a {0} count here corresponds
// to a block with 2 values. This makes invalid/redundant encodings
// impossible.
arity = count + 2;
length = 1 + len + arity;
types = pc + 1 + 1 + len;
for (uint32_t i = 0; i < arity; i++) {
uint32_t offset = 1 + 1 + len + i;
val = decoder->checked_read_u8(pc, offset, "block type");
decode_local_type(val, &type);
if (type == kWasmStmt) {
decoder->error(pc, pc + offset, "invalid block type");
return;
}
}
}
}
// Decode a byte representing a local type. Return {false} if the encoded
// byte was invalid or {kMultivalBlock}.
bool decode_local_type(uint8_t val, ValueType* result) {
switch (static_cast<ValueTypeCode>(val)) {
case kLocalVoid:
*result = kWasmStmt;
return true;
case kLocalI32:
*result = kWasmI32;
return true;
case kLocalI64:
*result = kWasmI64;
return true;
case kLocalF32:
*result = kWasmF32;
return true;
case kLocalF64:
*result = kWasmF64;
return true;
case kLocalS128:
*result = kWasmS128;
return true;
default:
*result = kWasmStmt;
return false;
}
}
ValueType read_entry(unsigned index) {
DCHECK_LT(index, arity);
ValueType result;
CHECK(decode_local_type(types[index], &result));
return result;
}
};
struct Control;
struct BreakDepthOperand {
uint32_t depth;
Control* target;
unsigned length;
inline BreakDepthOperand(Decoder* decoder, const byte* pc) {
depth = decoder->checked_read_u32v(pc, 1, &length, "break depth");
target = nullptr;
}
};
struct CallIndirectOperand {
uint32_t table_index;
uint32_t index;
FunctionSig* sig;
unsigned length;
inline CallIndirectOperand(Decoder* decoder, const byte* pc) {
unsigned len = 0;
index = decoder->checked_read_u32v(pc, 1, &len, "signature index");
table_index = decoder->checked_read_u8(pc, 1 + len, "table index");
if (table_index != 0) {
decoder->error(pc, pc + 1 + len, "expected table index 0, found %u",
table_index);
}
length = 1 + len;
sig = nullptr;
}
};
struct CallFunctionOperand {
uint32_t index;
FunctionSig* sig;
unsigned length;
inline CallFunctionOperand(Decoder* decoder, const byte* pc) {
unsigned len1 = 0;
unsigned len2 = 0;
index = decoder->checked_read_u32v(pc, 1 + len1, &len2, "function index");
length = len1 + len2;
sig = nullptr;
}
};
struct MemoryIndexOperand {
uint32_t index;
unsigned length;
inline MemoryIndexOperand(Decoder* decoder, const byte* pc) {
index = decoder->checked_read_u8(pc, 1, "memory index");
if (index != 0) {
decoder->error(pc, pc + 1, "expected memory index 0, found %u", index);
}
length = 1;
}
};
struct BranchTableOperand {
uint32_t table_count;
const byte* start;
const byte* table;
inline BranchTableOperand(Decoder* decoder, const byte* pc) {
DCHECK_EQ(kExprBrTable, decoder->checked_read_u8(pc, 0, "opcode"));
start = pc + 1;
unsigned len1 = 0;
table_count = decoder->checked_read_u32v(pc, 1, &len1, "table count");
if (table_count > (UINT_MAX / sizeof(uint32_t)) - 1 ||
len1 > UINT_MAX - (table_count + 1) * sizeof(uint32_t)) {
decoder->error(pc, "branch table size overflow");
}
table = pc + 1 + len1;
}
};
// A helper to iterate over a branch table.
class BranchTableIterator {
public:
unsigned cur_index() { return index_; }
bool has_next() { return decoder_->ok() && index_ <= table_count_; }
uint32_t next() {
DCHECK(has_next());
index_++;
unsigned length = 0;
uint32_t result =
decoder_->checked_read_u32v(pc_, 0, &length, "branch table entry");
pc_ += length;
return result;
}
// length, including the length of the {BranchTableOperand}, but not the
// opcode.
unsigned length() {
while (has_next()) next();
return static_cast<unsigned>(pc_ - start_);
}
const byte* pc() { return pc_; }
BranchTableIterator(Decoder* decoder, BranchTableOperand& operand)
: decoder_(decoder),
start_(operand.start),
pc_(operand.table),
index_(0),
table_count_(operand.table_count) {}
private:
Decoder* decoder_;
const byte* start_;
const byte* pc_;
uint32_t index_; // the current index.
uint32_t table_count_; // the count of entries, not including default.
};
struct MemoryAccessOperand {
uint32_t alignment;
uint32_t offset;
unsigned length;
inline MemoryAccessOperand(Decoder* decoder, const byte* pc,
uint32_t max_alignment) {
unsigned alignment_length;
alignment =
decoder->checked_read_u32v(pc, 1, &alignment_length, "alignment");
if (max_alignment < alignment) {
decoder->error(pc, pc + 1,
"invalid alignment; expected maximum alignment is %u, "
"actual alignment is %u",
max_alignment, alignment);
}
unsigned offset_length;
offset = decoder->checked_read_u32v(pc, 1 + alignment_length,
&offset_length, "offset");
length = alignment_length + offset_length;
}
};
// Operand for SIMD lane operations.
struct SimdLaneOperand {
uint8_t lane;
unsigned length;
inline SimdLaneOperand(Decoder* decoder, const byte* pc) {
lane = decoder->checked_read_u8(pc, 2, "lane");
length = 1;
}
};
// Operand for SIMD shift operations.
struct SimdShiftOperand {
uint8_t shift;
unsigned length;
inline SimdShiftOperand(Decoder* decoder, const byte* pc) {
shift = decoder->checked_read_u8(pc, 2, "shift");
length = 1;
}
};
} // namespace wasm
} // namespace internal
} // namespace v8
#endif // V8_WASM_FUNCTION_BODY_DECODER_IMPL_H_
......@@ -10,7 +10,9 @@
#include "src/zone/zone-containers.h"
#include "src/wasm/decoder.h"
#include "src/wasm/function-body-decoder-impl.h"
#include "src/wasm/function-body-decoder.h"
#include "src/wasm/wasm-limits.h"
#include "src/wasm/wasm-module.h"
#include "src/wasm/wasm-opcodes.h"
......@@ -149,28 +151,6 @@ struct Control {
(build() ? CheckForException(builder_->func(__VA_ARGS__)) : nullptr)
#define BUILD0(func) (build() ? CheckForException(builder_->func()) : nullptr)
// Operand for SIMD lane operations.
struct SimdLaneOperand {
uint8_t lane;
unsigned length;
inline SimdLaneOperand(Decoder* decoder, const byte* pc) {
lane = decoder->checked_read_u8(pc, 2, "lane");
length = 1;
}
};
// Operand for SIMD shift operations.
struct SimdShiftOperand {
uint8_t shift;
unsigned length;
inline SimdShiftOperand(Decoder* decoder, const byte* pc) {
shift = decoder->checked_read_u8(pc, 2, "shift");
length = 1;
}
};
// Generic Wasm bytecode decoder with utilities for decoding operands,
// lengths, etc.
class WasmDecoder : public Decoder {
......@@ -209,7 +189,7 @@ class WasmDecoder : public Decoder {
uint32_t count = decoder->consume_u32v("local count");
if (decoder->failed()) return false;
if ((count + type_list->size()) > kMaxNumWasmLocals) {
if ((count + type_list->size()) > kV8MaxWasmFunctionLocals) {
decoder->error(decoder->pc() - 1, "local count too large");
return false;
}
......@@ -722,10 +702,12 @@ class WasmFullDecoder : public WasmDecoder {
while (pc_ < end_) { // decoding loop.
unsigned len = 1;
WasmOpcode opcode = static_cast<WasmOpcode>(*pc_);
if (!WasmOpcodes::IsPrefixOpcode(opcode)) {
TRACE(" @%-8d #%02x:%-20s|", startrel(pc_), opcode,
#if DEBUG
if (FLAG_trace_wasm_decoder && !WasmOpcodes::IsPrefixOpcode(opcode)) {
TRACE(" @%-8d #%-20s|", startrel(pc_),
WasmOpcodes::OpcodeName(opcode));
}
#endif
FunctionSig* sig = WasmOpcodes::Signature(opcode);
if (sig) {
......@@ -1216,8 +1198,8 @@ class WasmFullDecoder : public WasmDecoder {
len++;
byte simd_index = checked_read_u8(pc_, 1, "simd index");
opcode = static_cast<WasmOpcode>(opcode << 8 | simd_index);
TRACE(" @%-4d #%02x #%02x:%-20s|", startrel(pc_), kSimdPrefix,
simd_index, WasmOpcodes::OpcodeName(opcode));
TRACE(" @%-4d #%-20s|", startrel(pc_),
WasmOpcodes::OpcodeName(opcode));
len += DecodeSimdOpcode(opcode);
break;
}
......
......@@ -26,295 +26,10 @@ class WasmGraphBuilder;
namespace wasm {
const uint32_t kMaxNumWasmLocals = 8000000;
struct WasmGlobal;
// Helpers for decoding different kinds of operands which follow bytecodes.
struct LocalIndexOperand {
uint32_t index;
ValueType type;
unsigned length;
inline LocalIndexOperand(Decoder* decoder, const byte* pc) {
index = decoder->checked_read_u32v(pc, 1, &length, "local index");
type = kWasmStmt;
}
};
struct ImmI8Operand {
int8_t value;
unsigned length;
inline ImmI8Operand(Decoder* decoder, const byte* pc) {
value = bit_cast<int8_t>(decoder->checked_read_u8(pc, 1, "immi8"));
length = 1;
}
};
struct ImmI32Operand {
int32_t value;
unsigned length;
inline ImmI32Operand(Decoder* decoder, const byte* pc) {
value = decoder->checked_read_i32v(pc, 1, &length, "immi32");
}
};
struct ImmI64Operand {
int64_t value;
unsigned length;
inline ImmI64Operand(Decoder* decoder, const byte* pc) {
value = decoder->checked_read_i64v(pc, 1, &length, "immi64");
}
};
struct ImmF32Operand {
float value;
unsigned length;
inline ImmF32Operand(Decoder* decoder, const byte* pc) {
// Avoid bit_cast because it might not preserve the signalling bit of a NaN.
uint32_t tmp = decoder->checked_read_u32(pc, 1, "immf32");
memcpy(&value, &tmp, sizeof(value));
length = 4;
}
};
struct ImmF64Operand {
double value;
unsigned length;
inline ImmF64Operand(Decoder* decoder, const byte* pc) {
// Avoid bit_cast because it might not preserve the signalling bit of a NaN.
uint64_t tmp = decoder->checked_read_u64(pc, 1, "immf64");
memcpy(&value, &tmp, sizeof(value));
length = 8;
}
};
struct GlobalIndexOperand {
uint32_t index;
ValueType type;
const WasmGlobal* global;
unsigned length;
inline GlobalIndexOperand(Decoder* decoder, const byte* pc) {
index = decoder->checked_read_u32v(pc, 1, &length, "global index");
global = nullptr;
type = kWasmStmt;
}
};
struct BlockTypeOperand {
uint32_t arity;
const byte* types; // pointer to encoded types for the block.
unsigned length;
inline BlockTypeOperand(Decoder* decoder, const byte* pc) {
uint8_t val = decoder->checked_read_u8(pc, 1, "block type");
ValueType type = kWasmStmt;
length = 1;
arity = 0;
types = nullptr;
if (decode_local_type(val, &type)) {
arity = type == kWasmStmt ? 0 : 1;
types = pc + 1;
} else {
// Handle multi-value blocks.
if (!FLAG_wasm_mv_prototype) {
decoder->error(pc, pc + 1, "invalid block arity > 1");
return;
}
if (val != kMultivalBlock) {
decoder->error(pc, pc + 1, "invalid block type");
return;
}
// Decode and check the types vector of the block.
unsigned len = 0;
uint32_t count = decoder->checked_read_u32v(pc, 2, &len, "block arity");
// {count} is encoded as {arity-2}, so that a {0} count here corresponds
// to a block with 2 values. This makes invalid/redundant encodings
// impossible.
arity = count + 2;
length = 1 + len + arity;
types = pc + 1 + 1 + len;
for (uint32_t i = 0; i < arity; i++) {
uint32_t offset = 1 + 1 + len + i;
val = decoder->checked_read_u8(pc, offset, "block type");
decode_local_type(val, &type);
if (type == kWasmStmt) {
decoder->error(pc, pc + offset, "invalid block type");
return;
}
}
}
}
// Decode a byte representing a local type. Return {false} if the encoded
// byte was invalid or {kMultivalBlock}.
bool decode_local_type(uint8_t val, ValueType* result) {
switch (static_cast<ValueTypeCode>(val)) {
case kLocalVoid:
*result = kWasmStmt;
return true;
case kLocalI32:
*result = kWasmI32;
return true;
case kLocalI64:
*result = kWasmI64;
return true;
case kLocalF32:
*result = kWasmF32;
return true;
case kLocalF64:
*result = kWasmF64;
return true;
case kLocalS128:
*result = kWasmS128;
return true;
default:
*result = kWasmStmt;
return false;
}
}
ValueType read_entry(unsigned index) {
DCHECK_LT(index, arity);
ValueType result;
CHECK(decode_local_type(types[index], &result));
return result;
}
};
struct Control;
struct BreakDepthOperand {
uint32_t depth;
Control* target;
unsigned length;
inline BreakDepthOperand(Decoder* decoder, const byte* pc) {
depth = decoder->checked_read_u32v(pc, 1, &length, "break depth");
target = nullptr;
}
};
struct CallIndirectOperand {
uint32_t table_index;
uint32_t index;
FunctionSig* sig;
unsigned length;
inline CallIndirectOperand(Decoder* decoder, const byte* pc) {
unsigned len = 0;
index = decoder->checked_read_u32v(pc, 1, &len, "signature index");
table_index = decoder->checked_read_u8(pc, 1 + len, "table index");
if (table_index != 0) {
decoder->error(pc, pc + 1 + len, "expected table index 0, found %u",
table_index);
}
length = 1 + len;
sig = nullptr;
}
};
struct CallFunctionOperand {
uint32_t index;
FunctionSig* sig;
unsigned length;
inline CallFunctionOperand(Decoder* decoder, const byte* pc) {
unsigned len1 = 0;
unsigned len2 = 0;
index = decoder->checked_read_u32v(pc, 1 + len1, &len2, "function index");
length = len1 + len2;
sig = nullptr;
}
};
struct MemoryIndexOperand {
uint32_t index;
unsigned length;
inline MemoryIndexOperand(Decoder* decoder, const byte* pc) {
index = decoder->checked_read_u8(pc, 1, "memory index");
if (index != 0) {
decoder->error(pc, pc + 1, "expected memory index 0, found %u", index);
}
length = 1;
}
};
struct BranchTableOperand {
uint32_t table_count;
const byte* start;
const byte* table;
inline BranchTableOperand(Decoder* decoder, const byte* pc) {
DCHECK_EQ(kExprBrTable, decoder->checked_read_u8(pc, 0, "opcode"));
start = pc + 1;
unsigned len1 = 0;
table_count = decoder->checked_read_u32v(pc, 1, &len1, "table count");
if (table_count > (UINT_MAX / sizeof(uint32_t)) - 1 ||
len1 > UINT_MAX - (table_count + 1) * sizeof(uint32_t)) {
decoder->error(pc, "branch table size overflow");
}
table = pc + 1 + len1;
}
};
// A helper to iterate over a branch table.
class BranchTableIterator {
public:
unsigned cur_index() { return index_; }
bool has_next() { return decoder_->ok() && index_ <= table_count_; }
uint32_t next() {
DCHECK(has_next());
index_++;
unsigned length = 0;
uint32_t result =
decoder_->checked_read_u32v(pc_, 0, &length, "branch table entry");
pc_ += length;
return result;
}
// length, including the length of the {BranchTableOperand}, but not the
// opcode.
unsigned length() {
while (has_next()) next();
return static_cast<unsigned>(pc_ - start_);
}
const byte* pc() { return pc_; }
BranchTableIterator(Decoder* decoder, BranchTableOperand& operand)
: decoder_(decoder),
start_(operand.start),
pc_(operand.table),
index_(0),
table_count_(operand.table_count) {}
private:
Decoder* decoder_;
const byte* start_;
const byte* pc_;
uint32_t index_; // the current index.
uint32_t table_count_; // the count of entries, not including default.
};
struct MemoryAccessOperand {
uint32_t alignment;
uint32_t offset;
unsigned length;
inline MemoryAccessOperand(Decoder* decoder, const byte* pc,
uint32_t max_alignment) {
unsigned alignment_length;
alignment =
decoder->checked_read_u32v(pc, 1, &alignment_length, "alignment");
if (max_alignment < alignment) {
decoder->error(pc, pc + 1,
"invalid alignment; expected maximum alignment is %u, "
"actual alignment is %u",
max_alignment, alignment);
}
unsigned offset_length;
offset = decoder->checked_read_u32v(pc, 1 + alignment_length,
&offset_length, "offset");
length = alignment_length + offset_length;
}
};
typedef compiler::WasmGraphBuilder TFBuilder;
struct WasmModule; // forward declaration of module interface.
// All of the various data structures necessary to decode a function body.
// A wrapper around the signature and bytes of a function.
struct FunctionBody {
FunctionSig* sig; // function signature
const byte* base; // base of the module bytes, for error reporting
......
......@@ -3,6 +3,7 @@
// found in the LICENSE file.
#include "src/wasm/module-decoder.h"
#include "src/wasm/function-body-decoder-impl.h"
#include "src/base/functional.h"
#include "src/base/platform/platform.h"
......
......@@ -8,6 +8,7 @@
#include "src/utils.h"
#include "src/wasm/decoder.h"
#include "src/wasm/function-body-decoder-impl.h"
#include "src/wasm/function-body-decoder.h"
#include "src/wasm/wasm-external-refs.h"
#include "src/wasm/wasm-limits.h"
......
......@@ -7,6 +7,7 @@
#include "src/debug/interface-types.h"
#include "src/ostreams.h"
#include "src/vector.h"
#include "src/wasm/function-body-decoder-impl.h"
#include "src/wasm/function-body-decoder.h"
#include "src/wasm/wasm-module.h"
#include "src/wasm/wasm-opcodes.h"
......
......@@ -10,8 +10,10 @@
#include "src/objects.h"
#include "src/wasm/function-body-decoder-impl.h"
#include "src/wasm/function-body-decoder.h"
#include "src/wasm/signature-map.h"
#include "src/wasm/wasm-limits.h"
#include "src/wasm/wasm-macro-gen.h"
#include "src/wasm/wasm-module.h"
#include "src/wasm/wasm-opcodes.h"
......@@ -372,22 +374,22 @@ TEST_F(FunctionBodyDecoderTest, GetLocal_off_end) {
}
TEST_F(FunctionBodyDecoderTest, NumLocalBelowLimit) {
AddLocals(kWasmI32, kMaxNumWasmLocals - 1);
AddLocals(kWasmI32, kV8MaxWasmFunctionLocals - 1);
EXPECT_VERIFIES(v_v, WASM_NOP);
}
TEST_F(FunctionBodyDecoderTest, NumLocalAtLimit) {
AddLocals(kWasmI32, kMaxNumWasmLocals);
AddLocals(kWasmI32, kV8MaxWasmFunctionLocals);
EXPECT_VERIFIES(v_v, WASM_NOP);
}
TEST_F(FunctionBodyDecoderTest, NumLocalAboveLimit) {
AddLocals(kWasmI32, kMaxNumWasmLocals + 1);
AddLocals(kWasmI32, kV8MaxWasmFunctionLocals + 1);
EXPECT_FAILURE(v_v, WASM_NOP);
}
TEST_F(FunctionBodyDecoderTest, GetLocal_varint) {
const int kMaxLocals = kMaxNumWasmLocals - 1;
const int kMaxLocals = kV8MaxWasmFunctionLocals - 1;
AddLocals(kWasmI32, kMaxLocals);
EXPECT_VERIFIES(i_i, kExprGetLocal, U32V_1(66));
......@@ -408,7 +410,7 @@ TEST_F(FunctionBodyDecoderTest, GetLocal_varint) {
}
TEST_F(FunctionBodyDecoderTest, GetLocal_toomany) {
AddLocals(kWasmI32, kMaxNumWasmLocals - 100);
AddLocals(kWasmI32, kV8MaxWasmFunctionLocals - 100);
AddLocals(kWasmI32, 100);
EXPECT_VERIFIES(i_v, kExprGetLocal, U32V_1(66));
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment