Skip to content

Append ref qualifier to signed-as type on 'this'#20

Open
slburson wants to merge 1 commit into
borodust:masterfrom
slburson:ref-qualifiers
Open

Append ref qualifier to signed-as type on 'this'#20
slburson wants to merge 1 commit into
borodust:masterfrom
slburson:ref-qualifiers

Conversation

@slburson
Copy link
Copy Markdown

This PR goes with libresect #13 and cl-resect #1.

The problem was that I was getting duplicate-definition errors from SBCL (well, it calls them warnings, but then it aborts compilation) on operator= methods. The cause turned out to be that some pairs of these in libtorch were distinguished only by ref qualifiers, which Claw was not capturing. So then iffi:defifun would emit defuns for both of them, causing the error.

I have arranged for the qualifier to appear in the :signed-as type for the this parameter.

@borodust
Copy link
Copy Markdown
Owner

Can you drop a link to the header with the offending C++ functions? I would like to have a look what libtorch does.

@slburson
Copy link
Copy Markdown
Author

@borodust
Copy link
Copy Markdown
Owner

Hm-m. I was pretty sure I've added support for these kinds, because I've had similar issues in the libraries I'd wrapped. I'll have another look.

@borodust
Copy link
Copy Markdown
Owner

Like these

@borodust
Copy link
Copy Markdown
Owner

C++ methods with different signatures should have had different mangled names and that should have guaranteed unique defun expansions. It's strange you've got the warning from SBCL. Did the methods have the same mangled name in your case?

@slburson
Copy link
Copy Markdown
Author

The mangled names were different, but the Lisp names were the same. The signed-param-types computed in iffi:defifun were also the same. defifun suppresses the defun in all but one case, where those cases are distinguished by signed-param-types. So if two defifun forms for the same name have the same signed-param-types, it will emit defun forms for both of them.

@slburson
Copy link
Copy Markdown
Author

Oh, when SBCL says "duplicate definition" it doesn't mean the two defun forms were identical; it just means they defined the same symbol.

@slburson
Copy link
Copy Markdown
Author

Output from the first link above, before my change (ignore the slight discrepancy in line numbers):

(iffi:defifun ("__claw__ZNR2at6TensoraSEOS0_" %torch::at+operator=)
    (:pointer %torch::at+tensor)
  "Tensor &at::Tensor::operator=(Tensor &&x) & noexcept;
/home/gyro/Repos/Claw-Torch/src/lib/torch-bin/include/ATen/core/TensorBody.h:214:11"
  (%torch::%%claw-this- (:pointer %torch::at+tensor))
  (%torch::x (:pointer %torch::at+tensor) :signed-as (:reference %torch::at+tensor :rvalue)))

(iffi:defifun ("__claw__ZNO2at6TensoraSEOS0_" %torch::at+operator=)
    (:pointer %torch::at+tensor)
  "Tensor &at::Tensor::operator=(Tensor &&rhs) &&;
/home/gyro/Repos/Claw-Torch/src/lib/torch-bin/include/ATen/core/TensorBody.h:226:11"
  (%torch::%%claw-this- (:pointer %torch::at+tensor))
  (%torch::rhs (:pointer %torch::at+tensor) :signed-as (:reference %torch::at+tensor :rvalue)))

After the change:

(iffi:defifun ("__claw__ZNR2at6TensoraSEOS0_" %torch::at+operator=)
    (:pointer %torch::at+tensor)
  "Tensor &at::Tensor::operator=(Tensor &&x) & noexcept;
/home/gyro/Repos/Claw-Torch/libtorch/include/ATen/core/TensorBody.h:214:11"
  (%torch::%%claw-this- (:pointer %torch::at+tensor) :signed-as (:pointer %torch::at+tensor :lvalue))
  (%torch::x (:pointer %torch::at+tensor) :signed-as (:reference %torch::at+tensor :rvalue)))

(iffi:defifun ("__claw__ZNO2at6TensoraSEOS0_" %torch::at+operator=)
    (:pointer %torch::at+tensor)
  "Tensor &at::Tensor::operator=(Tensor &&rhs) &&;
/home/gyro/Repos/Claw-Torch/libtorch/include/ATen/core/TensorBody.h:226:11"
  (%torch::%%claw-this- (:pointer %torch::at+tensor) :signed-as (:pointer %torch::at+tensor :rvalue))
  (%torch::rhs (:pointer %torch::at+tensor) :signed-as (:reference %torch::at+tensor :rvalue)))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants