[BUG] Exception may be throwed when attrs is constructed

[hanzz2007]

template <typename FFind>
class AttrInitVisitor {
 public:
  // Counter of number of matched attributes during visit.
  // This is used to decide if there is additional unmatched attributes.
  size_t hit_count_{0};
  // constructor
  AttrInitVisitor(const char* type_key, FFind ffind) : type_key_(type_key), ffind_(ffind) {}

  template <typename T>
  AttrInitEntry<T> operator()(const char* key, T* value) {  
    TVMArgValue val;
    AttrInitEntry<T> opt;
    opt.type_key_ = type_key_;
    opt.key_ = key;
    opt.value_ = value;
    if (ffind_(key, &val)) {
      SetValue(value, val);
      opt.value_missing_ = false;
      ++hit_count_;
    } else {
      opt.value_missing_ = true;
    }

    return opt;
   // for compilers without RVO optimization, the opt.~AttrInitEntry<T>() will be called when function exit 
   // An exception will be throwed if opt.value_missing_ is ture in destructor
  }

When the AttrsNode is first constructed

template <typename TAttrs>
inline TAttrs AttrsWithDefaultValues() {
  static_assert(std::is_base_of<Attrs, TAttrs>::value, "Can only take attr nodes");
  auto n = make_object<typename TAttrs::ContainerType>();
  // Note here, will cause a problem
  n->InitByPackedArgs(runtime::TVMArgs(nullptr, nullptr, 0), false);
  return TAttrs(n);
}

Maybe we could fix it via the following codes

https://gist.github.com/hanzz2007/803b28a7915c31686cffce0ac6450b73
https://gist.github.com/hanzz2007/e42b14b51c8c9c765cca107867e64d49

[hanzz2007]

for example

struct LoopPartitionConfigNode : public tvm::AttrsNode<LoopPartitionConfigNode> {
  bool partition_const_loop;

  TVM_DECLARE_ATTRS(LoopPartitionConfigNode, "tir.transform.LoopPartitionConfig") {
    //macro expanded:  __fvisitor__(xxx, xxx)   /*note here, __fvisitor__ return a AttrInitEntry<T> object by value, an copy constructor and destructor will be called */
    //     .describe().set_default()
    TVM_ATTR_FIELD(partition_const_loop).describe("Split constant loop").set_default(false);
  }
};

[tqchen]

Can you elaborate? It is the intended behavior to throw when a value is missing

[hanzz2007]

I know it's intended when we want to initialize AttrsNode with arguments, but what if initialize with default value, if (ffind_(key, &val)) will always return false, so opt.value_missing_ must be true in AttrInitEntry<T> operator()(const char* key, T* value) ,
and when return from AttrInitEntry<T> operator(), opt will be destructed so exception throwed.

opt.value_missing_ is set to false when ... constant loop").set_default(false); executed, but it's too late, because opt constructed in AttrInitEntry<T> operator() has been called destructor function when function finish.

In gcc and llvm, no exception throwed for opt.~AttrInitEntry() will not be called because RVO optimization I guess. But it should be a problem in standard way.

[tqchen]

I see, would it be suffice to force move in the return statement?

[hanzz2007]

Yes, move can solve. And we should add a default constructor and a rvref constructor to AttrInitEntry

template <typename T>
struct AttrInitEntry {
  // The attributes
  using TSelf = AttrInitEntry<T>;
  // The type key
  const char* type_key_;
  // field name
  const char* key_;
  // internal value.
  T* value_;
  // whether the value is missing.
  bool value_missing_{true};

  AttrInitEntry() = default;

  AttrInitEntry(AttrInitEntry&& other) {
    type_key_ = other.type_key_;
    key_ = other.key_;
    value_ = other.value_;
    value_missing_ = other.value_missing_;
    other.value_missing_ = false;  // note! avoid throw
  }

  // If the value is still missing in destruction time throw an error.
  ~AttrInitEntry() DMLC_THROW_EXCEPTION {
    if (value_missing_) {
      std::ostringstream os;
      os << type_key_ << ": Cannot find required field \'" << key_ << "\' during initialization";
      throw AttrError(os.str());
    }
  }
  // override fields.
  // This function sets the lower bound of the attribute
  TSelf& set_lower_bound(DMLC_ATTRIBUTE_UNUSED const T& begin) {
    if (this->value_missing_) return *this;
    const T& val = *value_;
    if (begin > val) {
      std::ostringstream os;
      os << type_key_ << "." << key_ << ": "
         << "value " << val << " is smaller than the lower bound " << begin;
      throw AttrError(os.str());
    }
    return *this;
  }
  // This function sets the upper bound of the attribute
  TSelf& set_upper_bound(DMLC_ATTRIBUTE_UNUSED const T& end) {
    if (this->value_missing_) return *this;
    const T& val = *value_;
    if (val > end) {
      std::ostringstream os;
      os << type_key_ << "." << key_ << ": "
         << "value " << val << " is bigger than the upper bound " << end;
      throw AttrError(os.str());
    }
    return *this;
  }
  // set default when
  TSelf& set_default(DMLC_ATTRIBUTE_UNUSED const T& value) {
    if (!value_missing_) return *this;
    *value_ = value;
    value_missing_ = false;
    return *this;
  }
  TSelf& describe(DMLC_ATTRIBUTE_UNUSED const char* str) { return *this; }
};


template <typename FFind>
class AttrInitVisitor {
 public:
  // Counter of number of matched attributes during visit.
  // This is used to decide if there is additional unmatched attributes.
  size_t hit_count_{0};
  // constructor
  AttrInitVisitor(const char* type_key, FFind ffind) : type_key_(type_key), ffind_(ffind) {}

  template <typename T>
  AttrInitEntry<T> operator()(const char* key, T* value) {
    AttrInitEntry<T> opt;
    TVMArgValue val;
    opt.type_key_ = type_key_;
    opt.key_ = key;
    opt.value_ = value;
    if (ffind_(key, &val)) {
      SetValue(value, val);
      opt.value_missing_ = false;
      ++hit_count_;
    } else {
      opt.value_missing_ = true;
    }
    return std::move(opt); // Note! force move
  }
  
 private:
  // the type key
  const char* type_key_;
  FFind ffind_;
};

[tqchen]

Sounds good, feel free to send a PR