ngraph-tf icon indicating copy to clipboard operation
ngraph-tf copied to clipboard

Fix the implicitly constructions of ArraySlice from initializer lists

Open samolisov opened this issue 6 years ago • 4 comments

In 'ngraph_utils.cc', methods like 'const gtl::ArraySlice<DataType>& NGraphNumericDTypes()' directly construct an instance of 'gtl::ArraySlice' from an intializer list. This leads to the corruption of 'DataType' instances held inside the array slice, and therefore no 'DataType' can pass the following check within the 'TypeConstraintOk' method:

DataType dt;

if (GetNodeAttr(node->attrs(), type_attr_name, &dt) != Status::OK() ||
      std::find(allowed_types.begin(), allowed_types.end(), dt) ==
	  allowed_types.end()) {

Since the 'allowed_types' array contains already destructed instances of 'DataType's.

The constructor of 'gtl::ArraySlice' that takes an initializer list has the following comment (see https://github.com/petewarden/tensorflow_makefile/blob/master/tensorflow/core/lib/gtl/array_slice.h):

// Implicitly constructs an ArraySlice from an initializer list. This makes it
// possible to pass a brace-enclosed initializer list to a function expecting
// an ArraySlice:
//   void Process(ArraySlice<int> x);
//   Process({1, 2, 3});
// The data referenced by the initializer_list must outlive this
// ArraySlice. For example, "ArraySlice<int> s={1,2};" and "return
// ArraySlice<int>({3,4});" are errors, as the resulting ArraySlice may
// reference data that is no longer valid.

So, an additional memory is required to store the held instances of the 'DataType' class. This commit introduces 'std::vector' as the store for 'DataType's. For backward compatibility, a static variable of 'ArraySlice<DataType>' is introduced in each method and the methods return references to those variables.

Signed-off-by: Pavel Samolysov [email protected]

samolisov avatar Mar 04 '19 10:03 samolisov

@avijit-nervana Could you have a look at the PR, please? Maybe I wasn't able to properly titled the PR, but it is very important because without this change any DataTypes in gtl::ArraySlices will destructed and no one DType will be considered as compatible with NGraph, so no NGraph-nodes will be detected in ANY graph. I see this at least if NGraph and NGgraph-TF are compiled with CLang 7/8.

samolisov avatar Mar 21 '19 07:03 samolisov

TESTNOW

avijit-nervana avatar Mar 23 '19 14:03 avijit-nervana

TESTNOW

crlishka avatar Mar 25 '19 06:03 crlishka

I have no access to the CI server and unfortunately am not able to see what is wrong with the commits.

samolisov avatar Mar 27 '19 15:03 samolisov