Segment flags should be MVP-compat when possible (#1286)

This should fix #1280, #1268, #1269.

Author: binji

src/binary-reader-ir.cc

@@ -1016,7 +1016,13 @@ Result BinaryReaderIR::BeginElemSegment(Index index,
   auto field = MakeUnique<ElemSegmentModuleField>(GetLocation());
   ElemSegment& elem_segment = field->elem_segment;
   elem_segment.table_var = Var(table_index, GetLocation());
-  elem_segment.flags = flags;
+  if ((flags & SegDeclared) == SegDeclared) {
+    elem_segment.kind = SegmentKind::Declared;
+  } else if ((flags & SegPassive) == SegPassive) {
+    elem_segment.kind = SegmentKind::Passive;
+  } else {
+    elem_segment.kind = SegmentKind::Active;
+  }
   elem_segment.elem_type = elem_type;
   module_->AppendField(std::move(field));
   return Result::Ok;
@@ -1071,7 +1077,11 @@ Result BinaryReaderIR::BeginDataSegment(Index index,
   auto field = MakeUnique<DataSegmentModuleField>(GetLocation());
   DataSegment& data_segment = field->data_segment;
   data_segment.memory_var = Var(memory_index, GetLocation());
-  data_segment.flags = flags;
+  if ((flags & SegPassive) == SegPassive) {
+    data_segment.kind = SegmentKind::Passive;
+  } else {
+    data_segment.kind = SegmentKind::Active;
+  }
   module_->AppendField(std::move(field));
   return Result::Ok;
 }

src/binary-reader.cc

@@ -2168,10 +2168,11 @@ Result BinaryReader::ReadElemSection(Offset section_size) {
   for (Index i = 0; i < num_elem_segments; ++i) {
     uint32_t flags;
     CHECK_RESULT(ReadU32Leb128(&flags, "elem segment flags"));
-    ERROR_IF(flags > ~(~0u << SegFlagMax), "invalid elem segment flags: %#x",
-             flags);
+    ERROR_IF(flags > SegFlagMax, "invalid elem segment flags: %#x", flags);
+    ERROR_IF((flags & SegDeclared) == SegDeclared,
+             "declared segments aren't supported");
     Index table_index(0);
-    if (flags & SegExplicitIndex) {
+    if ((flags & (SegPassive | SegExplicitIndex)) == SegExplicitIndex) {
       CHECK_RESULT(ReadIndex(&table_index, "elem segment table index"));
     }
     Type elem_type = Type::Funcref;
@@ -2185,8 +2186,7 @@ Result BinaryReader::ReadElemSection(Offset section_size) {
     }
 
     // For backwards compat we support not declaring the element kind.
-    bool legacy = !(flags & SegPassive) && !(flags & SegExplicitIndex);
-    if (!legacy) {
+    if (flags & (SegPassive | SegExplicitIndex)) {
       if (flags & SegUseElemExprs) {
         CHECK_RESULT(ReadType(&elem_type, "table elem type"));
         ERROR_UNLESS(IsRefType(elem_type),
@@ -2288,8 +2288,7 @@ Result BinaryReader::ReadDataSection(Offset section_size) {
   for (Index i = 0; i < num_data_segments; ++i) {
     uint32_t flags;
     CHECK_RESULT(ReadU32Leb128(&flags, "data segment flags"));
-    ERROR_IF(flags > ~(~0u << SegFlagMax), "invalid data segment flags: %#x",
-             flags);
+    ERROR_IF(flags > SegFlagMax, "invalid data segment flags: %#x", flags);
     Index memory_index(0);
     if (flags & SegExplicitIndex) {
       CHECK_RESULT(ReadIndex(&memory_index, "data segment memory index"));

src/binary-writer.cc

@@ -1055,30 +1055,29 @@ Result BinaryWriter::WriteModule() {
       ElemSegment* segment = module_->elem_segments[i];
       WriteHeader("elem segment header", i);
       // 1. flags
-      stream_->WriteU8(segment->flags, "segment flags");
+      uint8_t flags = segment->GetFlags(module_);
+      stream_->WriteU8(flags, "segment flags");
       // 2. optional target table
-      if (segment->flags & SegExplicitIndex) {
-        WriteU32Leb128(stream_, module_->GetTableIndex(segment->table_var), "table index");
+      if (flags & SegExplicitIndex) {
+        WriteU32Leb128(stream_, module_->GetTableIndex(segment->table_var),
+                       "table index");
       }
       // 3. optional target location within the table (active segments only)
-      if (!segment->is_passive()) {
+      if (!(flags & SegPassive)) {
         WriteInitExpr(segment->offset);
       }
-      // 4. type of item in the following list
-      bool legacy =
-          !segment->is_passive() && !(segment->flags & SegExplicitIndex);
-      if (!legacy) {
-        if (segment->flags & SegUseElemExprs) {
+      // 4. type of item in the following list (omitted for "legacy" segments)
+      if (flags & (SegPassive | SegExplicitIndex)) {
+        if (flags & SegUseElemExprs) {
           WriteType(stream_, segment->elem_type, "elem expr list type");
         } else {
-          assert(segment->elem_type == Type::Funcref);
           stream_->WriteU8Enum(ExternalKind::Func, "elem list type");
         }
       }
       // 5. actual list of elements (with extern indexes or elem expr's)
       // preceeded by length
       WriteU32Leb128(stream_, segment->elem_exprs.size(), "num elems");
-      if (segment->flags & SegUseElemExprs) {
+      if (flags & SegUseElemExprs) {
         for (const ElemExpr& elem_expr : segment->elem_exprs) {
           switch (elem_expr.kind) {
             case ElemExprKind::RefNull:
@@ -1137,8 +1136,9 @@ Result BinaryWriter::WriteModule() {
     for (size_t i = 0; i < module_->data_segments.size(); ++i) {
       const DataSegment* segment = module_->data_segments[i];
       WriteHeader("data segment header", i);
-      stream_->WriteU8(segment->flags, "segment flags");
-      if (!segment->is_passive()) {
+      uint8_t flags = segment->GetFlags(module_);
+      stream_->WriteU8(flags, "segment flags");
+      if (!(flags & SegPassive)) {
         assert(module_->GetMemoryIndex(segment->memory_var) == 0);
         WriteInitExpr(segment->offset);
       }

src/common.h

@@ -229,14 +229,21 @@ enum class Type : int32_t {
 };
 typedef std::vector<Type> TypeVector;
 
+enum class SegmentKind {
+  Active,
+  Passive,
+  Declared,
+};
+
 // Matches binary format, do not change.
 enum SegmentFlags : uint8_t {
   SegFlagsNone = 0,
   SegPassive = 1,        // bit 0: Is passive
-  SegExplicitIndex = 2,  // bit 1: Has explict index (Inplies table 0 if absent)
+  SegExplicitIndex = 2,  // bit 1: Has explict index (Implies table 0 if absent)
+  SegDeclared = 3,       // Only used for declared segments
   SegUseElemExprs = 4,   // bit 2: Is elemexpr (Or else index sequence)
 
-  SegFlagMax = SegUseElemExprs,
+  SegFlagMax = (SegUseElemExprs << 1) - 1,  // All bits set.
 };
 
 enum class RelocType {

src/ir.cc

@@ -651,4 +651,52 @@ Const::Const(F64Tag, uint64_t value, const Location& loc_)
 Const::Const(V128Tag, v128 value, const Location& loc_)
     : loc(loc_), type(Type::V128), vec128(value) {}
 
+uint8_t ElemSegment::GetFlags(const Module* module) const {
+  uint8_t flags = 0;
+
+  switch (kind) {
+    case SegmentKind::Active: {
+      Index table_index = module->GetTableIndex(table_var);
+      if (table_index != 0) {
+        flags |= SegExplicitIndex;
+      }
+      break;
+    }
+
+    case SegmentKind::Passive:
+      flags |= SegPassive;
+      break;
+
+    case SegmentKind::Declared:
+      flags |= SegDeclared;
+      break;
+  }
+
+  bool all_ref_func = std::all_of(
+      elem_exprs.begin(), elem_exprs.end(), [](const ElemExpr& elem_expr) {
+        return elem_expr.kind == ElemExprKind::RefFunc;
+      });
+  if (!all_ref_func) {
+    flags |= SegUseElemExprs;
+  }
+
+  return flags;
+}
+
+uint8_t DataSegment::GetFlags(const Module* module) const {
+  uint8_t flags = 0;
+
+  if (kind == SegmentKind::Passive) {
+    flags |= SegPassive;
+  }
+
+  Index memory_index = module->GetMemoryIndex(memory_var);
+  if (memory_index != 0) {
+    flags |= SegExplicitIndex;
+  }
+
+  return flags;
+}
+
+
 }  // namespace wabt

src/ir.h

@@ -33,6 +33,8 @@
 
 namespace wabt {
 
+struct Module;
+
 enum class VarType {
   Index,
   Name,
@@ -600,11 +602,11 @@ typedef std::vector<ElemExpr> ElemExprVector;
 
 struct ElemSegment {
   explicit ElemSegment(string_view name) : name(name.to_string()) {}
-  bool is_passive() const { return flags & SegPassive; }
+  uint8_t GetFlags(const Module*) const;
 
+  SegmentKind kind = SegmentKind::Active;
   std::string name;
   Var table_var;
-  uint8_t flags = 0;
   Type elem_type;
   ExprList offset;
   ElemExprVector elem_exprs;
@@ -619,11 +621,11 @@ struct Memory {
 
 struct DataSegment {
   explicit DataSegment(string_view name) : name(name.to_string()) {}
-  bool is_passive() const { return flags & SegPassive; }
+  uint8_t GetFlags(const Module*) const;
 
+  SegmentKind kind = SegmentKind::Active;
   std::string name;
   Var memory_var;
-  uint8_t flags = 0;
   ExprList offset;
   std::vector<uint8_t> data;
 };

src/validator.cc

@@ -1152,16 +1152,24 @@ void Validator::CheckTable(const Location* loc, const Table* table) {
 }
 
 void Validator::CheckElemSegments(const Module* module) {
+  bool bulk_or_ref_enabled = options_.features.bulk_memory_enabled() ||
+                             options_.features.reference_types_enabled();
+
   for (const ModuleField& field : module->fields) {
     if (auto elem_segment_field = dyn_cast<ElemSegmentModuleField>(&field)) {
       auto&& elem_segment = elem_segment_field->elem_segment;
       for (const ElemExpr& elem_expr : elem_segment.elem_exprs) {
         if (elem_expr.kind == ElemExprKind::RefFunc) {
           CheckFuncVar(&elem_expr.var, nullptr);
+        } else {
+          assert(elem_expr.kind == ElemExprKind::RefNull);
+          if (!bulk_or_ref_enabled) {
+            PrintError(&field.loc, "ref.null is not allowed");
+          }
         }
       }
 
-      if (elem_segment.is_passive()) {
+      if (elem_segment.kind == SegmentKind::Passive) {
         continue;
       }
       if (Failed(CheckTableVar(&elem_segment.table_var, nullptr))) {
@@ -1193,7 +1201,7 @@ void Validator::CheckDataSegments(const Module* module) {
     if (auto data_segment_field = dyn_cast<DataSegmentModuleField>(&field)) {
       auto&& data_segment = data_segment_field->data_segment;
       const Memory* memory;
-      if (data_segment.is_passive()) {
+      if (data_segment.kind == SegmentKind::Passive) {
         continue;
       }
       if (Failed(CheckMemoryVar(&data_segment.memory_var, &memory))) {

src/wast-parser.cc

@@ -933,12 +933,12 @@ Result WastParser::ParseDataModuleField(Module* module) {
   } else if (ParseVarOpt(&field->data_segment.memory_var, Var(0, loc))) {
     CHECK_RESULT(ParseOffsetExpr(&field->data_segment.offset));
   } else if (!ParseOffsetExprOpt(&field->data_segment.offset)) {
-    if (options_->features.bulk_memory_enabled()) {
-      field->data_segment.flags |= SegPassive;
-    } else {
+    if (!options_->features.bulk_memory_enabled()) {
       Error(loc, "passive data segments are not allowed");
       return Result::Error;
     }
+
+    field->data_segment.kind = SegmentKind::Passive;
   }
 
   ParseTextListOpt(&field->data_segment.data);
@@ -956,7 +956,7 @@ Result WastParser::ParseElemModuleField(Module* module) {
   // With MVP text format the name here was intended to refer to the table
   // that the elem segment was part of, but we never did anything with this name
   // since there was only one table anyway.
-  // With bulk-memory enabled this introduces a new name for the particualr
+  // With bulk-memory enabled this introduces a new name for the particular
   // elem segment.
   std::string initial_name;
   bool has_name = ParseBindVarOpt(&initial_name);
@@ -970,7 +970,6 @@ Result WastParser::ParseElemModuleField(Module* module) {
   // Optional table specifier
   if (options_->features.bulk_memory_enabled()) {
     if (PeekMatchLpar(TokenType::Table)) {
-      field->elem_segment.flags |= SegExplicitIndex;
       EXPECT(Lpar);
       EXPECT(Table);
       CHECK_RESULT(ParseVar(&field->elem_segment.table_var));
@@ -981,23 +980,25 @@ Result WastParser::ParseElemModuleField(Module* module) {
   } else {
     if (has_name) {
       field->elem_segment.table_var = Var(initial_name, loc);
-      field->elem_segment.flags |= SegExplicitIndex;
     } else {
       ParseVarOpt(&field->elem_segment.table_var, Var(0, loc));
     }
   }
 
   if (!ParseOffsetExprOpt(&field->elem_segment.offset)) {
-    field->elem_segment.flags |= SegPassive;
+    field->elem_segment.kind = SegmentKind::Passive;
   }
 
   if (ParseRefTypeOpt(&field->elem_segment.elem_type)) {
-    // TODO: Don't allow unless bulk memory is enabled.
-    field->elem_segment.flags |= (SegPassive | SegUseElemExprs);
     // Parse a potentially empty sequence of ElemExprs.
     while (true) {
       Var var;
+      Location loc = GetLocation();
       if (MatchLpar(TokenType::RefNull)) {
+        if (!(options_->features.bulk_memory_enabled() ||
+              options_->features.reference_types_enabled())) {
+          Error(loc, "ref.null not allowed");
+        }
         field->elem_segment.elem_exprs.emplace_back();
         EXPECT(Rpar);
       } else if (MatchLpar(TokenType::RefFunc)) {
@@ -1319,8 +1320,6 @@ Result WastParser::ParseTableModuleField(Module* module) {
     auto elem_segment_field = MakeUnique<ElemSegmentModuleField>(loc);
     ElemSegment& elem_segment = elem_segment_field->elem_segment;
     elem_segment.table_var = Var(module->tables.size());
-    if (module->tables.size() > 0)
-      elem_segment.flags |= SegExplicitIndex;
     elem_segment.offset.push_back(MakeUnique<ConstExpr>(Const::I32(0)));
     elem_segment.offset.back().loc = loc;
     elem_segment.elem_type = elem_type;

src/wat-writer.cc

@@ -1258,19 +1258,21 @@ void WatWriter::WriteElemSegment(const ElemSegment& segment) {
   WriteOpenSpace("elem");
   WriteNameOrIndex(segment.name, elem_segment_index_, NextChar::Space);
 
-  if (!segment.is_passive()) {
+  uint8_t flags = segment.GetFlags(&module);
+
+  if (!(flags & SegPassive)) {
     WriteInitExpr(segment.offset);
   }
 
-  if (segment.flags & SegUseElemExprs) {
+  if (flags & SegUseElemExprs) {
     WriteType(segment.elem_type, NextChar::Space);
   } else {
     assert(segment.elem_type == Type::Funcref);
     WritePuts("func", NextChar::Space);
   }
 
   for (const ElemExpr& expr : segment.elem_exprs) {
-    if (segment.flags & SegUseElemExprs) {
+    if (flags & SegUseElemExprs) {
       if (expr.kind == ElemExprKind::RefNull) {
         WriteOpenSpace("ref.null");
         WriteCloseSpace();
@@ -1301,7 +1303,7 @@ void WatWriter::WriteMemory(const Memory& memory) {
 void WatWriter::WriteDataSegment(const DataSegment& segment) {
   WriteOpenSpace("data");
   WriteNameOrIndex(segment.name, data_segment_index_, NextChar::Space);
-  if (!segment.is_passive()) {
+  if (segment.kind != SegmentKind::Passive) {
     WriteInitExpr(segment.offset);
   }
   WriteQuotedData(segment.data.data(), segment.data.size());

test/binary/bad-elem-flags-declared.txt

@@ -0,0 +1,17 @@
+;;; TOOL: run-gen-wasm-bad
+magic
+version
+section(TABLE) {
+  count[1]
+  anyfunc
+  has_max[0]
+  initial[0]
+}
+section(ELEM) {
+  count[1]
+  flags[3]
+}
+(;; STDERR ;;;
+0000012: error: declared segments aren't supported
+0000012: error: declared segments aren't supported
+;;; STDERR ;;)