Current Broken Specs and Issues

[kalinon]

@docelic I noticed we were both fixing some of the same stuff, so here is what i have left and what ive found.

Broken specs:

crystal spec spec/integration/containers_spec.cr:4 # container instantiation feature works
crystal spec spec/integration/arguments_spec.cr:4 # the argument translation functionality works

arguments_spec:

in [spec/integration/arguments.cpp] we have a test for setting the default string:

  int defaultString(std::string str = "Okay") {
    return str.length();
  }

However bindgen is currently outputting the following JSON when parsing it:

{
          "type": "MemberMethod",
          "access": "Public",
          "name": "defaultString",
          "isConst": false,
          "isVirtual": false,
          "isPure": false,
          "isExternC": false,
          "className": "Defaults",
          "firstDefaultArgument": 0,
          "arguments": [
            {
              "isConst": false,
              "isMove": false,
              "isReference": false,
              "isBuiltin": false,
              "isVoid": false,
              "pointer": 0,
              "baseName": "std::string",
              "fullName": "std::string",
              "template": {
                "baseName": "std::__1::basic_string",
                "fullName": "std::basic_string<char, std::char_traits<char>, std::allocator<char> >",
                "arguments": [
                  {
                    "isConst": false,
                    "isMove": false,
                    "isReference": false,
                    "isBuiltin": true,
                    "isVoid": false,
                    "pointer": 0,
                    "baseName": "char",
                    "fullName": "char",
                    "template": null
                  },
                  {
                    "isConst": false,
                    "isMove": false,
                    "isReference": false,
                    "isBuiltin": false,
                    "isVoid": false,
                    "pointer": 0,
                    "baseName": "std::char_traits<char>",
                    "fullName": "std::char_traits<char>",
                    "template": {
                      "baseName": "std::__1::char_traits",
                      "fullName": "std::char_traits<char>",
                      "arguments": [
                        {
                          "isConst": false,
                          "isMove": false,
                          "isReference": false,
                          "isBuiltin": true,
                          "isVoid": false,
                          "pointer": 0,
                          "baseName": "char",
                          "fullName": "char",
                          "template": null
                        }
                      ]
                    }
                  },
                  {
                    "isConst": false,
                    "isMove": false,
                    "isReference": false,
                    "isBuiltin": false,
                    "isVoid": false,
                    "pointer": 0,
                    "baseName": "std::allocator<char>",
                    "fullName": "std::allocator<char>",
                    "template": {
                      "baseName": "std::__1::allocator",
                      "fullName": "std::allocator<char>",
                      "arguments": [
                        {
                          "isConst": false,
                          "isMove": false,
                          "isReference": false,
                          "isBuiltin": true,
                          "isVoid": false,
                          "pointer": 0,
                          "baseName": "char",
                          "fullName": "char",
                          "template": null
                        }
                      ]
                    }
                  }
                ]
              },
              "hasDefault": true,
              "isVariadic": false,
              "name": "str",
              "value": null
            }
          ],
          "returnType": {
            "isConst": false,
            "isMove": false,
            "isReference": false,
            "isBuiltin": true,
            "isVoid": false,
            "pointer": 0,
            "baseName": "int",
            "fullName": "int",
            "template": null
          }
        }

Notice in particular:

              "hasDefault": true,
              "isVariadic": false,
              "name": "str",
              "value": null

So while it is noticing that there is a default value, the value itself is being returned as null. This is happening in [clang/src/type_helper.cpp]

Argument TypeHelper::processFunctionParameter(const clang::ParmVarDecl *decl) {
    clang::ASTContext &ctx = decl->getASTContext();
    Argument arg;

    clang::QualType qt = decl->getType();
    qualTypeToType(arg, qt, ctx);
    arg.name = decl->getQualifiedNameAsString();
    arg.isVariadic = false;
    arg.hasDefault = decl->hasDefaultArg();
    arg.value = JsonStream::Null;

    // If the parameter has a default value, try to figure out this value.  Can
    // fail if e.g. the call has side-effects (Like calling another method).  Will
    // work for constant expressions though, like `true` or `3 + 5`.
    if (arg.hasDefault) {
        TypeHelper::readValue(arg.value, qt, ctx, decl->getDefaultArg());
    }

    return arg;
}

and paricularly in:

bool TypeHelper::valueFromApValue(LiteralData &value, const clang::APValue &apValue, const clang::QualType &qt) {
    if (qt->isPointerType()) {
        // For a pointer-type, just store if it was `nullptr` (== true).
        value = apValue.isNullPointer();
    } else if (qt->isBooleanType()) {

Where it sets it as a nullptr

[kalinon]

Im not very good at C++ so i may need assistance resolving these last ones

[ZaWertun]

I see no errors after running crystal spec on my fork...

[kalinon]

I see no errors after running crystal spec on my fork...

Ill check it out. Are you also using latest crystal? (0.34.0)

[ZaWertun]

I see no errors after running crystal spec on my fork...

Ill check it out. Are you also using latest crystal? (0.34.0)

Yep.

$ crystal version                                                                                                                                                                                                                                            541.138s (0) 14:49:27
Crystal 0.34.0 (2020-04-07)

LLVM: 10.0.0
Default target: x86_64-unknown-linux-gnu

[kalinon]

@ZaWertun some of your changes to clang/find_clang.cr broke things for me, other than that, most merged without issue. but i still end up with the same 2 failures. For reference i am testing on OSX.

[ZaWertun]

@kalinon I have Mac Mini at home with almost latest mac os version, I can check this. Is it hard to install crystal on Mac?

[kalinon]

Nope its easy peazy. as long as you have brew installed just do a brew install crystal. Also different versions of llvm are super easy too brew install llvm@4 llvm@6 llvm@8

[ZaWertun]

Nope its easy peazy. as long as you have brew installed just do a brew install crystal. Also different versions of llvm are super easy too brew install llvm@4 llvm@6 llvm@8

Somehow it works, but only when built with llvm-10, I've got errors ld: symbol(s) not found for architecture x86_64 when building it with llvm-8. See this commit: https://github.com/ZaWertun/bindgen/commit/2745ad87eaead978dc29e16ffcad3e98d15a56e5. I think it will be far more simple to use just llvm-config.

[docelic]

Here's the TODO items gathered from the PRs:

1. The JSON UInt64 issue:

   +      value = v.getZExtValue();
   +      // FIXME: Perhaps we need to convert it to string because JSON does not support uint64?
   +      // Then translate it on the other end?
   +      // value = std::to_string(v.getZExtValue());

2. Temporary MacOS path additions

   --- a/spec/integration/spec_base.yml
   +++ b/spec/integration/spec_base.yml
   +    # FIXME: This is not an appropriate fix, however without it fails on OSX
   +    - /usr/local/include/
   +    - /usr/local/Cellar/llvm@6/6.0.1_2/include/c++/v1
   +    - /usr/local/Cellar/llvm@6/6.0.1_2/lib/clang/6.0.1/include

3. Crash in clang destructor

   @@ -100,6 +100,9 @@ void BindgenASTConsumer::serializeAndOutput() {
   
       stream << "macros" << JsonStream::Separator << this->m_macros;  // "macros": [ ... ]
       stream << JsonStream::ObjectEnd; // }
   +
   +       // FIXME: Currently the process crashes during clang's Parser destructor. This is a workaround.
   +       exit(0);
   }

[docelic]

4. Possibly add real option parsing to find_clang.cr. (This would be a further extension of an improvement done in https://github.com/Papierkorb/bindgen/commit/151c70723c9a5444f6e23edd62c0eb54c66c95ee )

[kalinon]

Opened #33 which should fix the arguments spec. As for the containers spec, i think i know why its failing, but its making my brain hurt.

So the error is:

In /usr/local/Cellar/crystal/0.34.0/src/indexable.cr:26:16

 26 | abstract def unsafe_fetch(index : Int)
                   ^-----------

Which in 0.34.0 crystal is more "uptight" about overrides. The generated code is:

    def unsafe_fetch(index : Int32) : String
      String.new(Binding.bg_Container_std__vector_std__string_at_int(self, index).ptr, Binding.bg_Container_std__vector_std__string_at_int(self, index).size)
    end

So i think we need to make it take index as an Int but i cant figure out where to override it.

[ZaWertun]

Opened #33 which should fix the arguments spec. As for the containers spec, i think i know why its failing, but its making my brain hurt.

So the error is:

In /usr/local/Cellar/crystal/0.34.0/src/indexable.cr:26:16

 26 | abstract def unsafe_fetch(index : Int)
                   ^-----------

Which in 0.34.0 crystal is more "uptight" about overrides. The generated code is:

    def unsafe_fetch(index : Int32) : String
      String.new(Binding.bg_Container_std__vector_std__string_at_int(self, index).ptr, Binding.bg_Container_std__vector_std__string_at_int(self, index).size)
    end

So i think we need to make it take index as an Int but i cant figure out where to override it.

unsafe_fetch has wrong signature - index argument must be of Int type (https://crystal-lang.org/api/0.34.0/Int.html).

[ZaWertun]

This patch should help, but there will be another error:

diff --git a/builtin_types.yml b/builtin_types.yml
index e8ad0a7..5ecee02 100644
--- a/builtin_types.yml
+++ b/builtin_types.yml
@@ -15,6 +15,7 @@ short: { binding_type: Int16, kind: Struct, builtin: true }
 int: { binding_type: Int32, kind: Struct, builtin: true }
 unsigned: { binding_type: UInt32, kind: Struct, builtin: true }
 "unsigned int": { binding_type: UInt32, kind: Struct, builtin: true }
+_Int: { binding_type: Int, crystal_type: Int, cpp_type: int, kind: Struct, builtin: true }

 # Long types
 "long": { binding_type: "LibC::Long", kind: Struct, builtin: true }
diff --git a/src/bindgen/processor/instantiate_containers.cr b/src/bindgen/processor/instantiate_containers.cr
index cd01473..9641e63 100644
--- a/src/bindgen/processor/instantiate_containers.cr
+++ b/src/bindgen/processor/instantiate_containers.cr
@@ -168,7 +168,7 @@ origin: origin,

       # Builds the access method for the *klass_name* of a instantiated container.
       private def access_method(container : Configuration::Container, klass_name : String, var_type : Parser::Type) : Parser::Method
-        idx_type = Parser::Type.builtin_type(CPP_INTEGER_TYPE)
+        idx_type = Parser::Type.builtin_type("_Int")
         idx_arg = Parser::Argument.new("index", idx_type)

         Parser::Method.build(

New error:

In tmp/containers.cr:99:70

 99 | fun bg_Container_std__vector_int_at__Int(_self_ : Void*, index : Int) : Int32
                                                                       ^--
Error: only primitive types, pointers, structs, unions, enums and tuples are allowed in lib declarations, not Int (did you mean LibC::Int?)

[ZaWertun]

Opened #33 which should fix the arguments spec. As for the containers spec, i think i know why its failing, but its making my brain hurt.

So the error is:

In /usr/local/Cellar/crystal/0.34.0/src/indexable.cr:26:16

 26 | abstract def unsafe_fetch(index : Int)
                   ^-----------

Which in 0.34.0 crystal is more "uptight" about overrides. The generated code is:

    def unsafe_fetch(index : Int32) : String
      String.new(Binding.bg_Container_std__vector_std__string_at_int(self, index).ptr, Binding.bg_Container_std__vector_std__string_at_int(self, index).size)
    end

So i think we need to make it take index as an Int but i cant figure out where to override it.

Here is fix: #34.

[docelic]

At this point all unresolved issues mentioned in this ticket exist as separate issues, so closing this. Feel free to reopen if you want to continue this thread/ticket.

[kalinon]

Well done!