Handle opaque types #94

kornilova203 · 2018-07-02T13:03:03Z

Closes #78
Closes #88

Create TypeDef instance that references OpaqueType
Replace OpaqueType instances by Struct or Union when types are declared

kornilova203 · 2018-07-03T09:43:09Z

OpaqueType was redundant.
For opaque types I decided to create instances of TypeDef that contain nullptr which is further replaced by a reference to Struct or Union.

jonas · 2018-07-03T10:57:04Z

bindgen/TypeTranslator.cpp

@@ -138,6 +147,10 @@ std::shared_ptr<Type> TypeTranslator::translate(const clang::QualType &qtpe,
            std::make_shared<PrimitiveType>("Byte"), sizeInBits / 8);
    }

+    if (tpe->isFunctionType()) {
+        return nullptr;


Is this for static inline functions?

No, it always used to return nullptr for function type, I just made it explicit.

I found this declaration in libio.h:

typedef __ssize_t __io_read_fn (void *__cookie, char *__buf, size_t __nbytes);

That caused an error because TypeDef was created with nullptr.

Actually I am not aware of where function types may be used and what should we do with them. Maybe we should create an issue for investigating it.

jonas · 2018-07-03T11:08:30Z

bindgen/ir/IR.cpp

@@ -317,7 +335,6 @@ T IR::getDeclarationWithName(std::vector<T> &declarations,
            return declaration;
        }
    }
-    llvm::errs() << "Failed to get declaration for " << name << "\n";


Should we throw an exception here to avoid segfaults.

No, nullptr is returned when:

struct or union type is needed by a function (or other declaration) but the type is not declared yet. So TypeDef with nullptr will be created.

When TreeVisitor visits struct/union declaration we need to check whether a TypeDef already exists for it.

I'll add comments about this to the code.

jonas · 2018-07-03T11:11:28Z

bindgen/ir/IR.cpp

+        std::make_shared<Union>(std::move(name), std::move(fields), maxSize);
+    unions.push_back(u);
+    typeDef.get()->setType(u);
+}


I suggest to unify addUnion and have it look up and conditional add/update the typedef. The same for addStruct.

jonas · 2018-07-03T11:13:39Z

bindgen/ir/IR.h

@@ -36,12 +37,24 @@ class IR {
    std::shared_ptr<Type>
    addStruct(std::string name, std::vector<Field *> fields, uint64_t typeSize);


Let's make it explicit that it returns a TypeDef.

jonas · 2018-07-03T11:54:31Z

bindgen/ir/TypeDef.cpp

@@ -2,9 +2,15 @@
 #include "../Utils.h"

 TypeDef::TypeDef(std::string name, std::shared_ptr<Type> type)
-    : TypeAndName(std::move(name), type) {}
+    : TypeAndName(std::move(name), std::move(type)) {}


Why is std:: move needed here? My understanding is that ownership does not have to be explicitly transferred or is this an optimization?

Yes, just a small optimization.

jonas · 2018-07-03T11:55:26Z

bindgen/ir/types/ArrayType.h

@@ -7,7 +7,7 @@ class ArrayType : public Type {
  public:
    ArrayType(std::shared_ptr<Type> elementsType, uint64_t size);

-    ~ArrayType() override;
+    ~ArrayType() override = default;


jonas · 2018-07-03T12:03:07Z

bindgen/visitor/TreeVisitor.cpp

@@ -115,7 +117,16 @@ void TreeVisitor::handleUnion(clang::RecordDecl *record, std::string name) {
        fields.push_back(new Field(fname, ftype));
    }

-    std::shared_ptr<Type> alias = ir.addUnion(name, std::move(fields), maxSize);
+    std::shared_ptr<Type> alias = nullptr;


There's no need to assign a default value

jonas · 2018-07-03T12:13:12Z

tests/samples/OpaqueTypes.h

+struct point *move(struct point *point, int x, int y);
+
+typedef struct points points;
+typedef union u u;


How about declaring u before points before point and have u use points which uses point? It would test that we generate types in the right order.

It will generate types in the order they appear in a header file.
It might be not a trivial task to sort them.

So in this example:

typedef struct points points; typedef struct point point; struct points { point *point1; point *point2; }; struct point { int x; int y; };

points appear before point. To sort them we need to find out that points actually uses point.
As I see it now we need to check for each type if it uses each other type and then do topological sort.
Tricky part is that there might be cyclic dependenceы:

struct b; struct c; struct a { struct b *b; }; struct b { struct c *c; }; struct c { struct a *a; };

I will create an issue about it

jonas · 2018-07-04T19:48:35Z

Thanks for fixing the broken merge

kornilova203 force-pushed the opaque-type branch 3 times, most recently from 786e311 to 72d8aee Compare July 2, 2018 13:30

kornilova203 changed the title ~~Handle opaque types~~ [WIP] Handle opaque types Jul 2, 2018

kornilova203 force-pushed the opaque-type branch from 72d8aee to 9433cd2 Compare July 3, 2018 09:06

Create TypeDef with nullptr for types that are not yet defined

5f1958d

kornilova203 force-pushed the opaque-type branch from 14411e2 to 5f1958d Compare July 3, 2018 09:41

kornilova203 changed the title ~~[WIP] Handle opaque types~~ Handle opaque types Jul 3, 2018

kornilova203 requested a review from jonas July 3, 2018 09:47

kornilova203 force-pushed the opaque-type branch from f8267c7 to 5f1958d Compare July 3, 2018 10:35

jonas reviewed Jul 3, 2018

View reviewed changes

kornilova203 added 2 commits July 3, 2018 18:55

Remove redundant aliasesMap

9834ee7

Check if TypeDef already exists in addStruct and addUnion methods

c244c88

kornilova203 requested a review from jonas July 4, 2018 05:29

jonas and others added 2 commits July 4, 2018 19:38

Merge branch 'master' into opaque-type

5c12773

Fix closing bracket

7d1f079

kornilova203 removed the request for review from jonas July 4, 2018 18:39

kornilova203 merged commit 753a512 into master Jul 4, 2018

kornilova203 deleted the opaque-type branch July 4, 2018 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle opaque types #94

Handle opaque types #94

kornilova203 commented Jul 2, 2018 •

edited

kornilova203 commented Jul 3, 2018

jonas Jul 3, 2018

kornilova203 Jul 3, 2018

jonas Jul 3, 2018

kornilova203 Jul 3, 2018

jonas Jul 3, 2018

jonas Jul 3, 2018

jonas Jul 3, 2018

kornilova203 Jul 3, 2018

jonas Jul 3, 2018

jonas Jul 3, 2018

jonas Jul 3, 2018 •

edited

kornilova203 Jul 4, 2018 •

edited

jonas commented Jul 4, 2018

		@@ -36,12 +37,24 @@ class IR {
		std::shared_ptr<Type>
		addStruct(std::string name, std::vector<Field *> fields, uint64_t typeSize);

Handle opaque types #94

Handle opaque types #94

Conversation

kornilova203 commented Jul 2, 2018 • edited

kornilova203 commented Jul 3, 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

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

jonas Jul 3, 2018 • edited

Choose a reason for hiding this comment

kornilova203 Jul 4, 2018 • edited

Choose a reason for hiding this comment

jonas commented Jul 4, 2018

kornilova203 commented Jul 2, 2018 •

edited

jonas Jul 3, 2018 •

edited

kornilova203 Jul 4, 2018 •

edited