Fix opaque types usages #114

kornilova203 · 2018-07-10T22:27:01Z

Closes #106

Replaces pointers to opaque types with pointers to Byte

struct undefinedStruct;
struct undefinedStruct *getUndefinedStruct();

def getUndefinedStruct(): native.Ptr[Byte] = native.extern

If there exists a declaration that uses an opaque type value then the declaration is skipped and error messages are printed:

struct undefinedStruct;
extern struct undefinedStruct s;

Error: type definition for struct_undefinedStruct was not found.
Error: Variable s is skipped because it references an opaque type.

jonas · 2018-07-11T14:24:10Z

tests/samples/include/opaqueTypes.h

@@ -0,0 +1,7 @@
+struct s;


Rename to start with upper case: OpaqueTypes.h

jonas · 2018-07-11T14:31:03Z

bindgen/ir/TypeDef.h

+
+    bool usesOpaqueType() const override;
+
+    using TypeAndName::replaceType;


Is this used?

TypeDef inherits from Type and TypeAndName, so we need to specify what version of replaceType should be used.
A typedef may contain pointer to opaque type that should be replaced with pointer to Byte:

typedef struct s *s;

type s = native.Ptr[Byte]

jonas · 2018-07-11T14:47:57Z

tests/samples/OpaqueTypes.scala

  def move(point: native.Ptr[struct_point], x: native.CInt, y: native.CInt): native.Ptr[struct_point] = native.extern
  def processPoints(p: native.Ptr[points]): native.Ptr[union_u] = native.extern
+  def usePointerToUndefinedStruct(anonymous0: native.Ptr[Byte]): Unit = native.extern


Looking at this I think it would be better to have

type struct_structWithPointerToUndefinedStruct = native.CStruct0 type union_unionWithPointerToUndefinedStruct = native.CArray[Byte, native.Nat._0] // not sure if 0 length is supported def usePointerToUndefinedStruct(anonymous0: native.Ptr[struct_structWithPointerToUndefinedStruct]): Unit = native.extern

So CStruct0 for opaque structs and then use the type alias in the pointers. With default opaque representations for structs and unions it should be possible to remove most use of replaceType except for updates to structs and unions.

No helper class should be generated for such types.

Seems like a good solution.
Should we leave a comment beside CStruct0 that it is an incomplete type or opaque type, so no one will think that it is indeed empty structure?

jonas · 2018-07-11T14:57:27Z

bindgen/ir/types/PointerType.cpp

@@ -23,3 +23,14 @@ bool PointerType::operator==(const Type &other) const {
    }
    return false;
 }
+
+bool PointerType::usesOpaqueType() const { return type->usesOpaqueType(); }


A pointer to an opaque type is always sound, no?

Yes, but by the time this method is called, all pointers to opaque type are replaced with pointers to Byte, so this method is used to finds illegal usages of opaque type

jonas · 2018-07-11T15:03:03Z

tests/src/test/scala/org/scalanative/bindgen/BindgenSpec.scala

+              errs == "Error: type definition for struct_undefinedStruct was not found.\n" +
+                "Error: Function useUndefinedStruct is skipped because it references an opaque type.")
+          case _ => assert(errs == "")
+        }


To keep the tests clean, I would prefer to have checks for errors in a separate Spec.

kornilova203 · 2018-07-12T11:38:46Z

Opaque types may be only Struct or Union, they are referenced only through corresponding pointers, so it is okay to replace nullptr in these TypeDefs with CStruct0.
But then we need to check if there are cases when the type is used as value. It may happen in these situations:

struct/union field of opaque type. This situation is illegal, Clang prints error error: field has incomplete type but it does not prevent construction of AST tree, so Scala Code is generated.
I don't think that we should stop bindgen in this case because them problem may be related to inaccuracy in compilation database. It should be okay to remove such damaged structures and all code that uses them.
struct/union field of array type with elements of opaque type. The same situation as above.
function pointer type that uses value of opaque type.
Seems like it is okay. Such pointers may be passed between Scala code and native lib code. It is even possible to execute such pointer if cast it to a function pointer that uses complete type.
function pointer type that uses array of opaque type.
That looks strange to me but C code does not compile if a function parameter is an array of incomplete type.
If we simply change struct incompleteS[] to struct incompleteS * code does compile.
I think we should not care about this much because in any case parameter of array type becomes parameter of pointer type in AST tree.
function uses value of opaque type. All functions that use values of structs or unions should be removed because it is not supported in Scala Native. (this is done in Skip functions that pass arrays or structs by value #119)
extern variable of opaque type. The variable and corresponding VarDefines may be safely removed.

kornilova203 · 2018-07-12T22:56:06Z

I decided not to remove structs with fields of opaque types because it is not normal situation, clangs prints error in this case.
Removing such structs and all declarations that use these structs will require relatively big amount of new code and it does not make a lot of sense because bindings cannot be correct if header files of compilation database is broken.

jonas · 2018-07-13T20:58:00Z

bindgen/ir/TypeDef.cpp

+    if (typeDef.type) {
+        s << typeDef.getType()->str();
+    } else {
+        s << "native.CStruct0 // incomplete type";


I suggest to remove the comment completely, else please change it to say: opaque type.

I used incomplete because it is the term that clang uses in error messages.

Okay makes sense then.

jonas · 2018-07-13T21:08:52Z

tests/src/test/scala/org/scalanative/bindgen/BindgenSpec.scala

+          case "LiteralDefine" =>
+            assert(
+              errs == "Warning: integer value does not fit into 8 bytes: 18446744073709551615\n" +
+                "Warning: integer value does not fit into 8 bytes: 9223372036854775809")


Sorry for not being more clear, what I meant by "have checks for errors in a separate Spec" was to do something like:

class BindgenReportingSpec extends FunSpec { it("Skip variable with opaque type") { val result = bindgen( input = "struct undefinedStruct; |extern struct undefinedStruct removedExtern; |#define removedExternAlias removedExtern |".stripMargin) assert( result.error == "Error: Variable alias removedExternAlias is skipped because it has incomplete type") ) } }

That is avoid mixing code with errors into the header files used by BindgenSpec and instead have focused tests.

I know it is a bit more work but I think the clarity it brings will pay off in the long run.

Add comment line to incomplete types

jonas reviewed Jul 11, 2018

View reviewed changes

kornilova203 force-pushed the fix-opaque-types-usages branch from 3660984 to 926ae00 Compare July 12, 2018 22:44

kornilova203 changed the base branch from master to skip-illegal-scala-functions July 12, 2018 22:44

kornilova203 force-pushed the fix-opaque-types-usages branch from 926ae00 to 3a792b8 Compare July 12, 2018 22:52

kornilova203 force-pushed the fix-opaque-types-usages branch from 3a792b8 to 072189c Compare July 12, 2018 22:59

kornilova203 requested a review from jonas July 13, 2018 11:17

kornilova203 mentioned this pull request Jul 13, 2018

Fix detection of cyclic structs and unions #116

Merged

3 tasks

kornilova203 force-pushed the fix-opaque-types-usages branch from 98b3cdc to ff5491e Compare July 13, 2018 19:37

jonas reviewed Jul 13, 2018

View reviewed changes

kornilova203 changed the base branch from skip-illegal-scala-functions to 0.3 July 16, 2018 09:09

kornilova203 force-pushed the fix-opaque-types-usages branch 2 times, most recently from 0048a24 to 8e88cf7 Compare July 17, 2018 17:10

kornilova203 force-pushed the 0.3 branch 3 times, most recently from f527470 to 65450fd Compare July 18, 2018 16:27

kornilova203 force-pushed the fix-opaque-types-usages branch 2 times, most recently from b1b7d5f to 591b889 Compare July 21, 2018 05:24

Replace opaque type with CStruct0

8c598e7

Add comment line to incomplete types

kornilova203 force-pushed the fix-opaque-types-usages branch from 591b889 to 8c598e7 Compare July 21, 2018 05:26

kornilova203 merged commit 9eb0ba5 into 0.3 Jul 22, 2018

kornilova203 deleted the fix-opaque-types-usages branch July 22, 2018 04:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix opaque types usages #114

Fix opaque types usages #114

kornilova203 commented Jul 10, 2018

jonas Jul 11, 2018

jonas Jul 11, 2018

kornilova203 Jul 12, 2018

jonas Jul 11, 2018 •

edited

jonas Jul 11, 2018

kornilova203 Jul 12, 2018

jonas Jul 11, 2018

kornilova203 Jul 12, 2018

jonas Jul 11, 2018

kornilova203 commented Jul 12, 2018 •

edited

kornilova203 commented Jul 12, 2018

jonas Jul 13, 2018

kornilova203 Jul 16, 2018

jonas Jul 16, 2018

jonas Jul 13, 2018


		bool usesOpaqueType() const override;

		using TypeAndName::replaceType;

Fix opaque types usages #114

Fix opaque types usages #114

Conversation

kornilova203 commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas Jul 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kornilova203 commented Jul 12, 2018 • edited

kornilova203 commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas Jul 11, 2018 •

edited

kornilova203 commented Jul 12, 2018 •

edited